On Mon, Mar 21 2022, Taylor Blau wrote:
> 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.
> - 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.
