{"thread":{"id":"57593","subject":"[RFC PATCH] for-each-ref: respect GIT_REF_PARANOIA setting","startedAt":"2022-03-21T16:30:52Z","lastAt":"2022-03-21T20:33:13Z","messageCount":3,"participants":["Taylor Blau","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"451732","messageId":"7283f826198aaec94c847f0b26e228ace9a38433.1647880211.git.me@ttaylorr.com","threadId":"57593","inReplyTo":null,"subject":"[RFC PATCH] for-each-ref: respect GIT_REF_PARANOIA setting","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-21T16:30:18Z","receivedAt":"2022-03-21T16:30:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When setting GIT_REF_PARANOIA=1 (which became the default in 968f12fdac\n(refs: turn on GIT_REF_PARANOIA by default, 2021-09-24)), `git\nfor-each-ref` will display a warning when it encounters a broken ref,\nbut does not terminate the program.\n\nThis can result in somewhat surprising behavior like:\n\n    $ echo bogus >.git/refs/heads/bogus\n    $ GIT_REF_PARANOIA=1 git for-each-ref; echo \"==> $?\"\n    warning: ignoring broken ref refs/heads/bogus\n    ==> 0\n\nThis seems to be the case even before the introduction of the ref-filter\ncode via 7ebc8cbedd (Merge branch 'kn/for-each-ref', 2015-08-03).\nLooking at 8afc493d11 (for-each-ref: report broken references correctly,\n2015-06-02) when this was last addressed, the fix at the time was to\nreport any broken references, but this warning did not affect the\ncommand's success.\n\nIt seems that `git for-each-ref` should exit non-zero in the case of a\nbroken reference when GIT_REF_PARANOIA is set to 1. This patch does\nthat, but there are a couple of open questions (hence its status as an\nRFC):\n\n  - First, there are a handful of other `filter_refs()` calls throughout\n    the builtin tree that lack similar error handling. I suspect that\n    these will need similar treatment, but I haven't looked at them\n    deeply yet.\n\n  - More pressing, though, is that there is some test fallout as a\n    result of this change. Namely, t6301 expects that `git for-each-ref`\n    should list out all of the non-broken references to stdout _even\n    when broken references exist_.\n\n    This patch changes that behavior and causes us to exit immediately,\n    without printing out any of the non-broken references (since we are\n    still building up the list of references to sort, and thus haven't\n    printed anything out by the time we're in the ref_filter_handler\n    callback).\n\n    The test fallout can be seen in the changes to t6301, namely that we\n    expect `for-each-ref` to fail in certain cases where it didn't\n    before, and that in those cases we no longer guarantee the contents\n    of stdout.\n\nThe second point gives me serious pause about whether or not this change\nis the right one. So I'm curious if or how we should handle this case.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/for-each-ref.c         |  9 ++++++---\n ref-filter.c                   |  2 +-\n t/t6301-for-each-ref-errors.sh | 10 ++++------\n 3 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 6f62f40d12..e1937b7e53 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -19,7 +19,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tint i;\n \tstruct ref_sorting *sorting;\n \tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n-\tint maxcount = 0, icase = 0;\n+\tint maxcount = 0, icase = 0, ret = 0;\n \tstruct ref_array array;\n \tstruct ref_filter filter;\n \tstruct ref_format format = REF_FORMAT_INIT;\n@@ -77,7 +77,9 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \n \tfilter.name_patterns = argv;\n \tfilter.match_as_path = 1;\n-\tfilter_refs(&array, &filter, FILTER_REFS_ALL);\n+\tret = filter_refs(&array, &filter, FILTER_REFS_ALL);\n+\tif (ret)\n+\t\tgoto cleanup;\n \tref_array_sort(sorting, &array);\n \n \tif (!maxcount || array.nr < maxcount)\n@@ -91,11 +93,12 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\tputchar('\\n');\n \t}\n \n+cleanup:\n \tstrbuf_release(&err);\n \tstrbuf_release(&output);\n \tref_array_clear(&array);\n \tfree_commit_list(filter.with_commit);\n \tfree_commit_list(filter.no_commit);\n \tref_sorting_release(sorting);\n-\treturn 0;\n+\treturn ret;\n }\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 7838bd22b8..f9667f6ca4 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2249,7 +2249,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \n \tif (flag & REF_ISBROKEN) {\n \t\twarning(_(\"ignoring broken ref %s\"), refname);\n-\t\treturn 0;\n+\t\treturn 1;\n \t}\n \n \t/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */\ndiff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh\nindex 40edf9dab5..2a5895c124 100755\n--- a/t/t6301-for-each-ref-errors.sh\n+++ b/t/t6301-for-each-ref-errors.sh\n@@ -19,8 +19,7 @@ test_expect_success 'Broken refs are reported correctly' '\n \t: >.git/$r &&\n \ttest_when_finished \"rm -f .git/$r\" &&\n \techo \"warning: ignoring broken ref $r\" >broken-err &&\n-\tgit for-each-ref >out 2>err &&\n-\ttest_cmp full-list out &&\n+\ttest_must_fail git for-each-ref 2>err &&\n \ttest_cmp broken-err err\n '\n \n@@ -29,11 +28,10 @@ test_expect_success 'NULL_SHA1 refs are reported correctly' '\n \techo $ZEROS >.git/$r &&\n \ttest_when_finished \"rm -f .git/$r\" &&\n \techo \"warning: ignoring broken ref $r\" >zeros-err &&\n-\tgit for-each-ref >out 2>err &&\n-\ttest_cmp full-list out &&\n+\ttest_must_fail git for-each-ref 2>err &&\n \ttest_cmp zeros-err err &&\n-\tgit for-each-ref --format=\"%(objectname) %(refname)\" >brief-out 2>brief-err &&\n-\ttest_cmp brief-list brief-out &&\n+\ttest_must_fail git for-each-ref --format=\"%(objectname) %(refname)\" \\\n+\t\t2>brief-err &&\n \ttest_cmp zeros-err brief-err\n '\n \n-- \n2.35.1.73.gccc5557600\n"},{"id":"451739","messageId":"220321.86wngn8arz.gmgdl@evledraar.gmail.com","threadId":"57593","inReplyTo":"7283f826198aaec94c847f0b26e228ace9a38433.1647880211.git.me@ttaylorr.com","subject":"Re: [RFC PATCH] for-each-ref: respect GIT_REF_PARANOIA setting","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-21T17:29:38Z","receivedAt":"2022-03-21T17:40:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 21 2022, Taylor Blau wrote:\n\n> When setting GIT_REF_PARANOIA=1 (which became the default in 968f12fdac\n> (refs: turn on GIT_REF_PARANOIA by default, 2021-09-24)), `git\n> for-each-ref` will display a warning when it encounters a broken ref,\n> but does not terminate the program.\n>\n> This can result in somewhat surprising behavior like:\n>\n>     $ echo bogus >.git/refs/heads/bogus\n>     $ GIT_REF_PARANOIA=1 git for-each-ref; echo \"==> $?\"\n>     warning: ignoring broken ref refs/heads/bogus\n>     ==> 0\n>\n> This seems to be the case even before the introduction of the ref-filter\n> code via 7ebc8cbedd (Merge branch 'kn/for-each-ref', 2015-08-03).\n> Looking at 8afc493d11 (for-each-ref: report broken references correctly,\n> 2015-06-02) when this was last addressed, the fix at the time was to\n> report any broken references, but this warning did not affect the\n> command's success.\n>\n> It seems that `git for-each-ref` should exit non-zero in the case of a\n> broken reference when GIT_REF_PARANOIA is set to 1. This patch does\n> that, but there are a couple of open questions (hence its status as an\n> RFC):\n\nI started playing with this fix-up:\n\t\n\tdiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n\tindex e1937b7e53e..05b7c9b2a69 100644\n\t--- a/builtin/for-each-ref.c\n\t+++ b/builtin/for-each-ref.c\n\t@@ -78,8 +78,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n\t \tfilter.name_patterns = argv;\n\t \tfilter.match_as_path = 1;\n\t \tret = filter_refs(&array, &filter, FILTER_REFS_ALL);\n\t-\tif (ret)\n\t-\t\tgoto cleanup;\n\t \tref_array_sort(sorting, &array);\n\t \n\t \tif (!maxcount || array.nr < maxcount)\n\t@@ -93,7 +91,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n\t \t\tputchar('\\n');\n\t \t}\n\t \n\t-cleanup:\n\t \tstrbuf_release(&err);\n\t \tstrbuf_release(&output);\n\t \tref_array_clear(&array);\n\tdiff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh\n\tindex 2a5895c124a..8a93685bb3e 100755\n\t--- a/t/t6301-for-each-ref-errors.sh\n\t+++ b/t/t6301-for-each-ref-errors.sh\n\t@@ -19,7 +19,8 @@ test_expect_success 'Broken refs are reported correctly' '\n\t \t: >.git/$r &&\n\t \ttest_when_finished \"rm -f .git/$r\" &&\n\t \techo \"warning: ignoring broken ref $r\" >broken-err &&\n\t-\ttest_must_fail git for-each-ref 2>err &&\n\t+\ttest_must_fail git for-each-ref >out 2>err &&\n\t+\ttest_must_be_empty out &&\n\t \ttest_cmp broken-err err\n\t '\n\t \n\t@@ -31,7 +32,11 @@ test_expect_success 'NULL_SHA1 refs are reported correctly' '\n\t \ttest_must_fail git for-each-ref 2>err &&\n\t \ttest_cmp zeros-err err &&\n\t \ttest_must_fail git for-each-ref --format=\"%(objectname) %(refname)\" \\\n\t-\t\t2>brief-err &&\n\t+\t\t>actual 2>brief-err &&\n\t+\tcat >expect <<-EOF &&\n\t+\t$(git rev-parse HEAD) $(git rev-parse --symbolic-full-name HEAD)\n\t+\tEOF\n\t+\ttest_cmp expect actual &&\n\t \ttest_cmp zeros-err brief-err\n\t '\n\nIt seems to me you'd want at least the part of it where we retain a\ntest_cmp or similar for the \"out\", rather than removing it completely.\n\n>   - First, there are a handful of other `filter_refs()` calls throughout\n>     the builtin tree that lack similar error handling. I suspect that\n>     these will need similar treatment, but I haven't looked at them\n>     deeply yet.\n>\n>   - More pressing, though, is that there is some test fallout as a\n>     result of this change. Namely, t6301 expects that `git for-each-ref`\n>     should list out all of the non-broken references to stdout _even\n>     when broken references exist_.\n>\n>     This patch changes that behavior and causes us to exit immediately,\n>     without printing out any of the non-broken references (since we are\n>     still building up the list of references to sort, and thus haven't\n>     printed anything out by the time we're in the ref_filter_handler\n>     callback).\n>\n>     The test fallout can be seen in the changes to t6301, namely that we\n>     expect `for-each-ref` to fail in certain cases where it didn't\n>     before, and that in those cases we no longer guarantee the contents\n>     of stdout.\n>\n> The second point gives me serious pause about whether or not this change\n> is the right one. So I'm curious if or how we should handle this case.\n\nI think the right thing to do is to add a filter_refs() where we add\nFILTER_REFS_WANT_REF_ISBROKEN and maybe FILTER_REFS_WANT_REF_BAD_NAME.\n\nThen have in the for-each-ref.c loop check the \"flags\" on the ref item\nfor REF_ISBROKEN etc, and either have format_ref_array_item() format it,\nor skip that and issue a warning() there manually.\n\nThat way API users can opt-in and detect these, but still be able to\nprint the rest, and it allows to make the more narrow change of amending\nthe exit code.\n\nI'd think builtin/{tag,branch}.c would want similar treatment, ditto\nfetch-pack.c.\n\nBut I really don't see why we'd want to abort the iteration early just\nbecause we see one broken ref, that's after all per-ref issue, and we\ndon't need to abort the walk entirely to issue a warning() or to change\nour exit code.\n\nSo that part of the patch really seems like it's in the wrong place to\nme, and that we should always have for-each-ref try as hard as it can to\nemit the output it can emit, even if we change the exit code due to a\nbroken ref.\n\n"},{"id":"451759","messageId":"xmqqzgljvyf3.fsf@gitster.g","threadId":"57593","inReplyTo":"7283f826198aaec94c847f0b26e228ace9a38433.1647880211.git.me@ttaylorr.com","subject":"Re: [RFC PATCH] for-each-ref: respect GIT_REF_PARANOIA setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-21T20:33:04Z","receivedAt":"2022-03-21T20:33:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> +\tret = filter_refs(&array, &filter, FILTER_REFS_ALL);\n> +\tif (ret)\n> +\t\tgoto cleanup;\n>  \tref_array_sort(sorting, &array);\n\nFixing the \"regression\" would be a simple matter of removing the\nearly return from here, and instead show what we have collected?\n\n>  \tif (!maxcount || array.nr < maxcount)\n> @@ -91,11 +93,12 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n>  \t\tputchar('\\n');\n>  \t}\n>  \n> +cleanup:\n>  \tstrbuf_release(&err);\n>  \tstrbuf_release(&output);\n>  \tref_array_clear(&array);\n>  \tfree_commit_list(filter.with_commit);\n>  \tfree_commit_list(filter.no_commit);\n>  \tref_sorting_release(sorting);\n> -\treturn 0;\n> +\treturn ret;\n>  }\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 7838bd22b8..f9667f6ca4 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2249,7 +2249,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n>  \n>  \tif (flag & REF_ISBROKEN) {\n>  \t\twarning(_(\"ignoring broken ref %s\"), refname);\n> -\t\treturn 0;\n> +\t\treturn 1;\n>  \t}\n\nAh, no, not really.  This causes us to stop iterating prematurely.\n\nIf we are iterating because we want to find any breakage, such an\nearly stop in iteration makes good sense, but most of the time, we\nare not, and it is questionable if such an early return makes much\nsense.\n\nI suspect that a handler may need to keep going, while recording a\nbit for each ref it collects.  ref_array_item may or may not have (I\ndo not know offhand) already a bit in its flag word to signal a broken\nref that we can carry this information out to the callers?\n"}]}