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
Attachment:
signature.asc
Description: PGP signature
