[bug] pull --prune could not delete references due to lock file already exists error

[bug] pull --prune could not delete references due to lock file already exists error

[ryenus]

today I tried to prune the remote refs in a local repo, but `git pull --prune`
failed with an error saying it could not delete references. I retried several
times and it failed consistently, without a single ref pruned.

I found this interesting comment while looking into the code here:
https://github.com/git/git/blob/bd99d6e8db5e2c56dd24395e9711ee7ee564bf4f/refs.c#L2863-L2895

> /*
>  * Since we don't check the references' old_oids, the
>  * individual updates can't fail, so we can pack all of the
>  * updates into a single transaction.
>  */

The problem is the assume is wrong, coz things could fail, one such case
is to have two refs like below:

1. origin/TOM/b1
2. origin/TOM/b2
3. origin/tom/b2

Notice there's `TOM` vs `tom` in the path. Another factor is that my local file
system is case insensitive. As a result the 3rd ref would fail and cause the
transaction to be always rolled back.

Re [bug] pull --prune could not delete references due to lock file already exists error

[K Jayatheerth]

First off thanks for reporting the bug :)
So I cannot test this bug as my files system is case sensitive
but to just read the code and give a thought in a direction (Assuming that the bug is recreatable)

int refs_delete_refs(struct ref_store *refs, const char *logmsg,
		     struct string_list *refnames, unsigned int flags)
{
...
	struct ref_transaction *transaction;
...
	for_each_string_list_item(item, refnames) {

		transaction = ref_store_transaction_begin(refs, 0, &err);
		if (!transaction) {
			warning(_("could not begin transaction to delete %s: %s"),
				item->string, err.buf);
			strbuf_reset(&err);
			failures = 1;
			continue;
		}

		ret = ref_transaction_delete(transaction, item->string,
					     NULL, NULL, flags, msg, &err);
		if (ret) {
			warning(_("could not delete reference %s: %s"),
				item->string, err.buf);
			strbuf_reset(&err);
			ref_transaction_free(transaction);
			failures = 1;
			continue;
		}

		ret = ref_transaction_commit(transaction, &err);
		if (ret) {
			warning(_("could not commit deletion of %s: %s"),
				item->string, err.buf);
			strbuf_reset(&err);
			failures = 1;
		}

		ref_transaction_free(transaction);
	}

	if (failures)
		ret = -1;

	strbuf_release(&err);
	free(msg);
	return ret;
}

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?

Thank you 

- Jayatheerth

Re: Re [bug] pull --prune could not delete references due to lock file already exists error

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]

On 2025-06-25 at 14:18:49, K Jayatheerth wrote:
> First off thanks for reporting the bug :)
> So I cannot test this bug as my files system is case sensitive
> but to just read the code and give a thought in a direction (Assuming that the bug is recreatable)

Just so you know, on Linux, you can create a case-insensitive JFS
partition on a loopback device and on macOS, you can create a
case-insensitive APFS or HFS partition in a disk image file that can
then be mounted (I think using `hdiutil` or the directions at [0]).

I have used the former in the rare occasion that I need to test a
case-insensitive file system.

[0] https://support.apple.com/en-ca/guide/disk-utility/dskutl11888/mac
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: Re [bug] pull --prune could not delete references due to lock file already exists error

[JAYATHEERTH K]

On Sat, Jun 28, 2025 at 12:29 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2025-06-25 at 14:18:49, K Jayatheerth wrote:
> > First off thanks for reporting the bug :)
> > So I cannot test this bug as my files system is case sensitive
> > but to just read the code and give a thought in a direction (Assuming that the bug is recreatable)
>
> Just so you know, on Linux, you can create a case-insensitive JFS
> partition on a loopback device and on macOS, you can create a
> case-insensitive APFS or HFS partition in a disk image file that can
> then be mounted (I think using `hdiutil` or the directions at [0]).
>
> I have used the former in the rare occasion that I need to test a
> case-insensitive file system.
>

Thanks Brian, that’s really helpful! I’ll try setting up a loopback filesystem
and use it for testing similar bugs in the future.
Appreciate the tip!

