refs/files: catch conflicts on case-insensitive file-systems

During the 'prepare' phase of a reference transaction in the files
backend, we create the lock files for references to be created. When
using batched updates on case-insensitive filesystems, the entire
batched updates would be aborted if there are conflicting names such as:

  refs/heads/Foo
  refs/heads/foo

This affects all commands which were migrated to use batched updates in
Git 2.51, including 'git-fetch(1)' and 'git-receive-pack(1)'. Before
that, reference updates would be applied serially with one transaction
used per update. When users fetched multiple references on
case-insensitive systems, subsequent references would simply overwrite
any earlier references. So when fetching:

  refs/heads/foo: 5f34ec0bfeac225b1c854340257a65b106f70ea6
  refs/heads/Foo: ec3053b0977e83d9b67fc32c4527a117953994f3
  refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56

The user would simply end up with:

  refs/heads/foo: ec3053b0977e83d9b67fc32c4527a117953994f3
  refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56

This is buggy behavior since the user is never informed about the
overrides performed and missing references. Nevertheless, the user is
left with a working repository with a subset of the references. Since
Git 2.51, in such situations fetches would simply fail without updating
any references. Which is also buggy behavior and worse off since the
user is left without any references.

The error is triggered in `lock_raw_ref()` where the files backend
attempts to create a lock file. When a lock file already exists the
function returns a 'REF_TRANSACTION_ERROR_GENERIC'. When this happens,
the entire batched updates, not individual operation, is aborted as if
it were in a transaction.

Change this to return 'REF_TRANSACTION_ERROR_CASE_CONFLICT' instead to
aid the batched update mechanism to simply reject such errors. The
change only affects batched updates since batched updates will reject
individual updates with non-generic errors. So specifically this would
only affect:

    1. git fetch
    2. git receive-pack
    3. git update-ref --batch-updates

This bubbles the error type up to `files_transaction_prepare()` which
tries to lock each reference update. So if the locking fails, we check
if the rejection type can be ignored, which is done by calling
`ref_transaction_maybe_set_rejected()`.

As the error type is now 'REF_TRANSACTION_ERROR_CASE_CONFLICT',
the specific reference update would simply be rejected, while other
updates in the transaction would continue to be applied. This allows
partial application of references in case-insensitive filesystems when
fetching colliding references.

While the earlier implementation allowed the last reference to be
applied overriding the initial references, this change would allow the
first reference to be applied while rejecting consequent collisions.
This should be an okay compromise since with the files backend, there is
no scenario possible where we would retain all colliding references.

Let's also be more proactive and notify users on case-insensitive
filesystems about such problems by providing a brief about the issue
while also recommending using the reftable backend, which doesn't have
the same issue.

Reported-by: Joe Drew <joe.drew@indexexchange.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 24645c465335de70cece7e1a2109eeaec4ed1748..c7ff3480fb182711eccca1421798387a3391a6c4 100644 (file)
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1643,7 +1643,8 @@ cleanup:
 
 struct ref_rejection_data {
        int *retcode;
-       int conflict_msg_shown;
+       bool conflict_msg_shown;
+       bool case_sensitive_msg_shown;
        const char *remote_name;
 };
 
