refs/files: handle D/F conflicts during locking

The previous commit added the necessary validation and checks for F/D
conflicts in the files backend when working on case insensitive systems.

There is still a possibility for D/F conflicts. This is a different from
the F/D since for F/D conflicts, there would not be a conflict during
the lock creation phase:

    refs/heads/foo.lock
    refs/heads/foo/bar.lock

However there would be a conflict when the locks are committed, since we
cannot have 'refs/heads/foo/bar' and 'refs/heads/foo'. These kinds of
conflicts are checked and resolved in
`refs_verify_refnames_available()`, so the previous commit ensured that
for case-insensitive filesystems, we would lowercase the inputs to that
function.

For D/F conflicts, there is a conflict during the lock creation phase
itself:

    refs/heads/foo/bar.lock
    refs/heads/foo.lock

As in `lock_raw_ref()` after creating the lock, we also check for D/F
conflicts. This can occur in case-insensitive filesystems when trying to
fetch case-conflicted references like:

    refs/heads/Foo/new
    refs/heads/foo

D/F conflicts can also occur in case-sensitive filesystems, when the
repository already contains a directory with a lock file
'refs/heads/foo/bar.lock' and trying to fetch 'refs/heads/foo'. This
doesn't concern directories containing garbage files as those are
handled on a higher level.

To fix this, simply categorize the error as a name conflict. Also remove
this reference from the list of valid refnames for availability checks.
By categorizing the error and removing it from the list of valid
references, batched updates now knows to reject such reference updates
and apply the other reference updates.

Fix a small typo in `ref_transaction_maybe_set_rejected()` while here.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs.c b/refs.c
index 4c1c339ed9b452f5b99e5690a31414422879fba3..e7109ea5fea39676281e2bebe99b11bd5ed77f6e 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -1223,7 +1223,7 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
                return 0;
 
        if (!transaction->rejections)
-               BUG("transaction not inititalized with failure support");
+               BUG("transaction not initialized with failure support");
 
        /*
         * Don't accept generic errors, since these errors are not user
@@ -1232,6 +1232,13 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
        if (err == REF_TRANSACTION_ERROR_GENERIC)
                return 0;
 
+       /*
+        * Rejected refnames shouldn't be considered in the availability
+        * checks, so remove them from the list.
+        */
+       string_list_remove(&transaction->refnames,
+                          transaction->updates[update_idx]->refname, 0);
+
        transaction->updates[update_idx]->rejection_err = err;
        ALLOC_GROW(transaction->rejections->update_indices,
                   transaction->rejections->nr + 1,

diff --git a/refs/files-backend.c b/refs/files-backend.c
index bfdf85121a8cb549fc7586ab715e3ede3ebd23d4..4a0343d827b5d215c7b86f7b7e3c9e17a386e2e8 100644 (file)
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -870,6 +870,7 @@ retry:
                                goto error_return;
                        } else if (remove_dir_recursively(&ref_file,
                                                          REMOVE_DIR_EMPTY_ONLY)) {
+                               ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
                                if (refs_verify_refname_available(
                                                    &refs->base, refname,
                                                    extras, NULL, 0, err)) {
@@ -877,14 +878,14 @@ retry:
                                         * The error message set by
                                         * verify_refname_available() is OK.
                                         */
-                                       ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
                                        goto error_return;
                                } else {
                                        /*
-                                        * We can't delete the directory,
-                                        * but we also don't know of any
-                                        * references that it should
-                                        * contain.
+                                        * Directory conflicts can occur if there
+                                        * is an existing lock file in the directory
+                                        * or if the filesystem is case-insensitive
+                                        * and the directory contains a valid reference
+                                        * but conflicts with the update.
                                         */
                                        strbuf_addf(err, "there is a non-empty directory '%s' "
                                                    "blocking reference '%s'",

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 08dbea6503c21117b3ceedc76562280d58faf381..6b2739db264f8b6956e9756111e1bea6e84fecb0 100755 (executable)
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -59,6 +59,12 @@ test_expect_success "clone and setup child repos" '
                cd case_sensitive_fd &&
                git branch foo/bar &&
                git branch Foo
+       ) &&
+       git clone --ref-format=reftable . case_sensitive_df &&
+       (
+               cd case_sensitive_df &&
+               git branch Foo/bar &&
+               git branch foo
        )
 '
 
@@ -1592,6 +1598,46 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'F/D conflict on case insensiti
        )
 '
 
+test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensitive filesystem' '
+       test_when_finished rm -rf case_insensitive &&
+       (
+               git init --bare case_insensitive &&
+               cd case_insensitive &&
+               git remote add origin -- ../case_sensitive_df &&
+               test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err &&
+               test_grep "failed: refname conflict" err &&
+               git rev-parse refs/heads/main >expect &&
+               git rev-parse refs/heads/Foo/bar >actual &&
+               test_cmp expect actual
+       )
+'
+
+test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '
+       (
+               git init --ref-format=reftable base &&
+               cd base &&
+               echo >file update &&
+               git add . &&
+               git commit -m "updated" &&
+               git branch -M main &&
+
+               git update-ref refs/heads/foo @ &&
+               git update-ref refs/heads/branch @ &&
+               cd .. &&
+
+               git init --ref-format=files --bare repo &&
+               cd repo &&
+               git remote add origin ../base &&
+               mkdir refs/heads/foo &&
+               touch refs/heads/foo/random.lock &&
+               test_must_fail git fetch origin "refs/heads/*:refs/heads/*" 2>err &&
+               test_grep "some local refs could not be updated; try running" err &&
+               git rev-parse refs/heads/main >expect &&
+               git rev-parse refs/heads/branch >actual &&
+               test_cmp expect actual
+       )
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd