On Mon, Jun 30, 2025 at 7:16 PM Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>
> K Jayatheerth <jayatheerthkulkarni2005@xxxxxxxxx> writes:
>
> [snip]
>
> > The original implementation:
> > Starts a single transaction using ref_store_transaction_begin().
> > Adds all deletions to that transaction.
> > Commits the transaction.
> > If any deletion fails, the entire transaction is aborted.
> > On case-insensitive file systems, two refs like:
> > may conflict at the file system level (e.g. both mapped to the same file or directory).
> > If Git tries to delete both in one go, the transaction fails due to a lock file or unlink error.
> > (Above are my assumptions till now).
> >
> > What has changed is:
> > Deletes each reference in its own transaction
> > struct ref_transaction *transaction = ref_store_transaction_begin(...);
> > ref_transaction_delete(transaction, ...);
> > ref_transaction_commit(transaction, ...);
> > ref_transaction_free(transaction);
> > If one deletion fails due to a case conflict, the others still proceed.
> > It avoids rolling back the entire prune operation just because of a single failure.
> > Keeps failure count and returns appropriately
> > Signals that something went wrong, but Git can now give partial success feedback.
> >
> >
> > The question I have is
> > If this approach seems viable or perhaps any solution,
> > would it be possible to write a test case for this scenario?
> >
>
> You analysis is right. With 'kn/fetch-push-bulk-ref-update' in the works
> (possibly be merged to next soon), we will start using batched updates
> in git-fetch(1) too. Batched updates allow individual updates to fail,
> while allowing the transaction as a whole to succeed.
>
> Unfortunately, because our transaction mechanism doesn't handle
> conflicts, we separate out pruning as a pre-step. So this bug would
> still be present there.
>
> The issue with the fix you're suggesting is a huge performance drop,
> since creating individual transaction for each deletion has a lot of
> overhead and the reftable backend would perform a lot worse in such
> situations.
>
> I can see few solutions overall (including the one you suggested).
>
> One solution is to drop duplicates in case insensitive systems, this is
> the shortest and easiest fix for now.
>
> Perhaps something like (untested back of the hand code):
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index cc0a3deb61..bc79d74b82 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1352,10 +1352,16 @@ static int prune_refs(struct display_state
> *display_state,
> goto cleanup;
> }
> } else {
> + const char *prev;
> struct string_list refnames = STRING_LIST_INIT_NODUP;
>
> - for (ref = stale_refs; ref; ref = ref->next)
> + for (ref = stale_refs; ref; ref = ref->next) {
> + if (ignore_case && prev && !strcasecmp(ref->next, prev))
> + continue;
> +
> string_list_append(&refnames, ref->name);
> + prev = ref->name;
> + }
>
> result = refs_delete_refs(get_main_ref_store(the_repository),
> "fetch: prune", &refnames,
>
>
> A bigger and eventual goal is to simply introduce conflict resolution in
> reference transactions. This would allow us to use batched transaction
> together for pruning and updating of refs, and using batched
> transactions would ensure that single reference changes can fail without
> failing the entire batch.
>
> - Karthik
Agreed that was a lazy fix.
I had thought it(multiple transactions) was not a viable solution.
Something I find interesting is
Loop through the refs marked for deletion (stale_refs)
If the filesystem is case-insensitive
Convert the ref name to lowercase
Check if this lowercase version was already seen
If it's not a duplicate, store the lowercase name
Add the actual (original-cased) ref to the refnames list
Free up the memory used in seen_refs map
---
struct strmap seen_refs = STRMAP_INIT;
...
for (ref = stale_refs; ref; ref = ref->next) {
if (ignore_case) {
char *lower = xstrdup(ref->name);
str_tolower(lower);
if (strmap_contains(&seen_refs, lower)) {
/* Skip this ref it would collide on
case-insensitive FS */
warning(_("Skipping deletion of '%s' due to
case-insensitive conflict"), ref->name);
free(lower);
continue;
}
/* Keep the lowercased key in the map to catch future
conflicts */
strmap_put(&seen_refs, lower, (void *)1);
}
string_list_append(&refnames, ref->name);
}
strmap_clear(&seen_refs, 1);
---
warning(_("Skipping deletion of '%s' due to case-insensitive
conflict"), ref->name);
When we find duplicates it should work fine I guess.
Let me know if this direction makes sense or if you'd prefer a
different handling strategy,
happy to start sending patches.