- Jayatheerth

Re: Re [bug] pull --prune could not delete references due to lock file already exists error

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3177 bytes --]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: Re [bug] pull --prune could not delete references due to lock file already exists error

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]

On 2025-06-30 at 13:46:35, Karthik Nayak wrote:
> 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;
> +			}

This won't work in the general case, since the two refs that match case
insensitively aren't guaranteed to be adjacent.  For instance:

    refs/heads/AAAA
    refs/heads/AAAB
    refs/heads/aaaa
    refs/heads/aaab

They'll be in the above order for a bytewise comparison, but the
matching entries won't be adjacent in the list.

Another option is for users on case-insensitive systems to use reftable,
which won't have the same problems as the file-based backend and will
preserve case properly.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: Re [bug] pull --prune could not delete references due to lock file already exists error

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> Another option is for users on case-insensitive systems to use reftable,
> which won't have the same problems as the file-based backend and will
> preserve case properly.

The more guinea-pigs^Wadopters we have, the merrier we are ;-).

Re: Re [bug] pull --prune could not delete references due to lock file already exists error

[Patrick Steinhardt]

On Mon, Jun 30, 2025 at 02:10:36PM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > Another option is for users on case-insensitive systems to use reftable,
> > which won't have the same problems as the file-based backend and will
> > preserve case properly.
> 
> The more guinea-pigs^Wadopters we have, the merrier we are ;-).

I'd really like to start thinking about reftables as the default
backend. They fix filesystem-specific issues, compress better, are more
efficient in most (not all) use cases, sometimes significantly so, have
better properties when it comes to repository maintenance. We could for
example make "features.experimental" enable the reftable backend by
default and add a note to the Git 3.0 breaking changes in that spirit.

I'm quite certain that reftables are stable enough to not cause any
problems as used via Git. We have it rolled out to a couple hundred
thousand of repositories on our staging systems at GitLab, and will soon
roll them out to production systems. I myself have been running with
reftables for 1.5 years now and haven't seen a bug after the first
couple months.

But the bigger issue unfortunately is the ecosystem. JGit supports
reftables, but other implementations like libgit2 and Gitoxide don't. I
partially got myself to blame here, because I haven't gotten around to
updating the libgit2 pull request and am lacking the time to finish that
one. So it's probably a non-starter if all tools that use those libraries
cannot access such reftable-enabled Git repositories at all anymore.

I bet there's also tons of scripts out there that just reach into the
filesystem to do stuff, but that's something that we cannot really help
with.

Too bad :/

Patrick

Re: Re [bug] pull --prune could not delete references due to lock file already exists error

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> I'd really like to start thinking about reftables as the default
> backend. They fix filesystem-specific issues, compress better, are more
> efficient in most (not all) use cases, sometimes significantly so, have
> better properties when it comes to repository maintenance. We could for
> example make "features.experimental" enable the reftable backend by
> default and add a note to the Git 3.0 breaking changes in that spirit.

I am a bit surprised that there is no reference to reftable in the
breaking changes document ;-)

I am all for allowing users to opt into it dynamically, and
feature.experimental is the ideal mechanism to do so.  Currently it
sets four configuration variables, and making it five would not make
it too scary for adventurous users.

> I bet there's also tons of scripts out there that just reach into the
> filesystem to do stuff, but that's something that we cannot really help
> with.

I thought you've done enough to make sure that common things people
would want to do by direct access to the .git/refs/ hierarchy can be
easily done with plumbing commands instead, so it would probably be
a matter of writing and publicizing the "how to migrate to the world
where you cannot write into files under .git/refs/ directory"
document?

Thanks.

Re: Re [bug] pull --prune could not delete references due to lock file already exists error

[Patrick Steinhardt]

On Tue, Jul 01, 2025 at 09:14:19AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > I bet there's also tons of scripts out there that just reach into the
> > filesystem to do stuff, but that's something that we cannot really help
> > with.
> 
> I thought you've done enough to make sure that common things people
> would want to do by direct access to the .git/refs/ hierarchy can be
> easily done with plumbing commands instead, so it would probably be
> a matter of writing and publicizing the "how to migrate to the world
> where you cannot write into files under .git/refs/ directory"
> document?

