Is filter.<driver>.required gitoxide-specific?

[EliahKagan]

gix-filter/tests/fixtures/baseline.sh sets filter.arrow.required to true (where arrow is the arrow.rs example in gix-filter) for all three of the sub-cases it prepares. For example:

gitoxide/gix-filter/tests/fixtures/baseline.sh

Line 20 in 8df0db2

git config filter.arrow.required true

filter.<driver>.required is not documented in git-config(1). But gix-filter does recognize it. It sets the value of:

gitoxide/gix-filter/src/lib.rs

Lines 69 to 73 in 8df0db2

	/// If `true`, the `clean` or `smudge` programs need to succeed in order to make their content usable. Otherwise their
	/// exit code is ignored.
	/// Note that this is more of a suggestion as we will always report errors as they happen as the driver API is streaming in nature,
	/// which makes soft-failures impossible unless the caller takes precautions.
	pub required: bool,

Is this specific to gitoxide, or is it also something Git has but is not documented (or not where I looked)?

[Byron]

That's good to bring up, and I wonder if its 'source' could be made clearer.

It's 'soft'-documented as gitattribute, as all one can do is search for 'required' in the text to find mentions of it.

[EliahKagan]

Thanks. It looks like I just missed it there in gitattributes(5). I was even reading the subsequent two sections, on long running filter processes and delays, recently, while researching GHSA-fqmf-w4xh-33rh (as well a forthcoming non-security bug), yet still missed it.

So I'm not sure anything special has to be done to point people from those gix-filter comments to gitattributes(5). (Probably it should be listed in git-config(1) as well, but I don't know that its omission there means there's anything that needs to be done in gitoxide documentation comments.)

However, this does make me wonder if there are significant differences in the implications of setting required in Git versus gitoxide. Maybe this should be expanded to make its implications clearer:

gitoxide/gix-filter/src/lib.rs

Lines 71 to 72 in 8df0db2

	/// Note that this is more of a suggestion as we will always report errors as they happen as the driver API is streaming in nature,
	/// which makes soft-failures impossible unless the caller takes precautions.

[Byron]

That's good to hear!

I thought I had implemented it similarly to how it's documented in .gitattributes, but the documentation I left certainly doesn't make the connection to it easy. At least I wasn't aware of a deviation as I would have documented that, at least.

Do you know what should be done here?

[EliahKagan]

Do you know what should be done here?

Unfortunately, I do not. To be clear, I am not saying that there is any problem with the implementation of required in gitoxide. I don't know much about the required attribute, and certainly nothing more than it documented for it in the associated gitoxide documentation comment and in gitattributes(5).

(I just learned about it yesterday while comparing a semi-manual test for an issue about delayed process filters to the existing automated tests in gitoxide. It looks like required does not make a difference in the context of that forthcoming issue.)

[Byron]

Even though I am not sure what the answer is, the effect of this flag fortunately boils down to a single location which is here:

gitoxide/gix-filter/src/driver/apply.rs

Lines 228 to 236 in 969ff0f

	if let Some((mut child, cmd)) = self.child.take() {
	let status = child.wait()?;
	if !status.success() {
	return Err(std::io::Error::new(
	std::io::ErrorKind::Other,
	format!("Driver process {cmd:?} failed"),
	));
	}
	}

child is only Some(_) if required == true, and leads to a status check after reading from the filter.
From that point of view I think the documentation is correct - maybe there can be improvements though, particularly in relation to the testing you are conducting.