refs: turn on GIT_REF_PARANOIA by default

The original point of the GIT_REF_PARANOIA flag was to include broken refs in iterations, so that possibly-destructive operations would not silently ignore them (and would generally instead try to operate on the oids and fail when the objects could not be accessed). We already turned this on by default for some dangerous operations, like "repack -ad" (where missing a reachability tip would mean dropping the associated history). But it was not on for general use, even though it could easily result in the spreading of corruption (e.g., imagine cloning a repository which simply omits some of its refs because their objects are missing; the result quietly succeeds even though you did not clone everything!). This patch turns on GIT_REF_PARANOIA by default. So a clone as mentioned above would actually fail (upload-pack tells us about the broken ref, and when we ask for the objects, pack-objects fails to deliver them). This may be inconvenient when working with a corrupted repository, but: - we are better off to err on the side of complaining about corruption, and then provide mechanisms for explicitly loosening safety. - this is only one type of corruption anyway. If we are missing any other objects in the history that _aren't_ ref tips, then we'd behave similarly (happily show the ref, but then barf when we started traversing). We retain the GIT_REF_PARANOIA variable, but simply default it to "1" instead of "0". That gives the user an escape hatch for loosening this when working with a corrupt repository. It won't work across a remote connection to upload-pack (because we can't necessarily set environment variables on the remote), but there the client has other options (e.g., choosing which refs to fetch). As a bonus, this also makes ref iteration faster in general (because we don't have to call has_object_file() for each ref), though probably not noticeably so in the general case. In a repo with a million refs, it shaved a few hundred milliseconds off of upload-pack's advertisement; that's noticeable, but most repos are not nearly that large. The possible downside here is that any operation which iterates refs but doesn't ever open their objects may now quietly claim to have X when the object is corrupted (e.g., "git rev-list new-branch --not --all" will treat a broken ref as uninteresting). But again, that's not really any different than corruption below the ref level. We might have refs/heads/old-branch as non-corrupt, but we are not actively checking that we have the entire reachable history. Or the pointed-to object could even be corrupted on-disk (but our "do we have it" check would still succeed). In that sense, this is merely bringing ref-corruption in line with general object corruption. One alternative implementation would be to actually check for broken refs, and then _immediately die_ if we see any. That would cause the "rev-list --not --all" case above to abort immediately. But in many ways that's the worst of all worlds: - it still spends time looking up the objects an extra time - it still doesn't catch corruption below the ref level - it's even more inconvenient; with the current implementation of GIT_REF_PARANOIA for something like upload-pack, we can make the advertisement and let the client choose a non-broken piece of history. If we bail as soon as we see a broken ref, they cannot even see the advertisement. The test changes here show some of the fallout. A non-destructive "git repack -adk" now fails by default (but we can override it). Deleting a broken ref now actually tells the hooks the correct "before" state, rather than a confusing null oid. Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Sep 24, 2021 at 14:46 UTC 968f12fdac2601086dea7e10db17f1c50d704a07
4 files changed +23 -15
Documentation/git.txt
+10 -9
@@ -867,15 +867,16 @@ for full details.
867 end user, to be recorded in the body of the reflog.
868
869 `GIT_REF_PARANOIA`::
870 - If set to `1`, include broken or badly named refs when iterating
871 - over lists of refs. In a normal, non-corrupted repository, this
872 - does nothing. However, enabling it may help git to detect and
873 - abort some operations in the presence of broken refs. Git sets
874 - this variable automatically when performing destructive
875 - operations like linkgit:git-prune[1]. You should not need to set
876 - it yourself unless you want to be paranoid about making sure
877 - an operation has touched every ref (e.g., because you are
878 - cloning a repository to make a backup).
870 + If set to `0`, ignore broken or badly named refs when iterating
871 + over lists of refs. Normally Git will try to include any such
872 + refs, which may cause some operations to fail. This is usually
873 + preferable, as potentially destructive operations (e.g.,
874 + linkgit:git-prune[1]) are better off aborting rather than
875 + ignoring broken refs (and thus considering the history they
876 + point to as not worth saving). The default value is `1` (i.e.,
877 + be paranoid about detecting and aborting all operations). You
878 + should not normally need to set this to `0`, but it may be
879 + useful when trying to salvage data from a corrupted repository.
880
881 `GIT_ALLOW_PROTOCOL`::
882 If set to a colon-separated list of protocols, behave as if
refs.c
+1 -1
@@ -1420,7 +1420,7 @@ struct ref_iterator *refs_ref_iterator_begin(
1420
1421 if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {
1422 if (ref_paranoia < 0)
1423 - ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0);
1423 + ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 1);
1424 if (ref_paranoia) {
1425 flags |= DO_FOR_EACH_INCLUDE_BROKEN;
1426 flags |= DO_FOR_EACH_OMIT_DANGLING_SYMREFS;
t/t5312-prune-corruption.sh
+8 -2
@@ -49,11 +49,17 @@ test_expect_success 'put bogus object into pack' '
49 git cat-file -e $bogus
50 '
51
52 -test_expect_success 'non-destructive repack ignores bogus name' '
52 +test_expect_success 'non-destructive repack bails on bogus ref' '
53 create_bogus_ref &&
54 - git repack -adk
54 + test_must_fail git repack -adk
55 '
56
57 +test_expect_success 'GIT_REF_PARANOIA=0 overrides safety' '
58 + create_bogus_ref &&
59 + GIT_REF_PARANOIA=0 git repack -adk
60 +'
61 +
62 +
63 test_expect_success 'destructive repack keeps packed object' '
64 create_bogus_ref &&
65 test_must_fail git repack -Ad --unpack-unreachable=now &&
t/t5516-fetch-push.sh
+4 -3
@@ -707,20 +707,21 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
707
708 test_expect_success 'deleting dangling ref triggers hooks with correct args' '
709 mk_test_with_hooks testrepo heads/branch &&
710 + orig=$(git -C testrepo rev-parse refs/heads/branch) &&
711 rm -f testrepo/.git/objects/??/* &&
712 git push testrepo :refs/heads/branch &&
713 (
714 cd testrepo/.git &&
715 cat >pre-receive.expect <<-EOF &&
715 - $ZERO_OID $ZERO_OID refs/heads/branch
716 + $orig $ZERO_OID refs/heads/branch
717 EOF
718
719 cat >update.expect <<-EOF &&
719 - refs/heads/branch $ZERO_OID $ZERO_OID
720 + refs/heads/branch $orig $ZERO_OID
721 EOF
722
723 cat >post-receive.expect <<-EOF &&
723 - $ZERO_OID $ZERO_OID refs/heads/branch
724 + $orig $ZERO_OID refs/heads/branch
725 EOF
726
727 cat >post-update.expect <<-EOF &&