{"thread":{"id":"56585","subject":"[PATCH 0/16] enabling GIT_REF_PARANOIA by default","startedAt":"2021-09-24T18:30:19Z","lastAt":"2021-09-27T19:49:49Z","messageCount":22,"participants":["Jeff King","Junio C Hamano","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":16},"messages":[{"id":"436998","messageId":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","threadId":"56585","inReplyTo":null,"subject":"[PATCH 0/16] enabling GIT_REF_PARANOIA by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:30:16Z","receivedAt":"2021-09-24T18:30:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I recently ran into a situation where dealing with a corrupted\nrepository was more confusing than necessary, because Git by default\nignores corrupted refs in many commands.\n\nA while ago we introduced GIT_REF_PARANOIA, which works by including\nbroken refs in iteration, which then typically causes later operations\nto fail (e.g., during repacking, you'd prefer to barf loudly when trying\nto access the missing object rather than incorrectly assume the objects\nfrom the broken ref aren't reachable).\n\nI think this is a better default for Git to have in general, not just\nfor a few select operations (we turn it on by default for pruning and\nsome repacks). We shouldn't see corruptions in general, and complaining\nloudly when we do is the safest option. The reason we held back when the\nknob was introduced was mostly out of deference to the historical\nbehavior.\n\nSo this series started as a patch to just flip that default, but I found\nsome interesting things:\n\n - there are a couple of tests that get confused. IMHO this is\n   vindicating the idea of flipping the default, beacuse in each case\n   these tests were poorly written (either corruptions they didn't\n   realize they had, or doing questionable operations on an incomplete\n   set of refs)\n\n - the existing GIT_REF_PARANOIA is over-eager to complain about\n   dangling symrefs, even though they're perfectly fine\n\n - as usual, there was some obvious cleanup along the way. ;)\n\nEven if you don't buy the argument that we should flip the default, I\nthink everything up through patch 11 is a worthwhile cleanup on its own.\n\nNote that this conflicts with jt/no-abuse-alternate-odb-for-submodules,\nsince it is touching the innards of DO_FOR_EACH_REF_INCLUDE_BROKEN, too.\nI left a note on that series about how I think that could be reconciled\n(i.e., the conflict is just around how the code is written, and not\ninherent to the goals).\n\nIn the end I left GIT_REF_PARANOIA as a knob, just defaulting to \"1\". I\nthink it's possibly useful as an escape hatch when dealing with a\ncorrupt repo. But we _could_ go all the way and basically drop\nDO_FOR_EACH_REF_INCLUDE_BROKEN's do-we-have-the-object check entirely.\nThat would totally sever the relationship between the ref store and the\nobject store, which would make things conceptually a lot simpler (and I\nsaw was discussed in some of those earlier threads).\n\nJust a breakdown of the series:\n\n  [01/16]: t7900: clean up some more broken refs\n  [02/16]: t5516: don't use HEAD ref for invalid ref-deletion tests\n  [03/16]: t5600: provide detached HEAD for corruption failures\n  [04/16]: t5312: drop \"verbose\" helper\n  [05/16]: t5312: create bogus ref as necessary\n  [06/16]: t5312: test non-destructive repack\n  [07/16]: t5312: be more assertive about command failure\n\n     Test cleanups. Necessary for the default flip, but I think each\n     stands on its own.\n\n  [08/16]: refs-internal.h: move DO_FOR_EACH_* flags next to each other\n  [09/16]: refs-internal.h: reorganize DO_FOR_EACH_* flag documentation\n\n     Cleanup of existing features.\n\n  [10/16]: refs: add DO_FOR_EACH_OMIT_DANGLING_SYMREFS flag\n  [11/16]: refs: omit dangling symrefs when using GIT_REF_PARANOIA\n\n     Fixing the current over-eager behavior of GIT_REF_PARANOIA.\n\n  [12/16]: refs: turn on GIT_REF_PARANOIA by default\n\n     The actual flip.\n\n  [13/16]: repack, prune: drop GIT_REF_PARANOIA settings\n  [14/16]: ref-filter: stop setting FILTER_REFS_INCLUDE_BROKEN\n  [15/16]: ref-filter: drop broken-ref code entirely\n  [16/16]: refs: drop \"broken\" flag from for_each_fullref_in()\n\n     Some small cleanups we can do as a result.\n\n Documentation/git.txt         | 19 ++++++------\n builtin/branch.c              |  2 +-\n builtin/for-each-ref.c        |  2 +-\n builtin/prune.c               |  1 -\n builtin/repack.c              |  3 --\n builtin/rev-parse.c           |  4 +--\n cache.h                       |  8 -----\n environment.c                 |  1 -\n ls-refs.c                     |  2 +-\n ref-filter.c                  | 22 ++++++--------\n ref-filter.h                  |  1 -\n refs.c                        | 42 +++++++++++++-------------\n refs.h                        |  9 ++----\n refs/files-backend.c          |  5 ++++\n refs/refs-internal.h          | 56 ++++++++++++++++++++++-------------\n revision.c                    |  2 +-\n t/t1430-bad-ref-name.sh       |  2 +-\n t/t5312-prune-corruption.sh   | 48 ++++++++++++++++++++++--------\n t/t5516-fetch-push.sh         | 19 ++++++------\n t/t5600-clone-fail-cleanup.sh |  4 ++-\n t/t7900-maintenance.sh        |  6 +++-\n 21 files changed, 142 insertions(+), 116 deletions(-)\n\n-Peff\n"},{"id":"436999","messageId":"YU4Zse+dAeDg8RqV@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 01/16] t7900: clean up some more broken refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:32:17Z","receivedAt":"2021-09-24T18:32:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"incremental-repack task\" test replaces the object directory with a\nknown state. As a result, some of our refs point to objects that are not\nincluded in that state.\n\nCommit 3cf5f221be (t7900: clean up some broken refs, 2021-01-19) cleaned\nup some of those (that were causing warnings to stderr from the\nmaintenance process). But there are a few more that were missed. These\naren't hurting anything for now, but it's certainly an unexpected state\nto leave the test repository in, and it will become a problem if repack\never gets more picky about broken refs.\n\nLet's clean up those additional refs (which are all in refs/remotes,\nwith nothing there that isn't broken), and add an extra \"for-each-ref\"\ncall to assert that we've got everything.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t7900-maintenance.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 36a4218745..31245f6276 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -277,7 +277,7 @@ test_expect_success 'incremental-repack task' '\n \n \t# Delete refs that have not been repacked in these packs.\n \tgit for-each-ref --format=\"delete %(refname)\" \\\n-\t\trefs/prefetch refs/tags >refs &&\n+\t\trefs/prefetch refs/tags refs/remotes >refs &&\n \tgit update-ref --stdin <refs &&\n \n \t# Replace the object directory with this pack layout.\n@@ -286,6 +286,10 @@ test_expect_success 'incremental-repack task' '\n \tls $packDir/*.pack >packs-before &&\n \ttest_line_count = 3 packs-before &&\n \n+\t# make sure we do not have any broken refs that were\n+\t# missed in the deletion above\n+\tgit for-each-ref &&\n+\n \t# the job repacks the two into a new pack, but does not\n \t# delete the old ones.\n \tgit maintenance run --task=incremental-repack &&\n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437000","messageId":"YU4Z3As2RoBeUbJa@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 02/16] t5516: don't use HEAD ref for invalid ref-deletion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:33:00Z","receivedAt":"2021-09-24T18:33:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A few tests in t5516 want to assert that we can delete a corrupted ref\nwhose pointed-to object is missing. They do so by using the \"main\"\nbranch, which is also pointed to by HEAD.\n\nThis does work, but only because of a subtle assumption about the\nimplementation. We do not block the deletion because of the invalid ref,\nbut we _also_ do not notice that the deleted branch is pointed to by\nHEAD. And so the safety rule of \"do not allow HEAD to be deleted in a\nnon-bare repository\" does not kick in, and the test passes.\n\nLet's instead use a non-HEAD branch. That still tests what we care about\nhere (deleting a corrupt ref), but without implicitly depending on our\nfailure to notice that we're deleting HEAD. That will future proof the\ntest against that behavior changing.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5516-fetch-push.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 4db8edd9c8..b13553ecf4 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -662,10 +662,10 @@ test_expect_success 'push does not update local refs on failure' '\n \n test_expect_success 'allow deleting an invalid remote ref' '\n \n-\tmk_test testrepo heads/main &&\n+\tmk_test testrepo heads/branch &&\n \trm -f testrepo/.git/objects/??/* &&\n-\tgit push testrepo :refs/heads/main &&\n-\t(cd testrepo && test_must_fail git rev-parse --verify refs/heads/main)\n+\tgit push testrepo :refs/heads/branch &&\n+\t(cd testrepo && test_must_fail git rev-parse --verify refs/heads/branch)\n \n '\n \n@@ -706,25 +706,25 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho\n '\n \n test_expect_success 'deleting dangling ref triggers hooks with correct args' '\n-\tmk_test_with_hooks testrepo heads/main &&\n+\tmk_test_with_hooks testrepo heads/branch &&\n \trm -f testrepo/.git/objects/??/* &&\n-\tgit push testrepo :refs/heads/main &&\n+\tgit push testrepo :refs/heads/branch &&\n \t(\n \t\tcd testrepo/.git &&\n \t\tcat >pre-receive.expect <<-EOF &&\n-\t\t$ZERO_OID $ZERO_OID refs/heads/main\n+\t\t$ZERO_OID $ZERO_OID refs/heads/branch\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $ZERO_OID $ZERO_OID\n+\t\trefs/heads/branch $ZERO_OID $ZERO_OID\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n-\t\t$ZERO_OID $ZERO_OID refs/heads/main\n+\t\t$ZERO_OID $ZERO_OID refs/heads/branch\n \t\tEOF\n \n \t\tcat >post-update.expect <<-EOF &&\n-\t\trefs/heads/main\n+\t\trefs/heads/branch\n \t\tEOF\n \n \t\ttest_cmp pre-receive.expect pre-receive.actual &&\n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437001","messageId":"YU4aHBX/cMht6rRX@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 03/16] t5600: provide detached HEAD for corruption failures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:34:04Z","receivedAt":"2021-09-24T18:34:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When checking how git-clone behaves when it fails, we stimulate some\nfailures by trying to do a clone from a local repository whose objects\nhave been removed. Because these clones use local optimizations, there's\na subtle dependency in how the corruption is handled on the sending\nside.\n\nIf upload-pack does not show us the broken refs (which it does not\ncurrently), then we see only HEAD (which is itself broken), and clone\nthat as a detached HEAD. When we try to write the ref, we notice that we\nnever got the object and bail.\n\nBut if upload-pack _does_ show us the broken refs (which it may in a\nfuture patch), then we'll realize that HEAD is a symref and just write\nthat. You'd think we'd fail when writing out the refs themselves, but we\ndon't; we do a bulk write and skip the connectivity check because of our\n--local optimizations. For the non-bare case, we do notice the problem\nwhen we try to checkout. But for a bare repository, we unexpectedly\ncomplete the clone successfully!\n\nAt first glance this may seem like a bug. But the whole point of those\nlocal optimizations is to give up some safety for speed. If you want to\nbe careful, you should be using \"--no-local\", which would notice that\nthe pack did not transfer sufficient objects. We could do that in these\ntests, but part of the point is for them to fail at specific moments\n(and indeed, we have a later test that checks for transport failure).\n\nHowever, we can make this less subtle and future-proof it against\nchanges on the upload-pack side by just having an explicit detached\nHEAD in the corrupted repo. Now we'll fail as expected during the ref\nwrite if any ref _or_ HEAD is corrupt, whether we're --bare or not.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5600-clone-fail-cleanup.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh\nindex 5bf10261d3..34b3df4027 100755\n--- a/t/t5600-clone-fail-cleanup.sh\n+++ b/t/t5600-clone-fail-cleanup.sh\n@@ -35,7 +35,9 @@ test_expect_success 'create a repo to clone' '\n '\n \n test_expect_success 'create objects in repo for later corruption' '\n-\ttest_commit -C foo file\n+\ttest_commit -C foo file &&\n+\tgit -C foo checkout --detach &&\n+\ttest_commit -C foo detached\n '\n \n # source repository given to git clone should be relative to the\n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437002","messageId":"YU4acRiq8YrhEOYu@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 04/16] t5312: drop \"verbose\" helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:35:29Z","receivedAt":"2021-09-24T18:35:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"t5312 has several uses of the \"verbose\" helper, as described in\n8ad1652418 (t5304: use helper to report failure of \"test foo = bar\",\n2014-10-10). Back then the \"-x\" trace option for tests was new, and was\nnot as pleasant to use (e.g., some tests failed under \"-x\", we did not\nsupport BASH_XTRACEFD, etc).\n\nThese days it is clear that \"-x\" is the preferred way to get extra\noutput, and we don't need to mark up individual tests. Let's get rid of\nthe uses of \"verbose\" here, as one step toward eradicating it totally.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI've been tempted to do this tree-wide for a while, and I don't mind\ndoing that separately. But as I was touching these tests, it seemed like\na good time to do it here.\n\n t/t5312-prune-corruption.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\nindex 11423b3cb2..165cc80fa4 100755\n--- a/t/t5312-prune-corruption.sh\n+++ b/t/t5312-prune-corruption.sh\n@@ -31,21 +31,21 @@ test_expect_success 'create history reachable only from a bogus-named ref' '\n test_expect_success 'pruning does not drop bogus object' '\n \ttest_when_finished \"git hash-object -w -t commit saved\" &&\n \ttest_might_fail git prune --expire=now &&\n-\tverbose git cat-file -e $bogus\n+\tgit cat-file -e $bogus\n '\n \n test_expect_success 'put bogus object into pack' '\n \tgit tag reachable $bogus &&\n \tgit repack -ad &&\n \tgit tag -d reachable &&\n-\tverbose git cat-file -e $bogus\n+\tgit cat-file -e $bogus\n '\n \n test_expect_success 'destructive repack keeps packed object' '\n \ttest_might_fail git repack -Ad --unpack-unreachable=now &&\n-\tverbose git cat-file -e $bogus &&\n+\tgit cat-file -e $bogus &&\n \ttest_might_fail git repack -ad &&\n-\tverbose git cat-file -e $bogus\n+\tgit cat-file -e $bogus\n '\n \n # subsequent tests will have different corruptions\n@@ -78,7 +78,7 @@ test_expect_success 'create history with missing tip commit' '\n test_expect_success 'pruning with a corrupted tip does not drop history' '\n \ttest_when_finished \"git hash-object -w -t commit saved\" &&\n \ttest_might_fail git prune --expire=now &&\n-\tverbose git cat-file -e $recoverable\n+\tgit cat-file -e $recoverable\n '\n \n test_expect_success 'pack-refs does not silently delete broken loose ref' '\n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437003","messageId":"YU4aoicz2qXWCle5@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 05/16] t5312: create bogus ref as necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:36:18Z","receivedAt":"2021-09-24T18:36:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some tests in t5312 create an illegally-named ref, and then see how\nvarious operations handle it. But between those operations, we also do\nsome more setup (e.g., repacking), and we are subtly depending on how\nthose setup steps react to the illegal ref.\n\nTo future-proof us against those behaviors changing, let's instead\ncreate and clean up our bogus ref on demand in the tests that need it.\n\nThis has two small extra advantages:\n\n - the tests are more stand-alone; we do not need an extra test to clean\n   up the ref before moving on to other parts of the script\n\n - the creation and cleanup is together in one helper function. Because\n   these depend on touching the refs in the filesystem directly, they\n   may need to be tweaked for a world with alternate backends (they have\n   not been noticed so far in the reftable work because with a non-file\n   backend the tests don't fail; they simply become uninteresting noops\n   because the broken ref isn't read at all).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5312-prune-corruption.sh | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\nindex 165cc80fa4..0704f3e930 100755\n--- a/t/t5312-prune-corruption.sh\n+++ b/t/t5312-prune-corruption.sh\n@@ -18,18 +18,23 @@ test_expect_success 'disable reflogs' '\n \tgit reflog expire --expire=all --all\n '\n \n+create_bogus_ref () {\n+\ttest_when_finished 'rm -f .git/refs/heads/bogus..name' &&\n+\techo $bogus >.git/refs/heads/bogus..name\n+}\n+\n test_expect_success 'create history reachable only from a bogus-named ref' '\n \ttest_tick && git commit --allow-empty -m main &&\n \tbase=$(git rev-parse HEAD) &&\n \ttest_tick && git commit --allow-empty -m bogus &&\n \tbogus=$(git rev-parse HEAD) &&\n \tgit cat-file commit $bogus >saved &&\n-\techo $bogus >.git/refs/heads/bogus..name &&\n \tgit reset --hard HEAD^\n '\n \n test_expect_success 'pruning does not drop bogus object' '\n \ttest_when_finished \"git hash-object -w -t commit saved\" &&\n+\tcreate_bogus_ref &&\n \ttest_might_fail git prune --expire=now &&\n \tgit cat-file -e $bogus\n '\n@@ -42,17 +47,13 @@ test_expect_success 'put bogus object into pack' '\n '\n \n test_expect_success 'destructive repack keeps packed object' '\n+\tcreate_bogus_ref &&\n \ttest_might_fail git repack -Ad --unpack-unreachable=now &&\n \tgit cat-file -e $bogus &&\n \ttest_might_fail git repack -ad &&\n \tgit cat-file -e $bogus\n '\n \n-# subsequent tests will have different corruptions\n-test_expect_success 'clean up bogus ref' '\n-\trm .git/refs/heads/bogus..name\n-'\n-\n # We create two new objects here, \"one\" and \"two\". Our\n # main branch points to \"two\", which is deleted,\n # corrupting the repository. But we'd like to make sure\n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437004","messageId":"YU4aukHUjq3+24zn@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 06/16] t5312: test non-destructive repack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:36:42Z","receivedAt":"2021-09-24T18:36:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In t5312, we create a state with a broken ref, and then make sure that\ndestructive repacks don't silently ignore the breakage (where a\ndestructive repack is one that might drop objects). But we don't check\nthe behavior of non-destructive repacks at all (i.e., ones where we'd\nkeep unreachable objects).\n\nSo let's add a test to confirm the current behavior, which is that\nthey are allowed (i.e., ignoring the breakage and considering any\nobjects it points to as unreachable). This may change in the future, but\nwe'd like for the test suite to alert us to that fact.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5312-prune-corruption.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\nindex 0704f3e930..c7010fb754 100755\n--- a/t/t5312-prune-corruption.sh\n+++ b/t/t5312-prune-corruption.sh\n@@ -46,6 +46,11 @@ test_expect_success 'put bogus object into pack' '\n \tgit cat-file -e $bogus\n '\n \n+test_expect_success 'non-destructive repack ignores bogus name' '\n+\tcreate_bogus_ref &&\n+\tgit repack -adk\n+'\n+\n test_expect_success 'destructive repack keeps packed object' '\n \tcreate_bogus_ref &&\n \ttest_might_fail git repack -Ad --unpack-unreachable=now &&\n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437006","messageId":"YU4a19JXV67wsV+o@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 07/16] t5312: be more assertive about command failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:37:11Z","receivedAt":"2021-09-24T18:37:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When repacking or pruning in a corrupted repository, our tests in t5312\nargue that it is OK to complete the operation or bail, as long as we\ndon't actually delete the objects pointed to by the corruption.\n\nThis isn't a wrong line of reasoning, but the tests are a bit permissive\nby using test_might_fail. The fact is that we _do_ bail currently, and\nif we ever stopped doing so, that would be worthy of a human\ninvestigating. So let's switch these to test_must_fail.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5312-prune-corruption.sh | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\nindex c7010fb754..1522a4ba8e 100755\n--- a/t/t5312-prune-corruption.sh\n+++ b/t/t5312-prune-corruption.sh\n@@ -7,6 +7,9 @@ if we see, for example, a ref with a bogus name, it is OK either to\n bail out or to proceed using it as a reachable tip, but it is _not_\n OK to proceed as if it did not exist. Otherwise we might silently\n delete objects that cannot be recovered.\n+\n+Note that we do assert command failure in these cases, because that is\n+what currently happens. If that changes, these tests should be revisited.\n '\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n@@ -35,7 +38,7 @@ test_expect_success 'create history reachable only from a bogus-named ref' '\n test_expect_success 'pruning does not drop bogus object' '\n \ttest_when_finished \"git hash-object -w -t commit saved\" &&\n \tcreate_bogus_ref &&\n-\ttest_might_fail git prune --expire=now &&\n+\ttest_must_fail git prune --expire=now &&\n \tgit cat-file -e $bogus\n '\n \n@@ -53,9 +56,9 @@ test_expect_success 'non-destructive repack ignores bogus name' '\n \n test_expect_success 'destructive repack keeps packed object' '\n \tcreate_bogus_ref &&\n-\ttest_might_fail git repack -Ad --unpack-unreachable=now &&\n+\ttest_must_fail git repack -Ad --unpack-unreachable=now &&\n \tgit cat-file -e $bogus &&\n-\ttest_might_fail git repack -ad &&\n+\ttest_must_fail git repack -ad &&\n \tgit cat-file -e $bogus\n '\n \n@@ -83,7 +86,7 @@ test_expect_success 'create history with missing tip commit' '\n \n test_expect_success 'pruning with a corrupted tip does not drop history' '\n \ttest_when_finished \"git hash-object -w -t commit saved\" &&\n-\ttest_might_fail git prune --expire=now &&\n+\ttest_must_fail git prune --expire=now &&\n \tgit cat-file -e $recoverable\n '\n \n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437007","messageId":"YU4bBmkszJMyiqta@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 08/16] refs-internal.h: move DO_FOR_EACH_* flags next to each other","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:37:58Z","receivedAt":"2021-09-24T18:38:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are currently two DO_FOR_EACH_* flags, which must not have their\nbits overlap. Yet they're defined hundreds of lines apart. Let's move\nthem next to each other to make it clear that they are related and are a\ncomplete set (which matters if you are adding a new flag and would like\nto know what the next available bit is).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nYou can probably guess how I found this problem. :)\n\n refs/refs-internal.h | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 3155708345..7b30910974 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -248,6 +248,14 @@ int refs_rename_ref_available(struct ref_store *refs,\n /* Include broken references in a do_for_each_ref*() iteration: */\n #define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n \n+/*\n+ * Only include per-worktree refs in a do_for_each_ref*() iteration.\n+ * Normally this will be used with a files ref_store, since that's\n+ * where all reference backends will presumably store their\n+ * per-worktree refs.\n+ */\n+#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x02\n+\n /*\n  * Reference iterators\n  *\n@@ -498,14 +506,6 @@ int do_for_each_repo_ref_iterator(struct repository *r,\n \t\t\t\t  struct ref_iterator *iter,\n \t\t\t\t  each_repo_ref_fn fn, void *cb_data);\n \n-/*\n- * Only include per-worktree refs in a do_for_each_ref*() iteration.\n- * Normally this will be used with a files ref_store, since that's\n- * where all reference backends will presumably store their\n- * per-worktree refs.\n- */\n-#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x02\n-\n struct ref_store;\n \n /* refs backends */\n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437008","messageId":"YU4bcPAsVa61t8Ze@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 09/16] refs-internal.h: reorganize DO_FOR_EACH_* flag documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:39:44Z","receivedAt":"2021-09-24T18:39:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The documentation for the DO_FOR_EACH_* flags is sprinkled over the\nrefs-internal.h file. We define the two flags in one spot, and then\ndescribe them in more detail far away from there, in the definitions of\nrefs_ref_iterator_begin() and ref_iterator_advance_fn().\n\nLet's try to organize this a bit better:\n\n  - convert the #defines to an enum. This makes it clear that they are\n    related, and that the enum shows the complete set of flags.\n\n  - combine all descriptions for each flag in a single spot, next to the\n    flag's definition\n\n  - use the enum rather than a bare int for functions which take the\n    flags. This helps readers realize which flags can be used.\n\n  - clarify the mention of flags for ref_iterator_advance_fn(). It does\n    not take flags itself, but is meant to depend on ones set up\n    earlier.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c               | 10 ++++++----\n refs/refs-internal.h | 46 ++++++++++++++++++++++++++------------------\n 2 files changed, 33 insertions(+), 23 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8b9f7c3a80..c28bd6a818 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1413,7 +1413,8 @@ int head_ref(each_ref_fn fn, void *cb_data)\n \n struct ref_iterator *refs_ref_iterator_begin(\n \t\tstruct ref_store *refs,\n-\t\tconst char *prefix, int trim, int flags)\n+\t\tconst char *prefix, int trim,\n+\t\tenum do_for_each_ref_flags flags)\n {\n \tstruct ref_iterator *iter;\n \n@@ -1479,7 +1480,8 @@ static int do_for_each_ref_helper(struct repository *r,\n }\n \n static int do_for_each_ref(struct ref_store *refs, const char *prefix,\n-\t\t\t   each_ref_fn fn, int trim, int flags, void *cb_data)\n+\t\t\t   each_ref_fn fn, int trim,\n+\t\t\t   enum do_for_each_ref_flags flags, void *cb_data)\n {\n \tstruct ref_iterator *iter;\n \tstruct do_for_each_ref_help hp = { fn, cb_data };\n@@ -1516,7 +1518,7 @@ int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)\n \n int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken)\n {\n-\tunsigned int flag = 0;\n+\tenum do_for_each_ref_flags flag = 0;\n \n \tif (broken)\n \t\tflag = DO_FOR_EACH_INCLUDE_BROKEN;\n@@ -1528,7 +1530,7 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,\n \t\t\t     each_ref_fn fn, void *cb_data,\n \t\t\t     unsigned int broken)\n {\n-\tunsigned int flag = 0;\n+\tenum do_for_each_ref_flags flag = 0;\n \n \tif (broken)\n \t\tflag = DO_FOR_EACH_INCLUDE_BROKEN;\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 7b30910974..2c4e1739f2 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -245,16 +245,30 @@ int refs_rename_ref_available(struct ref_store *refs,\n /* We allow \"recursive\" symbolic refs. Only within reason, though */\n #define SYMREF_MAXDEPTH 5\n \n-/* Include broken references in a do_for_each_ref*() iteration: */\n-#define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n-\n /*\n- * Only include per-worktree refs in a do_for_each_ref*() iteration.\n- * Normally this will be used with a files ref_store, since that's\n- * where all reference backends will presumably store their\n- * per-worktree refs.\n+ * These flags are passed to refs_ref_iterator_begin() (and do_for_each_ref(),\n+ * which feeds it).\n  */\n-#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x02\n+enum do_for_each_ref_flags {\n+\t/*\n+\t * Include broken references in a do_for_each_ref*() iteration, which\n+\t * would normally be omitted. This includes both refs that point to\n+\t * missing objects (a true repository corruption), ones with illegal\n+\t * names (which we prefer not to expose to callers), as well as\n+\t * dangling symbolic refs (i.e., those that point to a non-existent\n+\t * ref; this is not a corruption, but as they have no valid oid, we\n+\t * omit them from normal iteration results).\n+\t */\n+\tDO_FOR_EACH_INCLUDE_BROKEN = (1 << 0),\n+\n+\t/*\n+\t * Only include per-worktree refs in a do_for_each_ref*() iteration.\n+\t * Normally this will be used with a files ref_store, since that's\n+\t * where all reference backends will presumably store their\n+\t * per-worktree refs.\n+\t */\n+\tDO_FOR_EACH_PER_WORKTREE_ONLY = (1 << 1),\n+};\n \n /*\n  * Reference iterators\n@@ -357,16 +371,12 @@ int is_empty_ref_iterator(struct ref_iterator *ref_iterator);\n  * Return an iterator that goes over each reference in `refs` for\n  * which the refname begins with prefix. If trim is non-zero, then\n  * trim that many characters off the beginning of each refname.\n- * The output is ordered by refname. The following flags are supported:\n- *\n- * DO_FOR_EACH_INCLUDE_BROKEN: include broken references in\n- *         the iteration.\n- *\n- * DO_FOR_EACH_PER_WORKTREE_ONLY: only produce REF_TYPE_PER_WORKTREE refs.\n+ * The output is ordered by refname.\n  */\n struct ref_iterator *refs_ref_iterator_begin(\n \t\tstruct ref_store *refs,\n-\t\tconst char *prefix, int trim, int flags);\n+\t\tconst char *prefix, int trim,\n+\t\tenum do_for_each_ref_flags flags);\n \n /*\n  * A callback function used to instruct merge_ref_iterator how to\n@@ -454,10 +464,8 @@ void base_ref_iterator_free(struct ref_iterator *iter);\n /*\n  * backend-specific implementation of ref_iterator_advance. For symrefs, the\n  * function should set REF_ISSYMREF, and it should also dereference the symref\n- * to provide the OID referent. If DO_FOR_EACH_INCLUDE_BROKEN is set, symrefs\n- * with non-existent referents and refs pointing to non-existent object names\n- * should also be returned. If DO_FOR_EACH_PER_WORKTREE_ONLY, only\n- * REF_TYPE_PER_WORKTREE refs should be returned.\n+ * to provide the OID referent. It should respect do_for_each_ref_flags\n+ * that were passed to refs_ref_iterator_begin().\n  */\n typedef int ref_iterator_advance_fn(struct ref_iterator *ref_iterator);\n \n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437010","messageId":"YU4b3LxHdQiVGtWW@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 10/16] refs: add DO_FOR_EACH_OMIT_DANGLING_SYMREFS flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:41:32Z","receivedAt":"2021-09-24T18:41:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the DO_FOR_EACH_INCLUDE_BROKEN flag is used, we include both actual\ncorrupt refs (illegal names, missing objects), but also symrefs that\npoint to nothing. This latter is not really a corruption, but just\nsomething that may happen normally. For example, the symref at\nrefs/remotes/origin/HEAD may point to a tracking branch which is later\ndeleted. (The local HEAD may also be unborn, of course, but we do not\naccess it through ref iteration).\n\nMost callers of for_each_ref() etc, do not care. They don't pass\nINCLUDE_BROKEN, so don't see it at all. But for those which do pass it,\nthis somewhat-normal state causes extra warnings (e.g., from\nfor-each-ref) or even aborts operations (destructive repacks with\nGIT_REF_PARANOIA set).\n\nThis patch just introduces the flag and the mechanism; there are no\ncallers yet (and hence no tests). Two things to note on the\nimplementation:\n\n  - we actually skip any symref that does not resolve to a ref. This\n    includes ones which point to an invalidly-named ref. You could argue\n    this is a more serious breakage than simple dangling. But the\n    overall effect is the same (we could not follow the symref), as well\n    as the impact on things like REF_PARANOIA (either way, a symref we\n    can't follow won't impact reachability, because we'll see the ref\n    itself during iteration). The underlying resolution function doesn't\n    distinguish these two cases (they both get REF_ISBROKEN).\n\n  - we change the iterator in refs/files-backend.c where we check\n    INCLUDE_BROKEN. There's a matching spot in refs/packed-backend.c,\n    but we don't know need to do anything there. The packed backend does\n    not support symrefs at all.\n\nThe resulting set of flags might be a bit easier to follow if we broke\nthis down into \"INCLUDE_CORRUPT_REFS\" and \"INCLUDE_DANGLING_SYMREFS\".\nBut there are a few reasons not do so:\n\n  - adding a new OMIT_DANGLING_SYMREFS flag lets us leave existing\n    callers intact, without changing their behavior (and some of them\n    really do want to see the dangling symrefs; e.g., t5505 has a test\n    which expects us to report when a symref becomes dangling)\n\n  - they're not actually independent. You cannot say \"include dangling\n    symrefs\" without also including refs whose objects are not\n    reachable, because dangling symrefs by definition do not have an\n    object. We could tweak the implementation to distinguish this, but\n    in practice nobody wants to ask for that. Adding the OMIT flag keeps\n    the implementation simple and makes sure we don't regress the\n    current behavior.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs/files-backend.c | 5 +++++\n refs/refs-internal.h | 6 ++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 74c0385873..1148c0cf09 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -744,6 +744,11 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t    ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE)\n \t\t\tcontinue;\n \n+\t\tif ((iter->flags & DO_FOR_EACH_OMIT_DANGLING_SYMREFS) &&\n+\t\t    (iter->iter0->flags & REF_ISSYMREF) &&\n+\t\t    (iter->iter0->flags & REF_ISBROKEN))\n+\t\t\tcontinue;\n+\n \t\tif (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&\n \t\t    !ref_resolves_to_object(iter->iter0->refname,\n \t\t\t\t\t    iter->iter0->oid,\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 2c4e1739f2..96911fb26e 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -268,6 +268,12 @@ enum do_for_each_ref_flags {\n \t * per-worktree refs.\n \t */\n \tDO_FOR_EACH_PER_WORKTREE_ONLY = (1 << 1),\n+\n+\t/*\n+\t * Omit dangling symrefs from output; this only has an effect with\n+\t * INCLUDE_BROKEN, since they are otherwise not included at all.\n+\t */\n+\tDO_FOR_EACH_OMIT_DANGLING_SYMREFS = (1 << 2),\n };\n \n /*\n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437011","messageId":"YU4cHlGaEwBzTJ0k@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 11/16] refs: omit dangling symrefs when using GIT_REF_PARANOIA","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:42:38Z","receivedAt":"2021-09-24T18:42:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Dangling symrefs aren't actually a corruption problem. It's perfectly\nfine for refs/remotes/origin/HEAD to point to an unborn branch. And in\nparticular, if you are trying to establish reachability, a symref that\npoints nowhere doesn't matter either way. Any ref it could point to will\nbe examined during the rest of the traversal.\n\nIt's possible that a symref pointing nowhere _could_ be a sign that the\nref it was meant to point to was deleted accidentally (e.g., via\ncorruption). But there is no particular reason to think that is true for\nany given case, and in the meantime, GIT_REF_PARANOIA kicking in\nautomatically for some operations means they'll fail unnecessarily.\n\nSo let's loosen it just a bit. The new test in t5312 shows off an\nexample that is safe, but currently fails (and no longer does after this\npatch).\n\nNote that we don't do anything if the caller explicitly asked for\nDO_FOR_EACH_INCLUDE_BROKEN. In that case they may be looking for\ndangling symrefs themselves, and setting GIT_REF_PARANOIA should not\n_loosen_ things from what the caller asked for.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c                      | 12 ++++++++----\n t/t5312-prune-corruption.sh |  7 +++++++\n 2 files changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex c28bd6a818..e72510813e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1418,10 +1418,14 @@ struct ref_iterator *refs_ref_iterator_begin(\n {\n \tstruct ref_iterator *iter;\n \n-\tif (ref_paranoia < 0)\n-\t\tref_paranoia = git_env_bool(\"GIT_REF_PARANOIA\", 0);\n-\tif (ref_paranoia)\n-\t\tflags |= DO_FOR_EACH_INCLUDE_BROKEN;\n+\tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {\n+\t\tif (ref_paranoia < 0)\n+\t\t\tref_paranoia = git_env_bool(\"GIT_REF_PARANOIA\", 0);\n+\t\tif (ref_paranoia) {\n+\t\t\tflags |= DO_FOR_EACH_INCLUDE_BROKEN;\n+\t\t\tflags |= DO_FOR_EACH_OMIT_DANGLING_SYMREFS;\n+\t\t}\n+\t}\n \n \titer = refs->be->iterator_begin(refs, prefix, flags);\n \ndiff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\nindex 1522a4ba8e..d8ec5a7462 100755\n--- a/t/t5312-prune-corruption.sh\n+++ b/t/t5312-prune-corruption.sh\n@@ -62,6 +62,13 @@ test_expect_success 'destructive repack keeps packed object' '\n \tgit cat-file -e $bogus\n '\n \n+test_expect_success 'destructive repack not confused by dangling symref' '\n+\ttest_when_finished \"git symbolic-ref -d refs/heads/dangling\" &&\n+\tgit symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&\n+\tgit repack -ad &&\n+\ttest_must_fail git cat-file -e $bogus\n+'\n+\n # We create two new objects here, \"one\" and \"two\". Our\n # main branch points to \"two\", which is deleted,\n # corrupting the repository. But we'd like to make sure\n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437012","messageId":"YU4c9S6jypbS4YCK@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 12/16] refs: turn on GIT_REF_PARANOIA by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:46:13Z","receivedAt":"2021-09-24T18:46:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The original point of the GIT_REF_PARANOIA flag was to include broken\nrefs in iterations, so that possibly-destructive operations would not\nsilently ignore them (and would generally instead try to operate on the\noids and fail when the objects could not be accessed).\n\nWe already turned this on by default for some dangerous operations, like\n\"repack -ad\" (where missing a reachability tip would mean dropping the\nassociated history). But it was not on for general use, even though it\ncould easily result in the spreading of corruption (e.g., imagine\ncloning a repository which simply omits some of its refs because\ntheir objects are missing; the result quietly succeeds even though you\ndid not clone everything!).\n\nThis patch turns on GIT_REF_PARANOIA by default. So a clone as mentioned\nabove would actually fail (upload-pack tells us about the broken ref,\nand when we ask for the objects, pack-objects fails to deliver them).\nThis may be inconvenient when working with a corrupted repository, but:\n\n  - we are better off to err on the side of complaining about\n    corruption, and then provide mechanisms for explicitly loosening\n    safety.\n\n  - this is only one type of corruption anyway. If we are missing any\n    other objects in the history that _aren't_ ref tips, then we'd\n    behave similarly (happily show the ref, but then barf when we\n    started traversing).\n\nWe retain the GIT_REF_PARANOIA variable, but simply default it to \"1\"\ninstead of \"0\". That gives the user an escape hatch for loosening this\nwhen working with a corrupt repository. It won't work across a remote\nconnection to upload-pack (because we can't necessarily set environment\nvariables on the remote), but there the client has other options (e.g.,\nchoosing which refs to fetch).\n\nAs a bonus, this also makes ref iteration faster in general (because we\ndon't have to call has_object_file() for each ref), though probably not\nnoticeably so in the general case. In a repo with a million refs, it\nshaved a few hundred milliseconds off of upload-pack's advertisement;\nthat's noticeable, but most repos are not nearly that large.\n\nThe possible downside here is that any operation which iterates refs but\ndoesn't ever open their objects may now quietly claim to have X when the\nobject is corrupted (e.g., \"git rev-list new-branch --not --all\" will\ntreat a broken ref as uninteresting). But again, that's not really any\ndifferent than corruption below the ref level. We might have\nrefs/heads/old-branch as non-corrupt, but we are not actively checking\nthat we have the entire reachable history. Or the pointed-to object\ncould even be corrupted on-disk (but our \"do we have it\" check would\nstill succeed). In that sense, this is merely bringing ref-corruption in\nline with general object corruption.\n\nOne alternative implementation would be to actually check for broken\nrefs, and then _immediately die_ if we see any. That would cause the\n\"rev-list --not --all\" case above to abort immediately. But in many ways\nthat's the worst of all worlds:\n\n  - it still spends time looking up the objects an extra time\n\n  - it still doesn't catch corruption below the ref level\n\n  - it's even more inconvenient; with the current implementation of\n    GIT_REF_PARANOIA for something like upload-pack, we can make\n    the advertisement and let the client choose a non-broken piece of\n    history. If we bail as soon as we see a broken ref, they cannot even\n    see the advertisement.\n\nThe test changes here show some of the fallout. A non-destructive \"git\nrepack -adk\" now fails by default (but we can override it). Deleting a\nbroken ref now actually tells the hooks the correct \"before\" state,\nrather than a confusing null oid.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git.txt       | 19 ++++++++++---------\n refs.c                      |  2 +-\n t/t5312-prune-corruption.sh | 10 ++++++++--\n t/t5516-fetch-push.sh       |  7 ++++---\n 4 files changed, 23 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex abace9eac2..d63c65e67d 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -867,15 +867,16 @@ for full details.\n \tend user, to be recorded in the body of the reflog.\n \n `GIT_REF_PARANOIA`::\n-\tIf set to `1`, include broken or badly named refs when iterating\n-\tover lists of refs. In a normal, non-corrupted repository, this\n-\tdoes nothing. However, enabling it may help git to detect and\n-\tabort some operations in the presence of broken refs. Git sets\n-\tthis variable automatically when performing destructive\n-\toperations like linkgit:git-prune[1]. You should not need to set\n-\tit yourself unless you want to be paranoid about making sure\n-\tan operation has touched every ref (e.g., because you are\n-\tcloning a repository to make a backup).\n+\tIf set to `0`, ignore broken or badly named refs when iterating\n+\tover lists of refs. Normally Git will try to include any such\n+\trefs, which may cause some operations to fail. This is usually\n+\tpreferable, as potentially destructive operations (e.g.,\n+\tlinkgit:git-prune[1]) are better off aborting rather than\n+\tignoring broken refs (and thus considering the history they\n+\tpoint to as not worth saving). The default value is `1` (i.e.,\n+\tbe paranoid about detecting and aborting all operations). You\n+\tshould not normally need to set this to `0`, but it may be\n+\tuseful when trying to salvage data from a corrupted repository.\n \n `GIT_ALLOW_PROTOCOL`::\n \tIf set to a colon-separated list of protocols, behave as if\ndiff --git a/refs.c b/refs.c\nindex e72510813e..ac19c689fa 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1420,7 +1420,7 @@ struct ref_iterator *refs_ref_iterator_begin(\n \n \tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {\n \t\tif (ref_paranoia < 0)\n-\t\t\tref_paranoia = git_env_bool(\"GIT_REF_PARANOIA\", 0);\n+\t\t\tref_paranoia = git_env_bool(\"GIT_REF_PARANOIA\", 1);\n \t\tif (ref_paranoia) {\n \t\t\tflags |= DO_FOR_EACH_INCLUDE_BROKEN;\n \t\t\tflags |= DO_FOR_EACH_OMIT_DANGLING_SYMREFS;\ndiff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\nindex d8ec5a7462..ea889c088a 100755\n--- a/t/t5312-prune-corruption.sh\n+++ b/t/t5312-prune-corruption.sh\n@@ -49,11 +49,17 @@ test_expect_success 'put bogus object into pack' '\n \tgit cat-file -e $bogus\n '\n \n-test_expect_success 'non-destructive repack ignores bogus name' '\n+test_expect_success 'non-destructive repack bails on bogus ref' '\n \tcreate_bogus_ref &&\n-\tgit repack -adk\n+\ttest_must_fail git repack -adk\n '\n \n+test_expect_success 'GIT_REF_PARANOIA=0 overrides safety' '\n+\tcreate_bogus_ref &&\n+\tGIT_REF_PARANOIA=0 git repack -adk\n+'\n+\n+\n test_expect_success 'destructive repack keeps packed object' '\n \tcreate_bogus_ref &&\n \ttest_must_fail git repack -Ad --unpack-unreachable=now &&\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex b13553ecf4..8212ca56dc 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -707,20 +707,21 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho\n \n test_expect_success 'deleting dangling ref triggers hooks with correct args' '\n \tmk_test_with_hooks testrepo heads/branch &&\n+\torig=$(git -C testrepo rev-parse refs/heads/branch) &&\n \trm -f testrepo/.git/objects/??/* &&\n \tgit push testrepo :refs/heads/branch &&\n \t(\n \t\tcd testrepo/.git &&\n \t\tcat >pre-receive.expect <<-EOF &&\n-\t\t$ZERO_OID $ZERO_OID refs/heads/branch\n+\t\t$orig $ZERO_OID refs/heads/branch\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/branch $ZERO_OID $ZERO_OID\n+\t\trefs/heads/branch $orig $ZERO_OID\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n-\t\t$ZERO_OID $ZERO_OID refs/heads/branch\n+\t\t$orig $ZERO_OID refs/heads/branch\n \t\tEOF\n \n \t\tcat >post-update.expect <<-EOF &&\n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437013","messageId":"YU4dDY5Pf1gBxI9G@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 13/16] repack, prune: drop GIT_REF_PARANOIA settings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:46:37Z","receivedAt":"2021-09-24T18:46:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that GIT_REF_PARANOIA is the default, we don't need to selectively\nenable it for destructive operations. In fact, it's harmful to do so,\nbecause it overrides any GIT_REF_PARANOIA=0 setting that the user may\nhave provided (because they're trying to work around some corruption).\n\nWith these uses gone, we can further clean up the ref_paranoia global,\nand make it a static variable inside the refs code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/prune.c  | 1 -\n builtin/repack.c | 3 ---\n cache.h          | 8 --------\n environment.c    | 1 -\n refs.c           | 2 ++\n 5 files changed, 2 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 02c6ab7cba..485c9a3c56 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -143,7 +143,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \texpire = TIME_MAX;\n \tsave_commit_buffer = 0;\n \tread_replace_refs = 0;\n-\tref_paranoia = 1;\n \trepo_init_revisions(the_repository, &revs, prefix);\n \n \targc = parse_options(argc, argv, prefix, options, prune_usage, 0);\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex c1a209013b..cb9f4bfed3 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -586,15 +586,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tstrvec_pushf(&cmd.args,\n \t\t\t\t\t     \"--unpack-unreachable=%s\",\n \t\t\t\t\t     unpack_unreachable);\n-\t\t\t\tstrvec_push(&cmd.env_array, \"GIT_REF_PARANOIA=1\");\n \t\t\t} else if (pack_everything & LOOSEN_UNREACHABLE) {\n \t\t\t\tstrvec_push(&cmd.args,\n \t\t\t\t\t    \"--unpack-unreachable\");\n \t\t\t} else if (keep_unreachable) {\n \t\t\t\tstrvec_push(&cmd.args, \"--keep-unreachable\");\n \t\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n-\t\t\t} else {\n-\t\t\t\tstrvec_push(&cmd.env_array, \"GIT_REF_PARANOIA=1\");\n \t\t\t}\n \t\t}\n \t} else if (geometry) {\ndiff --git a/cache.h b/cache.h\nindex f6295f3b04..f445008895 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -994,14 +994,6 @@ extern const char *core_fsmonitor;\n extern int core_apply_sparse_checkout;\n extern int core_sparse_checkout_cone;\n \n-/*\n- * Include broken refs in all ref iterations, which will\n- * generally choke dangerous operations rather than letting\n- * them silently proceed without taking the broken ref into\n- * account.\n- */\n-extern int ref_paranoia;\n-\n /*\n  * Returns the boolean value of $GIT_OPTIONAL_LOCKS (or the default value).\n  */\ndiff --git a/environment.c b/environment.c\nindex b4ba4fa22d..7923ab21dd 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -31,7 +31,6 @@ int prefer_symlink_refs;\n int is_bare_repository_cfg = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n-int ref_paranoia = -1;\n int repository_format_precious_objects;\n int repository_format_worktree_config;\n const char *git_commit_encoding;\ndiff --git a/refs.c b/refs.c\nindex ac19c689fa..d0f4e8726b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1419,6 +1419,8 @@ struct ref_iterator *refs_ref_iterator_begin(\n \tstruct ref_iterator *iter;\n \n \tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {\n+\t\tstatic int ref_paranoia = -1;\n+\n \t\tif (ref_paranoia < 0)\n \t\t\tref_paranoia = git_env_bool(\"GIT_REF_PARANOIA\", 1);\n \t\tif (ref_paranoia) {\n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437014","messageId":"YU4dZZ7P34/kSduu@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 14/16] ref-filter: stop setting FILTER_REFS_INCLUDE_BROKEN","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:48:05Z","receivedAt":"2021-09-24T18:48:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Of the ref-filter callers, for-each-ref and git-branch both set the\nINCLUDE_BROKEN flag (but git-tag does not, which is a weird\ninconsistency).  But now that GIT_REF_PARANOIA is on by default, that\nproduces almost the same outcome for all three.\n\nThe one exception is that GIT_REF_PARANOIA will omit dangling symrefs.\nThat's a better behavior for these tools, as they would never include\nsuch a symref in the main output anyway (they can't, as it doesn't point\nto an object). Instead they issue a warning to stderr. But that warning\nis somewhat useless; a dangling symref is a perfectly reasonable thing\nto have in your repository, and is not a sign of corruption. It's much\nfriendlier to just quietly ignore it.\n\nAnd in terms of robustness, the warning gains us little. It does not\nimpact the exit code of either tool. So while the warning _might_ clue\nin a user that they have an unexpected broken symref, it would not help\nany kind of scripted use.\n\nThis patch converts for-each-ref and git-branch to stop using the\nINCLUDE_BROKEN flag. That gives them more reasonable behavior, and\nharmonizes them with git-tag.\n\nWe have to change one test to adapt to the situation. t1430 tries to\ntrigger all of the REF_ISBROKEN behaviors from the underlying ref code.\nIt uses for-each-ref to do so (because there isn't any other mechanism).\nThat will no longer issue a warning about the symref which points to an\ninvalid name, as it's considered dangling (and we can instead be sure\nthat it's _not_ mentioned on stderr). Note that we do still complain\nabout the illegally named \"broken..symref\"; its problem is not that it's\ndangling, but the name of the symref itself is illegal.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/branch.c        | 2 +-\n builtin/for-each-ref.c  | 2 +-\n t/t1430-bad-ref-name.sh | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 03c7b7253a..0b7ed82654 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -427,7 +427,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \n \tmemset(&array, 0, sizeof(array));\n \n-\tfilter_refs(&array, filter, filter->kind | FILTER_REFS_INCLUDE_BROKEN);\n+\tfilter_refs(&array, filter, filter->kind);\n \n \tif (filter->verbose)\n \t\tmaxwidth = calc_maxwidth(&array, strlen(remote_prefix));\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 89cb6307d4..642b4b888f 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -77,7 +77,7 @@ 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 | FILTER_REFS_INCLUDE_BROKEN);\n+\tfilter_refs(&array, &filter, FILTER_REFS_ALL);\n \tref_array_sort(sorting, &array);\n \n \tif (!maxcount || array.nr < maxcount)\ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex b1839e0877..fa3aeb80f2 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -170,7 +170,7 @@ test_expect_success 'for-each-ref emits warnings for broken names' '\n \t! grep -e \"badname\" output &&\n \t! grep -e \"broken\\.\\.\\.symref\" output &&\n \ttest_i18ngrep \"ignoring ref with broken name refs/heads/broken\\.\\.\\.ref\" error &&\n-\ttest_i18ngrep \"ignoring broken ref refs/heads/badname\" error &&\n+\ttest_i18ngrep ! \"ignoring broken ref refs/heads/badname\" error &&\n \ttest_i18ngrep \"ignoring ref with broken name refs/heads/broken\\.\\.\\.symref\" error\n '\n \n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437015","messageId":"YU4ddK+tLFwj9oCT@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 15/16] ref-filter: drop broken-ref code entirely","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:48:20Z","receivedAt":"2021-09-24T18:48:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that none of our callers passes the INCLUDE_BROKEN flag, we can drop\nit entirely, along with the code to plumb it through to the\nfor_each_fullref_in() functions.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c | 11 ++++-------\n ref-filter.h |  1 -\n 2 files changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 93ce2a6ef2..e59bb4cf9c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2405,13 +2405,10 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n {\n \tstruct ref_filter_cbdata ref_cbdata;\n \tint ret = 0;\n-\tunsigned int broken = 0;\n \n \tref_cbdata.array = array;\n \tref_cbdata.filter = filter;\n \n-\tif (type & FILTER_REFS_INCLUDE_BROKEN)\n-\t\tbroken = 1;\n \tfilter->kind = type & FILTER_REFS_KIND_MASK;\n \n \tinit_contains_cache(&ref_cbdata.contains_cache);\n@@ -2428,13 +2425,13 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \t\t * of filter_ref_kind().\n \t\t */\n \t\tif (filter->kind == FILTER_REFS_BRANCHES)\n-\t\t\tret = for_each_fullref_in(\"refs/heads/\", ref_filter_handler, &ref_cbdata, broken);\n+\t\t\tret = for_each_fullref_in(\"refs/heads/\", ref_filter_handler, &ref_cbdata, 0);\n \t\telse if (filter->kind == FILTER_REFS_REMOTES)\n-\t\t\tret = for_each_fullref_in(\"refs/remotes/\", ref_filter_handler, &ref_cbdata, broken);\n+\t\t\tret = for_each_fullref_in(\"refs/remotes/\", ref_filter_handler, &ref_cbdata, 0);\n \t\telse if (filter->kind == FILTER_REFS_TAGS)\n-\t\t\tret = for_each_fullref_in(\"refs/tags/\", ref_filter_handler, &ref_cbdata, broken);\n+\t\t\tret = for_each_fullref_in(\"refs/tags/\", ref_filter_handler, &ref_cbdata, 0);\n \t\telse if (filter->kind & FILTER_REFS_ALL)\n-\t\t\tret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata, broken);\n+\t\t\tret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata, 0);\n \t\tif (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))\n \t\t\thead_ref(ref_filter_handler, &ref_cbdata);\n \t}\ndiff --git a/ref-filter.h b/ref-filter.h\nindex c15dee8d6b..b636f4389d 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -13,7 +13,6 @@\n #define QUOTE_PYTHON 4\n #define QUOTE_TCL 8\n \n-#define FILTER_REFS_INCLUDE_BROKEN 0x0001\n #define FILTER_REFS_TAGS           0x0002\n #define FILTER_REFS_BRANCHES       0x0004\n #define FILTER_REFS_REMOTES        0x0008\n-- \n2.33.0.1071.gb37e412355\n\n"},{"id":"437016","messageId":"YU4dkMt6rWhmsOlq@coredump.intra.peff.net","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"[PATCH 16/16] refs: drop \"broken\" flag from for_each_fullref_in()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T18:48:48Z","receivedAt":"2021-09-24T18:49:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"No callers pass in anything but \"0\" here. Likewise to our sibling\nfunctions. Note that some of them ferry along the flag, but none of\ntheir callers pass anything but \"0\" either.\n\nNor is anybody likely to change that. Callers which really want to see\nall of the raw refs use for_each_rawref(). And anybody interested in\niterating a subset of the refs will likely be happy to use the\nnow-default behavior of showing broken refs, but omitting dangling\nsymlinks.\n\nSo we can get rid of this whole feature.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rev-parse.c |  4 ++--\n ls-refs.c           |  2 +-\n ref-filter.c        | 19 +++++++++----------\n refs.c              | 22 ++++++----------------\n refs.h              |  9 +++------\n revision.c          |  2 +-\n 6 files changed, 22 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 22c4e1a4ff..8480a59f57 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -863,8 +863,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--bisect\")) {\n-\t\t\t\tfor_each_fullref_in(\"refs/bisect/bad\", show_reference, NULL, 0);\n-\t\t\t\tfor_each_fullref_in(\"refs/bisect/good\", anti_reference, NULL, 0);\n+\t\t\t\tfor_each_fullref_in(\"refs/bisect/bad\", show_reference, NULL);\n+\t\t\t\tfor_each_fullref_in(\"refs/bisect/good\", anti_reference, NULL);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (opt_with_value(arg, \"--branches\", &arg)) {\ndiff --git a/ls-refs.c b/ls-refs.c\nindex be09568910..7fe9675d3c 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -171,7 +171,7 @@ int ls_refs(struct repository *r, struct packet_reader *request)\n \tif (!data.prefixes.nr)\n \t\tstrvec_push(&data.prefixes, \"\");\n \tfor_each_fullref_in_prefixes(get_git_namespace(), data.prefixes.v,\n-\t\t\t\t     send_ref, &data, 0);\n+\t\t\t\t     send_ref, &data);\n \tpacket_fflush(stdout);\n \tstrvec_clear(&data.prefixes);\n \tstrbuf_release(&data.buf);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e59bb4cf9c..e15f0c4bac 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2100,16 +2100,15 @@ static int filter_pattern_match(struct ref_filter *filter, const char *refname)\n  */\n static int for_each_fullref_in_pattern(struct ref_filter *filter,\n \t\t\t\t       each_ref_fn cb,\n-\t\t\t\t       void *cb_data,\n-\t\t\t\t       int broken)\n+\t\t\t\t       void *cb_data)\n {\n \tif (!filter->match_as_path) {\n \t\t/*\n \t\t * in this case, the patterns are applied after\n \t\t * prefixes like \"refs/heads/\" etc. are stripped off,\n \t\t * so we have to look at everything:\n \t\t */\n-\t\treturn for_each_fullref_in(\"\", cb, cb_data, broken);\n+\t\treturn for_each_fullref_in(\"\", cb, cb_data);\n \t}\n \n \tif (filter->ignore_case) {\n@@ -2118,16 +2117,16 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,\n \t\t * so just return everything and let the caller\n \t\t * sort it out.\n \t\t */\n-\t\treturn for_each_fullref_in(\"\", cb, cb_data, broken);\n+\t\treturn for_each_fullref_in(\"\", cb, cb_data);\n \t}\n \n \tif (!filter->name_patterns[0]) {\n \t\t/* no patterns; we have to look at everything */\n-\t\treturn for_each_fullref_in(\"\", cb, cb_data, broken);\n+\t\treturn for_each_fullref_in(\"\", cb, cb_data);\n \t}\n \n \treturn for_each_fullref_in_prefixes(NULL, filter->name_patterns,\n-\t\t\t\t\t    cb, cb_data, broken);\n+\t\t\t\t\t    cb, cb_data);\n }\n \n /*\n@@ -2425,13 +2424,13 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \t\t * of filter_ref_kind().\n \t\t */\n \t\tif (filter->kind == FILTER_REFS_BRANCHES)\n-\t\t\tret = for_each_fullref_in(\"refs/heads/\", ref_filter_handler, &ref_cbdata, 0);\n+\t\t\tret = for_each_fullref_in(\"refs/heads/\", ref_filter_handler, &ref_cbdata);\n \t\telse if (filter->kind == FILTER_REFS_REMOTES)\n-\t\t\tret = for_each_fullref_in(\"refs/remotes/\", ref_filter_handler, &ref_cbdata, 0);\n+\t\t\tret = for_each_fullref_in(\"refs/remotes/\", ref_filter_handler, &ref_cbdata);\n \t\telse if (filter->kind == FILTER_REFS_TAGS)\n-\t\t\tret = for_each_fullref_in(\"refs/tags/\", ref_filter_handler, &ref_cbdata, 0);\n+\t\t\tret = for_each_fullref_in(\"refs/tags/\", ref_filter_handler, &ref_cbdata);\n \t\telse if (filter->kind & FILTER_REFS_ALL)\n-\t\t\tret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata, 0);\n+\t\t\tret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata);\n \t\tif (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))\n \t\t\thead_ref(ref_filter_handler, &ref_cbdata);\n \t}\ndiff --git a/refs.c b/refs.c\nindex d0f4e8726b..2be0d0f057 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1522,25 +1522,16 @@ int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)\n \treturn refs_for_each_ref_in(get_main_ref_store(the_repository), prefix, fn, cb_data);\n }\n \n-int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken)\n+int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data)\n {\n-\tenum do_for_each_ref_flags flag = 0;\n-\n-\tif (broken)\n-\t\tflag = DO_FOR_EACH_INCLUDE_BROKEN;\n \treturn do_for_each_ref(get_main_ref_store(the_repository),\n-\t\t\t       prefix, fn, 0, flag, cb_data);\n+\t\t\t       prefix, fn, 0, 0, cb_data);\n }\n \n int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,\n-\t\t\t     each_ref_fn fn, void *cb_data,\n-\t\t\t     unsigned int broken)\n+\t\t\t     each_ref_fn fn, void *cb_data)\n {\n-\tenum do_for_each_ref_flags flag = 0;\n-\n-\tif (broken)\n-\t\tflag = DO_FOR_EACH_INCLUDE_BROKEN;\n-\treturn do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);\n+\treturn do_for_each_ref(refs, prefix, fn, 0, 0, cb_data);\n }\n \n int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)\n@@ -1632,8 +1623,7 @@ static void find_longest_prefixes(struct string_list *out,\n \n int for_each_fullref_in_prefixes(const char *namespace,\n \t\t\t\t const char **patterns,\n-\t\t\t\t each_ref_fn fn, void *cb_data,\n-\t\t\t\t unsigned int broken)\n+\t\t\t\t each_ref_fn fn, void *cb_data)\n {\n \tstruct string_list prefixes = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *prefix;\n@@ -1648,7 +1638,7 @@ int for_each_fullref_in_prefixes(const char *namespace,\n \n \tfor_each_string_list_item(prefix, &prefixes) {\n \t\tstrbuf_addstr(&buf, prefix->string);\n-\t\tret = for_each_fullref_in(buf.buf, fn, cb_data, broken);\n+\t\tret = for_each_fullref_in(buf.buf, fn, cb_data);\n \t\tif (ret)\n \t\t\tbreak;\n \t\tstrbuf_setlen(&buf, namespace_len);\ndiff --git a/refs.h b/refs.h\nindex 48970dfc7e..10e7696a64 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -342,10 +342,8 @@ int for_each_ref(each_ref_fn fn, void *cb_data);\n int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);\n \n int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,\n-\t\t\t     each_ref_fn fn, void *cb_data,\n-\t\t\t     unsigned int broken);\n-int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,\n-\t\t\tunsigned int broken);\n+\t\t\t     each_ref_fn fn, void *cb_data);\n+int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data);\n \n /**\n  * iterate all refs in \"patterns\" by partitioning patterns into disjoint sets\n@@ -354,8 +352,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,\n  * callers should be prepared to ignore references that they did not ask for.\n  */\n int for_each_fullref_in_prefixes(const char *namespace, const char **patterns,\n-\t\t\t\t each_ref_fn fn, void *cb_data,\n-\t\t\t\t unsigned int broken);\n+\t\t\t\t each_ref_fn fn, void *cb_data);\n /**\n  * iterate refs from the respective area.\n  */\ndiff --git a/revision.c b/revision.c\nindex 0dabb5a0bc..b7a2baad0e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2548,7 +2548,7 @@ static int for_each_bisect_ref(struct ref_store *refs, each_ref_fn fn,\n \tstruct strbuf bisect_refs = STRBUF_INIT;\n \tint status;\n \tstrbuf_addf(&bisect_refs, \"refs/bisect/%s\", term);\n-\tstatus = refs_for_each_fullref_in(refs, bisect_refs.buf, fn, cb_data, 0);\n+\tstatus = refs_for_each_fullref_in(refs, bisect_refs.buf, fn, cb_data);\n \tstrbuf_release(&bisect_refs);\n \treturn status;\n }\n-- \n2.33.0.1071.gb37e412355\n"},{"id":"437041","messageId":"xmqqk0j5soz0.fsf@gitster.g","threadId":"56585","inReplyTo":"YU4ZOF9+ubmoItmK@coredump.intra.peff.net","subject":"Re: [PATCH 0/16] enabling GIT_REF_PARANOIA by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-24T20:22:43Z","receivedAt":"2021-09-24T20:22:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I recently ran into a situation where dealing with a corrupted\n> repository was more confusing than necessary, because Git by default\n> ignores corrupted refs in many commands.\n>\n> A while ago we introduced GIT_REF_PARANOIA, which works by including\n> broken refs in iteration, which then typically causes later operations\n> to fail (e.g., during repacking, you'd prefer to barf loudly when trying\n> to access the missing object rather than incorrectly assume the objects\n> from the broken ref aren't reachable).\n>\n> I think this is a better default for Git to have in general, not just\n> for a few select operations (we turn it on by default for pruning and\n> some repacks). We shouldn't see corruptions in general, and complaining\n> loudly when we do is the safest option. The reason we held back when the\n> knob was introduced was mostly out of deference to the historical\n> behavior.\n\nYeah, having an escape hatch to serve as a tool to deal with and\nrepair a corrupt repository might be worth considering, but I tend\nto agree that it is a better default to notice and loudly report a\ncorruption.\n\nThe series was a quite pleasant read.\n\nThanks.\n"},{"id":"437168","messageId":"20210927173818.528679-1-jonathantanmy@google.com","threadId":"56585","inReplyTo":"YU4Zse+dAeDg8RqV@coredump.intra.peff.net","subject":"Re: [PATCH 01/16] t7900: clean up some more broken refs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-27T17:38:18Z","receivedAt":"2021-09-27T17:47:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> @@ -277,7 +277,7 @@ test_expect_success 'incremental-repack task' '\n>  \n>  \t# Delete refs that have not been repacked in these packs.\n>  \tgit for-each-ref --format=\"delete %(refname)\" \\\n> -\t\trefs/prefetch refs/tags >refs &&\n> +\t\trefs/prefetch refs/tags refs/remotes >refs &&\n>  \tgit update-ref --stdin <refs &&\n>  \n>  \t# Replace the object directory with this pack layout.\n> @@ -286,6 +286,10 @@ test_expect_success 'incremental-repack task' '\n>  \tls $packDir/*.pack >packs-before &&\n>  \ttest_line_count = 3 packs-before &&\n>  \n> +\t# make sure we do not have any broken refs that were\n> +\t# missed in the deletion above\n> +\tgit for-each-ref &&\n\nFor what it's worth, I verified that a fatal error is indeed caused in\n\"git for-each-ref\" if refs/remotes was not deleted. This patch looks\ngood.\n"},{"id":"437169","messageId":"20210927174237.529476-1-jonathantanmy@google.com","threadId":"56585","inReplyTo":"YU4c9S6jypbS4YCK@coredump.intra.peff.net","subject":"Re: [PATCH 12/16] refs: turn on GIT_REF_PARANOIA by default","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-27T17:42:37Z","receivedAt":"2021-09-27T17:49:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Deleting a\n> broken ref now actually tells the hooks the correct \"before\" state,\n> rather than a confusing null oid.\n\nI believe that this is due to write_head_info() in\nbuiltin/receive-pack.c now advertising the broken ref, so that the\nclient knows what the old OID is when pushing in order to delete the ref\n(verified by running the relevant \"git push\" in the test with\nGIT_TRACE_PACKET=1, with and without GIT_REF_PARANOIA=0). If rerolling,\nmaybe this is worth adding in parentheses (e.g. \"(because such a ref\nwould be advertised to the client and the client would thus send the\n\"before\" OID when deleting it)\").\n\nAll the patches up to this look good.\n"},{"id":"437170","messageId":"20210927174703.530229-1-jonathantanmy@google.com","threadId":"56585","inReplyTo":"YU4dkMt6rWhmsOlq@coredump.intra.peff.net","subject":"Re: [PATCH 16/16] refs: drop \"broken\" flag from for_each_fullref_in()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-27T17:47:03Z","receivedAt":"2021-09-27T17:51:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> No callers pass in anything but \"0\" here. Likewise to our sibling\n> functions. Note that some of them ferry along the flag, but none of\n> their callers pass anything but \"0\" either.\n> \n> Nor is anybody likely to change that. Callers which really want to see\n> all of the raw refs use for_each_rawref(). And anybody interested in\n> iterating a subset of the refs will likely be happy to use the\n> now-default behavior of showing broken refs, but omitting dangling\n> symlinks.\n> \n> So we can get rid of this whole feature.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n\nAll the patches look good.\nReviewed-by: Jonathan Tan <jonathantanmy@google.com>\n\nAs you have mentioned before, this clashes with my series [1]. But since\nI'll need to update my series anyway to reinclude the\nDO_FOR_EACH_INCLUDE_BROKEN flag, the obvious thing is for me to rebase\nmine on top of yours so that yours can go in first, so I don't mind\ndoing that.\n\n[1] https://lore.kernel.org/git/cover.1632242495.git.jonathantanmy@google.com/\n"},{"id":"437178","messageId":"YVIgWnsApPs35YUd@coredump.intra.peff.net","threadId":"56585","inReplyTo":"20210927173818.528679-1-jonathantanmy@google.com","subject":"Re: [PATCH 01/16] t7900: clean up some more broken refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-27T19:49:46Z","receivedAt":"2021-09-27T19:49:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 27, 2021 at 10:38:18AM -0700, Jonathan Tan wrote:\n\n> > @@ -277,7 +277,7 @@ test_expect_success 'incremental-repack task' '\n> >  \n> >  \t# Delete refs that have not been repacked in these packs.\n> >  \tgit for-each-ref --format=\"delete %(refname)\" \\\n> > -\t\trefs/prefetch refs/tags >refs &&\n> > +\t\trefs/prefetch refs/tags refs/remotes >refs &&\n> >  \tgit update-ref --stdin <refs &&\n> >  \n> >  \t# Replace the object directory with this pack layout.\n> > @@ -286,6 +286,10 @@ test_expect_success 'incremental-repack task' '\n> >  \tls $packDir/*.pack >packs-before &&\n> >  \ttest_line_count = 3 packs-before &&\n> >  \n> > +\t# make sure we do not have any broken refs that were\n> > +\t# missed in the deletion above\n> > +\tgit for-each-ref &&\n> \n> For what it's worth, I verified that a fatal error is indeed caused in\n> \"git for-each-ref\" if refs/remotes was not deleted. This patch looks\n> good.\n\nMe too. :) It works because for-each-ref's default format will try to\nshow the type of the object, so it complains when the object is missing.\n\nWe could make this \"git fsck\" if that's less subtle, but this is a bit\ncheaper.\n\n-Peff\n"}]}