@@ -1657,11 +1658,25 @@ static void ref_transaction_rejection_handler(const char *refname,
 {
        struct ref_rejection_data *data = cb_data;
 
-       if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
+       if (err == REF_TRANSACTION_ERROR_CASE_CONFLICT && ignore_case &&
+           !data->case_sensitive_msg_shown) {
+               error(_("You're on a case-insensitive filesystem, and the remote you are\n"
+                       "trying to fetch from has references that only differ in casing. It\n"
+                       "is impossible to store such references with the 'files' backend. You\n"
+                       "can either accept this as-is, in which case you won't be able to\n"
+                       "store all remote references on disk. Or you can alternatively\n"
+                       "migrate your repository to use the 'reftable' backend with the\n"
+                       "following command:\n\n    git refs migrate --ref-format=reftable\n\n"
+                       "Please keep in mind that not all implementations of Git support this\n"
+                       "new format yet. So if you use tools other than Git to access this\n"
+                       "repository it may not be an option to migrate to reftables.\n"));
+               data->case_sensitive_msg_shown = true;
+       } else if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT &&
+                  !data->conflict_msg_shown) {
                error(_("some local refs could not be updated; try running\n"
                        " 'git remote prune %s' to remove any old, conflicting "
                        "branches"), data->remote_name);
-               data->conflict_msg_shown = 1;
+               data->conflict_msg_shown = true;
        } else {
                const char *reason = ref_transaction_error_msg(err);
 

diff --git a/refs.c b/refs.c
index bfdbe718b70a3806cdc2e1da88dc96a90aeee3ce..4c1c339ed9b452f5b99e5690a31414422879fba3 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -3321,6 +3321,8 @@ const char *ref_transaction_error_msg(enum ref_transaction_error err)
                return "invalid new value provided";
        case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
                return "expected symref but found regular ref";
+       case REF_TRANSACTION_ERROR_CASE_CONFLICT:
+               return "reference conflict due to case-insensitive filesystem";
        default:
                return "unknown failure";
        }

diff --git a/refs.h b/refs.h
index eedbb599c52aac93408ae0690ac207dc9b7caa82..41915086b36da1a5c9d040f4b08f4897a3662bf0 100644 (file)
--- a/refs.h
+++ b/refs.h
@@ -31,6 +31,8 @@ enum ref_transaction_error {
        REF_TRANSACTION_ERROR_INVALID_NEW_VALUE = -6,
        /* Expected ref to be symref, but is a regular ref */
        REF_TRANSACTION_ERROR_EXPECTED_SYMREF = -7,
+       /* Cannot create ref due to case-insensitive filesystem */
+       REF_TRANSACTION_ERROR_CASE_CONFLICT = -8,
 };
 
 /*

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 088b52c740b9ff0587fe3935da33f69478ae1c5d..01df32904bee61cff4cdb75fcba972fe4d232409 100644 (file)
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -647,6 +647,26 @@ static void unlock_ref(struct ref_lock *lock)
        }
 }
 
+/*
+ * Check if the transaction has another update with a case-insensitive refname
+ * match.
+ *
+ * If the update is part of the transaction, we only check up to that index.
+ * Further updates are expected to call this function to match previous indices.
+ */
+static bool transaction_has_case_conflicting_update(struct ref_transaction *transaction,
+                                                   struct ref_update *update)
+{
+       for (size_t i = 0; i < transaction->nr; i++) {
+               if (transaction->updates[i] == update)
+                       break;
+
+               if (!strcasecmp(transaction->updates[i]->refname, update->refname))
+                       return true;
+       }
+       return false;
+}
+
 /*
  * Lock refname, without following symrefs, and set *lock_p to point
  * at a newly-allocated lock object. Fill in lock->old_oid, referent,
@@ -677,16 +697,17 @@ static void unlock_ref(struct ref_lock *lock)
  * - Generate informative error messages in the case of failure
  */
 static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
-                                              struct ref_update *update,
+                                              struct ref_transaction *transaction,
                                               size_t update_idx,
                                               int mustexist,
                                               struct string_list *refnames_to_check,
-                                              const struct string_list *extras,
                                               struct ref_lock **lock_p,
                                               struct strbuf *referent,
                                               struct strbuf *err)
 {
        enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC;
+       struct ref_update *update = transaction->updates[update_idx];
+       const struct string_list *extras = &transaction->refnames;
        const char *refname = update->refname;
        unsigned int *type = &update->type;
        struct ref_lock *lock;
@@ -776,6 +797,9 @@ retry:
                        goto retry;
                } else {
                        unable_to_lock_message(ref_file.buf, myerr, err);
+                       if (myerr == EEXIST && ignore_case &&
+                           transaction_has_case_conflicting_update(transaction, update))
+                               ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
                        goto error_return;
                }
        }
