clone: die() instead of BUG() on bad refs

When cloning directly from a local repository, we load a list of refs
based on scanning the $GIT_DIR/refs/ directory of the "server"
repository. If files exist in that directory that do not parse as
hexadecimal hashes, then the ref array used by write_remote_refs()
ends up with some entries with null OIDs. This causes us to hit a BUG()
statement in ref_transaction_create():

  BUG: create called without valid new_oid

This BUG() call used to be a die() until 033abf97f (Replace all
die("BUG: ...") calls by BUG() ones, 2018-05-02). Before that, the die()
was added by f04c5b552 (ref_transaction_create(): check that new_sha1 is
valid, 2015-02-17).

The original report for this bug [1] mentioned that this problem did not
exist in Git 2.27.0. The failure bisects unsurprisingly to 968f12fda
(refs: turn on GIT_REF_PARANOIA by default, 2021-09-24). When
GIT_REF_PARANOIA is enabled, this case always fails as far back as I am
able to successfully compile and test the Git codebase.

[1] https://github.com/git-for-windows/git/issues/3781

There are two approaches to consider here. One would be to remove this
BUG() statement in favor of returning with an error. There are only two
callers to ref_transaction_create(), so this would have a limited
impact.

The other approach would be to add special casing in 'git clone' to
avoid this faulty input to the method.

While I originally started with changing 'git clone', I decided that
modifying ref_transaction_create() was a more complete solution. This
prevents failing with a BUG() statement when we already have a good way
to report an error (including a reason for that error) within the
method. Both callers properly check the return value and die() with the
error message, so this is an appropriate direction.

The added test helps check against a regression, but does check that our
intended error message is handled correctly.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs.c b/refs.c
index 9db66e995518c9f85acd0de0c5fa44658fdcf523..90bcb2716873592864e2496951f913618521cb45 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -1109,8 +1109,10 @@ int ref_transaction_create(struct ref_transaction *transaction,
                           unsigned int flags, const char *msg,
                           struct strbuf *err)
 {
-       if (!new_oid || is_null_oid(new_oid))
-               BUG("create called without valid new_oid");
+       if (!new_oid || is_null_oid(new_oid)) {
+               strbuf_addf(err, "'%s' has a null OID", refname);
+               return 1;
+       }
        return ref_transaction_update(transaction, refname, new_oid,
                                      null_oid(), flags, msg, err);
 }

diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
index 7d63365f93a0a50eda1d4ec9b55c19890f610102..21ab6192839a006060b6e2e864aab93989a418f1 100755 (executable)
--- a/t/t5605-clone-local.sh
+++ b/t/t5605-clone-local.sh
@@ -141,4 +141,13 @@ test_expect_success 'cloning locally respects "-u" for fetching refs' '
        test_must_fail git clone --bare -u false a should_not_work.git
 '
 
+test_expect_success 'local clone from repo with corrupt refs fails gracefully' '
+       git init corrupt &&
+       test_commit -C corrupt one &&
+       echo a >corrupt/.git/refs/heads/topic &&
+
+       test_must_fail git clone corrupt working 2>err &&
+       grep "has a null OID" err
+'
+
 test_done