Yeah, all the tools are there. I'm not even sure whether such a guide
would be required in the first place -- tools are just what you are used
to already anyway. So I don't know whether I really need to explain how
to use git-update-ref(1) and friends.

I am of course biased here, so if disagree and think that this would be
required I might just do it.

Anyway, I'll hack something up and send it to the mailing list soonish
to get the discussion going.

Patrick

Re: Re [bug] pull --prune could not delete references due to lock file already exists error

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2025-06-30 at 13:46:35, Karthik Nayak wrote:
>> 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;
>> +			}
>
> This won't work in the general case, since the two refs that match case
> insensitively aren't guaranteed to be adjacent.  For instance:
>
>     refs/heads/AAAA
>     refs/heads/AAAB
>     refs/heads/aaaa
>     refs/heads/aaab
>
> They'll be in the above order for a bytewise comparison, but the
> matching entries won't be adjacent in the list.
>

Indeed. Good catch, this wouldn't work! I guess you'd have to use a
hashmap in that case, I think that is what Jayatheerth also mentioned in
his reply.

> Another option is for users on case-insensitive systems to use reftable,
> which won't have the same problems as the file-based backend and will
> preserve case properly.

Yes, that's always recommended! But until we start marking the files
backend as deprecated, we do have to plug this bug.

> --
> brian m. carlson (they/them)
> Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: Re [bug] pull --prune could not delete references due to lock file already exists error

[Chris Torek]

On Mon, Jun 30, 2025 at 7:21 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
[regarding]
> > +                             if (ignore_case && prev && !strcasecmp(ref->next, prev))
> This won't work in the general case, since the two refs that match case
> insensitively aren't guaranteed to be adjacent.

Also worth mention: it's not just case-folding that matters.

On OS X (Macs), path names get "normalized" so that the names

   s c h combining-umlaut o n

and

   s c h umlaut-o n

refer to the *same* file or directory. On a typical Linux/Unix FS, they differ.

(I don't know what Windows does!)

So, if you have a "folder-full" of "pretty" German refnames, some
spelled one way and some another, well...

(It's not clear to me what, if anything, Git should attempt to do here.)

Chris

Re: Re [bug] pull --prune could not delete references due to lock file already exists error

[Junio C Hamano]

Chris Torek <chris.torek@gmail.com> writes:

> On Mon, Jun 30, 2025 at 7:21 AM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> [regarding]
>> > +                             if (ignore_case && prev && !strcasecmp(ref->next, prev))
>> This won't work in the general case, since the two refs that match case
>> insensitively aren't guaranteed to be adjacent.
>
> Also worth mention: it's not just case-folding that matters.
>
> On OS X (Macs), path names get "normalized" so that the names
>
>    s c h combining-umlaut o n
>
> and
>
>    s c h umlaut-o n
>
> refer to the *same* file or directory. On a typical Linux/Unix FS, they differ.
>
> (I don't know what Windows does!)
>
> So, if you have a "folder-full" of "pretty" German refnames, some
> spelled one way and some another, well...
>
> (It's not clear to me what, if anything, Git should attempt to do here.)

The system supplied argv[] is fed to the precompose_argv_prefix()
helper in compat/precompose_utf8.c; opendir/readdir/closedir are
also wrapped with similar NFD/NFC normalization (really, UTF-8-MAC
vs UTF-8) helpers defined in the same file.  So the path you read
(via the opendir/readdir like dir.c does) from the system, or the
path you are fed from the command line (via argv[]), are normalized
before code in Git above the compat layer even sees them.  The path
recoreded in various mechanisms in Git like the index and the tree
objects are all normalized.

Re: Re [bug] pull --prune could not delete references due to lock file already exists error

[JAYATHEERTH K]

On Mon, Jun 30, 2025 at 7:16 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>
> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> 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.