@@ -2583,9 +2607,8 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re
        if (lock) {
                lock->count++;
        } else {
-               ret = lock_raw_ref(refs, update, update_idx, mustexist,
-                                  refnames_to_check, &transaction->refnames,
-                                  &lock, &referent, err);
+               ret = lock_raw_ref(refs, transaction, update_idx, mustexist,
+                                  refnames_to_check, &lock, &referent, err);
                if (ret) {
                        char *reason;
 

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 96648a6e5dfd89cc31f2c75654dd831c39ad5611..08d5df2af726b1ffedc8a0c07fdf9b84cc4218cb 100755 (executable)
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -2294,6 +2294,59 @@ do
                )
        '
 
+       test_expect_success CASE_INSENSITIVE_FS,REFFILES "stdin $type batch-updates existing reference" '
+               git init repo &&
+               test_when_finished "rm -fr repo" &&
+               (
+                       cd repo &&
+                       test_commit one &&
+                       old_head=$(git rev-parse HEAD) &&
+                       test_commit two &&
+                       head=$(git rev-parse HEAD) &&
+
+                       {
+                               format_command $type "create refs/heads/foo" "$head" &&
+                               format_command $type "create refs/heads/ref" "$old_head" &&
+                               format_command $type "create refs/heads/Foo" "$old_head"
+                       } >stdin &&
+                       git update-ref $type --stdin --batch-updates <stdin >stdout &&
+
+                       echo $head >expect &&
+                       git rev-parse refs/heads/foo >actual &&
+                       echo $old_head >expect &&
+                       git rev-parse refs/heads/ref >actual &&
+                       test_cmp expect actual &&
+                       test_grep -q "reference conflict due to case-insensitive filesystem" stdout
+               )
+       '
+
+       test_expect_success CASE_INSENSITIVE_FS "stdin $type batch-updates existing reference" '
+               git init --ref-format=reftable repo &&
+               test_when_finished "rm -fr repo" &&
+               (
+                       cd repo &&
+                       test_commit one &&
+                       old_head=$(git rev-parse HEAD) &&
+                       test_commit two &&
+                       head=$(git rev-parse HEAD) &&
+
+                       {
+                               format_command $type "create refs/heads/foo" "$head" &&
+                               format_command $type "create refs/heads/ref" "$old_head" &&
+                               format_command $type "create refs/heads/Foo" "$old_head"
+                       } >stdin &&
+                       git update-ref $type --stdin --batch-updates <stdin >stdout &&
+
+                       echo $head >expect &&
+                       git rev-parse refs/heads/foo >actual &&
+                       echo $old_head >expect &&
+                       git rev-parse refs/heads/ref >actual &&
+                       test_cmp expect actual &&
+                       git rev-parse refs/heads/Foo >actual &&
+                       test_cmp expect actual
+               )
+       '
+
        test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" '
                git init repo &&
                test_when_finished "rm -fr repo" &&

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ebc696546bc33252d582ec5a15f2e1f031d19065..57f60da81bd61bfc8ed5432aaa05a685bfb9f4a4 100755 (executable)
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -47,7 +47,13 @@ test_expect_success "clone and setup child repos" '
                git config set branch.main.merge refs/heads/one
        ) &&
        git clone . bundle &&
-       git clone . seven
+       git clone . seven &&
+       git clone --ref-format=reftable . case_sensitive &&
+       (
+               cd case_sensitive &&
+               git branch branch1 &&
+               git branch bRanch1
+       )
 '
 
 test_expect_success "fetch test" '
@@ -1526,6 +1532,20 @@ test_expect_success SYMLINKS 'clone does not get confused by a D/F conflict' '
        test_path_is_missing whoops
 '
 
+test_expect_success CASE_INSENSITIVE_FS,REFFILES 'existing references in a case insensitive filesystem' '
+       test_when_finished rm -rf case_insensitive &&
+       (
+               git init --bare case_insensitive &&
+               cd case_insensitive &&
+               git remote add origin -- ../case_sensitive &&
+               test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err &&
+               test_grep "You${SQ}re on a case-insensitive filesystem" err &&
+               git rev-parse refs/heads/main >expect &&
+               git rev-parse refs/heads/branch1 >actual &&
+               test_cmp expect actual
+       )
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd