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>

diff --git a/Documentation/git.txt b/Documentation/git.txt
index abace9eac2e2f1f8dca2e0c98341d6be5260c43d..d63c65e67d825d6aac6181de79689557bd76a019 100644 (file)
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -867,15 +867,16 @@ for full details.
        end user, to be recorded in the body of the reflog.
 
 `GIT_REF_PARANOIA`::
-       If set to `1`, include broken or badly named refs when iterating
-       over lists of refs. In a normal, non-corrupted repository, this
-       does nothing. However, enabling it may help git to detect and
-       abort some operations in the presence of broken refs. Git sets
-       this variable automatically when performing destructive
-       operations like linkgit:git-prune[1]. You should not need to set
-       it yourself unless you want to be paranoid about making sure
-       an operation has touched every ref (e.g., because you are
-       cloning a repository to make a backup).
+       If set to `0`, ignore broken or badly named refs when iterating
+       over lists of refs. Normally Git will try to include any such
+       refs, which may cause some operations to fail. This is usually
+       preferable, as potentially destructive operations (e.g.,
+       linkgit:git-prune[1]) are better off aborting rather than
+       ignoring broken refs (and thus considering the history they
+       point to as not worth saving). The default value is `1` (i.e.,
+       be paranoid about detecting and aborting all operations). You
+       should not normally need to set this to `0`, but it may be
+       useful when trying to salvage data from a corrupted repository.
 
 `GIT_ALLOW_PROTOCOL`::
        If set to a colon-separated list of protocols, behave as if

diff --git a/refs.c b/refs.c
index e72510813e5dc90cec13f286d55a242bfee8b94f..ac19c689fafc3ee50689306ebd7d0e11a98740f7 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -1420,7 +1420,7 @@ struct ref_iterator *refs_ref_iterator_begin(
 
        if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {
                if (ref_paranoia < 0)
-                       ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0);
+                       ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 1);
                if (ref_paranoia) {
                        flags |= DO_FOR_EACH_INCLUDE_BROKEN;
                        flags |= DO_FOR_EACH_OMIT_DANGLING_SYMREFS;

diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh
index d8ec5a746269f264fe9aa50400f476ec581d347a..ea889c088a51f635e1e111f1e55d02cd21b53d0f 100755 (executable)
--- a/t/t5312-prune-corruption.sh
+++ b/t/t5312-prune-corruption.sh
@@ -49,11 +49,17 @@ test_expect_success 'put bogus object into pack' '
        git cat-file -e $bogus
 '
 
-test_expect_success 'non-destructive repack ignores bogus name' '
+test_expect_success 'non-destructive repack bails on bogus ref' '
        create_bogus_ref &&
-       git repack -adk
+       test_must_fail git repack -adk
 '
 
+test_expect_success 'GIT_REF_PARANOIA=0 overrides safety' '
+       create_bogus_ref &&
+       GIT_REF_PARANOIA=0 git repack -adk
+'
+
+
 test_expect_success 'destructive repack keeps packed object' '
        create_bogus_ref &&
        test_must_fail git repack -Ad --unpack-unreachable=now &&

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b13553ecf4456ab143989c733eda6ad742e9d61d..8212ca56dc5b132edb552fba714657eed3579681 100755 (executable)
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -707,20 +707,21 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
 
 test_expect_success 'deleting dangling ref triggers hooks with correct args' '
        mk_test_with_hooks testrepo heads/branch &&
+       orig=$(git -C testrepo rev-parse refs/heads/branch) &&
        rm -f testrepo/.git/objects/??/* &&
        git push testrepo :refs/heads/branch &&
        (
                cd testrepo/.git &&
                cat >pre-receive.expect <<-EOF &&
-               $ZERO_OID $ZERO_OID refs/heads/branch
+               $orig $ZERO_OID refs/heads/branch
                EOF
 
                cat >update.expect <<-EOF &&
-               refs/heads/branch $ZERO_OID $ZERO_OID
+               refs/heads/branch $orig $ZERO_OID
                EOF
 
                cat >post-receive.expect <<-EOF &&
-               $ZERO_OID $ZERO_OID refs/heads/branch
+               $orig $ZERO_OID refs/heads/branch
                EOF
 
                cat >post-update.expect <<-EOF &&