Re: [RFC PATCH] for-each-ref: respect GIT_REF_PARANOIA setting

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 21, 2022, 17:29 UTC
Message-ID: <220321.86wngn8arz.gmgdl@evledraar.gmail.com>
In-Reply-To: <7283f826198aaec94c847f0b26e228ace9a38433.1647880211.git.me@ttaylorr.com>

On Mon, Mar 21 2022, Taylor Blau wrote:

Show 23 quoted lines

> When setting GIT_REF_PARANOIA=1 (which became the default in 968f12fdac
> (refs: turn on GIT_REF_PARANOIA by default, 2021-09-24)), `git
> for-each-ref` will display a warning when it encounters a broken ref,
> but does not terminate the program.
>
> This can result in somewhat surprising behavior like:
>
>     $ echo bogus >.git/refs/heads/bogus
>     $ GIT_REF_PARANOIA=1 git for-each-ref; echo "==> $?"
>     warning: ignoring broken ref refs/heads/bogus
>     ==> 0
>
> This seems to be the case even before the introduction of the ref-filter
> code via 7ebc8cbedd (Merge branch 'kn/for-each-ref', 2015-08-03).
> Looking at 8afc493d11 (for-each-ref: report broken references correctly,
> 2015-06-02) when this was last addressed, the fix at the time was to
> report any broken references, but this warning did not affect the
> command's success.
>
> It seems that `git for-each-ref` should exit non-zero in the case of a
> broken reference when GIT_REF_PARANOIA is set to 1. This patch does
> that, but there are a couple of open questions (hence its status as an
> RFC):

I started playing with this fix-up:
	
	diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
	index e1937b7e53e..05b7c9b2a69 100644
	--- a/builtin/for-each-ref.c
	+++ b/builtin/for-each-ref.c
	@@ -78,8 +78,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
	 	filter.name_patterns = argv;
	 	filter.match_as_path = 1;
	 	ret = filter_refs(&array, &filter, FILTER_REFS_ALL);
	-	if (ret)
	-		goto cleanup;
	 	ref_array_sort(sorting, &array);
	 
	 	if (!maxcount || array.nr < maxcount)
	@@ -93,7 +91,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
	 		putchar('\n');
	 	}
	 
	-cleanup:
	 	strbuf_release(&err);
	 	strbuf_release(&output);
	 	ref_array_clear(&array);
	diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh
	index 2a5895c124a..8a93685bb3e 100755
	--- a/t/t6301-for-each-ref-errors.sh
	+++ b/t/t6301-for-each-ref-errors.sh
	@@ -19,7 +19,8 @@ test_expect_success 'Broken refs are reported correctly' '
	 	: >.git/$r &&
	 	test_when_finished "rm -f .git/$r" &&
	 	echo "warning: ignoring broken ref $r" >broken-err &&
	-	test_must_fail git for-each-ref 2>err &&
	+	test_must_fail git for-each-ref >out 2>err &&
	+	test_must_be_empty out &&
	 	test_cmp broken-err err
	 '
	 
	@@ -31,7 +32,11 @@ test_expect_success 'NULL_SHA1 refs are reported correctly' '
	 	test_must_fail git for-each-ref 2>err &&
	 	test_cmp zeros-err err &&
	 	test_must_fail git for-each-ref --format="%(objectname) %(refname)" \
	-		2>brief-err &&
	+		>actual 2>brief-err &&
	+	cat >expect <<-EOF &&
	+	$(git rev-parse HEAD) $(git rev-parse --symbolic-full-name HEAD)
	+	EOF
	+	test_cmp expect actual &&
	 	test_cmp zeros-err brief-err
	 '

It seems to me you'd want at least the part of it where we retain a test_cmp or similar for the "out", rather than removing it completely.

Show 23 quoted lines

>   - First, there are a handful of other `filter_refs()` calls throughout
>     the builtin tree that lack similar error handling. I suspect that
>     these will need similar treatment, but I haven't looked at them
>     deeply yet.
>
>   - More pressing, though, is that there is some test fallout as a
>     result of this change. Namely, t6301 expects that `git for-each-ref`
>     should list out all of the non-broken references to stdout _even
>     when broken references exist_.
>
>     This patch changes that behavior and causes us to exit immediately,
>     without printing out any of the non-broken references (since we are
>     still building up the list of references to sort, and thus haven't
>     printed anything out by the time we're in the ref_filter_handler
>     callback).
>
>     The test fallout can be seen in the changes to t6301, namely that we
>     expect `for-each-ref` to fail in certain cases where it didn't
>     before, and that in those cases we no longer guarantee the contents
>     of stdout.
>
> The second point gives me serious pause about whether or not this change
> is the right one. So I'm curious if or how we should handle this case.

I think the right thing to do is to add a filter_refs() where we add FILTER_REFS_WANT_REF_ISBROKEN and maybe FILTER_REFS_WANT_REF_BAD_NAME.

Then have in the for-each-ref.c loop check the "flags" on the ref item for REF_ISBROKEN etc, and either have format_ref_array_item() format it, or skip that and issue a warning() there manually.

That way API users can opt-in and detect these, but still be able to print the rest, and it allows to make the more narrow change of amending the exit code.

I'd think builtin/{tag,branch}.c would want similar treatment, ditto fetch-pack.c.

But I really don't see why we'd want to abort the iteration early just because we see one broken ref, that's after all per-ref issue, and we don't need to abort the walk entirely to issue a warning() or to change our exit code.

So that part of the patch really seems like it's in the wrong place to me, and that we should always have for-each-ref try as hard as it can to emit the output it can emit, even if we change the exit code due to a broken ref.