[PATCH v2 0/4] refs/files: fix issues with git-fetch on case-insensitive FS
From: Karthik Nayak <hidden>
Date: 2025-09-08 12:37:40
Hello! With Git 2.51 we moved 'git-fetch(1)' and 'git-receive-pack(1)' to use batched updates while doing reference updates. This provided a nice perf boost since both commands will now use a single transaction for reference updates. This removes the overhead of using individual transaction per reference update and also avoids unnecessary auto-compaction between reference updates in the reftable backend. However, in the files-backend it does introduce a few bugs around conflicts. The reported bug was around case-insensitive filesystems [1], but we also fix some adjacent issues: 1. When fetching references such as: - refs/heads/foo - refs/heads/Foo Earlier we would simply overwrite the first reference with the second and continue. Since Git 2.51 we simply abort stating a conflict. This is resolved in the first commit by explicitly categorizing the error as non-GENERIC. This allows batched updates to reject the particular update, while updating the rest. 2. When fetching references and a lock for a particular reference already exits. We treat this is a GENERIC error, which fails the entire update. By categorizing this error as non-GENERIC, we can reject this specific update and update the other references. 3. When fetching references such as with F/D conflict: - refs/heads/foo - refs/heads/Foo/bar Earlier we would apply the first, while the second would fail due to conflict. Since Git 2.51, the lock files for both would be created, but the 'commit' phase would abruptly end leaving the lock files. The second commit fixes this by ensuring that on case-insensitive filesystems we lowercase the refnames for availability check to ensure F/D are caught and reported to the user. 4. When fetching references with D/F conflict: - refs/heads/Foo/bar - refs/heads/foo The creation of the second reference's lock in `lock_raw_ref()` catches the D/F conflict, but we mark this as a GENERIC error. By categorizing this as non-GENERIC, we can allow the updates to continue while rejecting this specific error. - Karthik [1]: https://lore.kernel.org/all/YQXPR01MB3046197EF39296549EE6DD669A33A@YQXPR01MB3046.CANPRD01.PROD.OUTLOOK.COM/ (local) Signed-off-by: Karthik Nayak <redacted> --- Changes in v2: - This version fixes two more issues: - Fetching while locks already exist in the repository - D/F conflicts while fetching - Add a specific error to the first case, so we can nicely show a relevant error. Also check explicitly that the issue is due to case-insensitive filesystems. - Cleanup the commit messages. - Use `string_list_append_nodup()` with `strbuf_detach`, reducing the number of allocations. --- builtin/fetch.c | 21 ++++++++++-- refs.c | 10 +++++- refs.h | 2 ++ refs/files-backend.c | 51 ++++++++++++++++++++++++----- t/t1400-update-ref.sh | 53 +++++++++++++++++++++++++++++++ t/t5510-fetch.sh | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++- 6 files changed, 212 insertions(+), 13 deletions(-) Karthik Nayak (4): refs/files: catch conflicts on case-insensitive file-systems refs/files: use correct error type when lock exists refs/files: handle F/D conflicts in case-insensitive FS refs/files: handle D/F conflicts during locking Range-diff versus v1: 1: fe6e2c12e7 ! 1: bab864e28a refs/files: use correct error type when locking fails @@ Metadata Author: Karthik Nayak [off-list ref] ## Commit message ## - refs/files: use correct error type when locking fails + refs/files: catch conflicts on case-insensitive file-systems During the 'prepare' phase of 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 transactions - would be aborted if there are conflicting names such as: + 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, references updates would be applied serially with one transaction + 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: @@ Commit message refs/heads/foo: ec3053b0977e83d9b67fc32c4527a117953994f3 refs/heads/sample: 2eefd1150e06d8fca1ddfa684dec016f36bf4e56 - This is buggy behavior since the user is never intimated about the + 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 applying + 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'. Change this to return - 'REF_TRANSACTION_ERROR_CREATE_EXISTS' instead to aid the batched update - mechanism to simply reject such errors. + 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_CREATE_EXISTS', 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. + 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 + This should be an okay compromise since with the files backend, there is no scenario possible where we would retain all colliding references. - 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 - Let's also be more pro-active 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 [off-list ref] + Helped-by: Patrick Steinhardt [off-list ref] Signed-off-by: Karthik Nayak [off-list ref] ## builtin/fetch.c ## @@ builtin/fetch.c: static void ref_transaction_rejection_handler(const char *refna struct ref_rejection_data *data = cb_data; - if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) { -+ if (err == REF_TRANSACTION_ERROR_CREATE_EXISTS && ignore_case && ++ 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" @@ builtin/fetch.c: static void ref_transaction_rejection_handler(const char *refna const char *reason = ref_transaction_error_msg(err); + ## refs.c ## +@@ refs.c: 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"; + } + + ## refs.h ## +@@ refs.h: 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, + }; + + /* + ## refs/files-backend.c ## +@@ refs/files-backend.c: static void unlock_ref(struct ref_lock *lock) + } + } + ++static bool duplicate_reference_case_cmp(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, +@@ refs/files-backend.c: 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; @@ refs/files-backend.c: static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs, goto retry; } else { unable_to_lock_message(ref_file.buf, myerr, err); -+ if (myerr == EEXIST) -+ ret = REF_TRANSACTION_ERROR_CREATE_EXISTS; ++ if (myerr == EEXIST && ignore_case && ++ duplicate_reference_case_cmp(transaction, update)) ++ ret = REF_TRANSACTION_ERROR_CASE_CONFLICT; goto error_return; } } +@@ refs/files-backend.c: 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; + ## t/t1400-update-ref.sh ## @@ t/t1400-update-ref.sh: do @@ t/t1400-update-ref.sh: do + test_commit two && + head=$(git rev-parse HEAD) && + -+ format_command $type "create refs/heads/foo" "$head" >stdin && -+ format_command $type "create refs/heads/ref" "$old_head" >>stdin && -+ format_command $type "create refs/heads/Foo" "$old_head" >>stdin && ++ { ++ 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 && @@ t/t1400-update-ref.sh: do + echo $old_head >expect && + git rev-parse refs/heads/ref >actual && + test_cmp expect actual && -+ test_grep -q "reference already exists" stdout ++ git rev-parse refs/heads/Foo >actual && ++ test_cmp expect actual + ) + ' + -: ---------- > 2: b0ecf6f10d refs/files: use correct error type when lock exists 2: 30a2629ebc ! 3: 1842ddee90 refs/files: handle F/D conflicts in case-insensitive FS @@ Metadata ## Commit message ## refs/files: handle F/D conflicts in case-insensitive FS - Similar to the previous commit, when using the files-backend on - case-insensitive filesystems, there is possibility of hitting F/D - conflicts when creating references within a single transaction, such as: + When using the files-backend on case-insensitive filesystems, there is + possibility of hitting F/D conflicts when creating references within a + single transaction, such as: - 'refs/heads/foo' - 'refs/heads/Foo/bar' @@ Commit message which is responsible for checking F/D conflicts within a given transaction. This utility function is shared across the reference backends. As such, it doesn't consider the issues of using a - case-insensitive, which only affects the files-backend. + case-insensitive file system, which only affects the files-backend. - While one solution would be to make the function aware of such issues. - This feels like leaking implementation details of file-backend specific + While one solution would be to make the function aware of such issues, + this feels like leaking implementation details of file-backend specific issues into the utility function. So opt for the more simpler option, of lowercasing all references sent to this function when on a case-insensitive filesystem and operating on the files-backend. @@ refs/files-backend.c: static enum ref_transaction_error lock_raw_ref(struct file + strbuf_addstr(&lower, refname); + strbuf_tolower(&lower); + -+ item = string_list_append(refnames_to_check, lower.buf); -+ strbuf_release(&lower); ++ item = string_list_append_nodup(refnames_to_check, ++ strbuf_detach(&lower, NULL)); + } else { + item = string_list_append(refnames_to_check, refname); + } @@ t/t5510-fetch.sh: test_expect_success "clone and setup child repos" ' ) ' -@@ t/t5510-fetch.sh: test_expect_success CASE_INSENSITIVE_FS,REFFILES 'existing references in a case +@@ t/t5510-fetch.sh: test_expect_success REFFILES 'existing reference lock in repo' ' ) ' -: ---------- > 4: 81759b0e51 refs/files: handle D/F conflicts during locking base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0 change-id: 20250822-587-git-fetch-1-fails-fetches-on-case-insensitive-repositories-0a187c7ac41e Thanks - Karthik