threads / patch / 47632

patchfiles_initial_transaction_commit(): only unlock if locked

Subject: [PATCH] files_initial_transaction_commit(): only unlock if locked

## tl;dr

5 messages between Jan 18, 2018 and Jan 22, 2018. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

Mathias Rav· Jan 18, 2018, 13:38 UTC · lore

Running git clone --single-branch --mirror -b TAGNAME previously triggered the following error message:

	fatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.
This error condition is handled in files_initial_transaction_commit().

42c7f7ff9 ("commit_packed_refs(): remove call to `packed_refs_unlock()`", 2017-06-23) introduced incorrect unlocking in the error path of this function, which changes the error message to

	fatal: BUG: packed_refs_unlock() called when not locked

Move the call to packed_refs_unlock() above the "cleanup:" label since the unlocking should only be done in the last error path.

Signed-off-by: Mathias Rav <m@git.strova.dk>
---
 refs/files-backend.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Show changes to refs/files-backend.c +1 −2
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a80d60aa0..afe5c4e94 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2874,13 +2874,12 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 
 	if (initial_ref_transaction_commit(packed_transaction, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
-		goto cleanup;
 	}
 
+	packed_refs_unlock(refs->packed_ref_store);
 cleanup:
 	if (packed_transaction)
 		ref_transaction_free(packed_transaction);
-	packed_refs_unlock(refs->packed_ref_store);
 	transaction->state = REF_TRANSACTION_CLOSED;
 	string_list_clear(&affected_refnames, 0);
 	return ret;

-- 
2.15.1
Jeff King· Jan 18, 2018, 14:19 UTC · re: Mathias Rav · lore

Re: [PATCH] files_initial_transaction_commit(): only unlock if locked

On Thu, Jan 18, 2018 at 02:38:41PM +0100, Mathias Rav wrote:
Show 15 quoted lines
> Running git clone --single-branch --mirror -b TAGNAME previously
> triggered the following error message:
> 
> 	fatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.
> 
> This error condition is handled in files_initial_transaction_commit().
> 
> 42c7f7ff9 ("commit_packed_refs(): remove call to `packed_refs_unlock()`", 2017-06-23)
> introduced incorrect unlocking in the error path of this function,
> which changes the error message to
> 
> 	fatal: BUG: packed_refs_unlock() called when not locked
> 
> Move the call to packed_refs_unlock() above the "cleanup:" label
> since the unlocking should only be done in the last error path.

Thanks, this solution looks correct to me. It's pretty low-impact since the locking is the second-to-last thing in the function, so we don't have to re-add the unlock to a bunch of error code paths. But one alternative would be to just do:

  if (packed_refs_is_locked(refs))
	packed_refs_unlock(refs->packed_ref_store);
in the cleanup section.
-Peff
Junio C Hamano· Jan 19, 2018, 22:14 UTC · re: Jeff King · lore

Re: [PATCH] files_initial_transaction_commit(): only unlock if locked

Jeff King <peff@peff.net> writes:
Show 27 quoted lines
> On Thu, Jan 18, 2018 at 02:38:41PM +0100, Mathias Rav wrote:
>
>> Running git clone --single-branch --mirror -b TAGNAME previously
>> triggered the following error message:
>> 
>> 	fatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.
>> 
>> This error condition is handled in files_initial_transaction_commit().
>> 
>> 42c7f7ff9 ("commit_packed_refs(): remove call to `packed_refs_unlock()`", 2017-06-23)
>> introduced incorrect unlocking in the error path of this function,
>> which changes the error message to
>> 
>> 	fatal: BUG: packed_refs_unlock() called when not locked
>> 
>> Move the call to packed_refs_unlock() above the "cleanup:" label
>> since the unlocking should only be done in the last error path.
>
> Thanks, this solution looks correct to me. It's pretty low-impact since
> the locking is the second-to-last thing in the function, so we don't
> have to re-add the unlock to a bunch of error code paths. But one
> alternative would be to just do:
>
>   if (packed_refs_is_locked(refs))
> 	packed_refs_unlock(refs->packed_ref_store);
>
> in the cleanup section.

Yeah, that may be a more future-proof alternative, and just as you said the patch as posted would be sufficient, too.

Thanks.
Michael Haggerty· Jan 22, 2018, 09:25 UTC · re: Junio C Hamano · lore

Re: [PATCH] files_initial_transaction_commit(): only unlock if locked

On 01/19/2018 11:14 PM, Junio C Hamano wrote:
Show 32 quoted lines
> Jeff King <peff@peff.net> writes:
> 
>> On Thu, Jan 18, 2018 at 02:38:41PM +0100, Mathias Rav wrote:
>>
>>> Running git clone --single-branch --mirror -b TAGNAME previously
>>> triggered the following error message:
>>>
>>> 	fatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.
>>>
>>> This error condition is handled in files_initial_transaction_commit().
>>>
>>> 42c7f7ff9 ("commit_packed_refs(): remove call to `packed_refs_unlock()`", 2017-06-23)
>>> introduced incorrect unlocking in the error path of this function,
>>> which changes the error message to
>>>
>>> 	fatal: BUG: packed_refs_unlock() called when not locked
>>>
>>> Move the call to packed_refs_unlock() above the "cleanup:" label
>>> since the unlocking should only be done in the last error path.
>>
>> Thanks, this solution looks correct to me. It's pretty low-impact since
>> the locking is the second-to-last thing in the function, so we don't
>> have to re-add the unlock to a bunch of error code paths. But one
>> alternative would be to just do:
>>
>>   if (packed_refs_is_locked(refs))
>> 	packed_refs_unlock(refs->packed_ref_store);
>>
>> in the cleanup section.
> 
> Yeah, that may be a more future-proof alternative, and just as you
> said the patch as posted would be sufficient, too.
Either solution LGTM. Thanks for finding and fixing this bug.
But let's also take a step back. The invocation
    git clone --single-branch --mirror -b TAGNAME

seems curious. Does it even make sense to use `--mirror` and `--single-branch` at the same time? What should it do?

Normally `--mirror` implies (aside from `--bare`) that the remote references should be converted 1:1 to local references and should be overwritten at every fetch; i.e., the refspec should be set to `+refs/*:refs/*`.

To me the most plausible interpretation of `--mirror --single-branch -b BRANCHNAME` would be that the single branch should be fetched and made the HEAD, and the refspec should be set to `+refs/heads/BRANCHNAME:refs/heads/BRANCHNAME`. It also wouldn't be very surprising if it were forbidden to use these options together.

Currently, we do neither of those things. Instead we fetch that one reference (as `refs/heads/BRANCHNAME`) but set the refspec to `+refs/*:refs/*`; i.e., the next fetch would fetch all of the history.

It's even more mind-bending if `-b` is passed a `TAGNAME` rather than a `BRANCHNAME`. The documentation says that `-b TAGNAME` "detaches the HEAD at that commit in the resulting repository". If `--single-branch -b TAGNAME` is used, then the refspec is set to `+refs/tags/TAGNAME:refs/tags/TAGNAME`. But what if `--mirror` is also used?

Currently, this fails, apparently because `--mirror` and `-b TAGNAME` each independently try to set `refs/tags/TAGNAME` (presumably to the same value). *If* this is a useful use case, we could fix it so that it doesn't fail. If not, maybe we should prohibit it explicitly and emit a clearer error message.

Mathias: if you encountered this problem in the real world, what were
you trying to accomplish? What behavior would you have expected?

Maybe the behavior could be made more sane if there were a way to get the 1:1 reference mapping that `--mirror` implies without also getting `--bare` [1]. Suppose there were a `--refspec` option. Then instead of

    git clone --mirror --single-branch -b BRANCHNAME

with it's non-obvious semantics, you could prohibit that use and instead support

    git clone --bare
--refspec='+refs/heads/BRANCHNAME:refs/heads/BRANCHNAME'

which seems clearer in its intent, if perhaps not super obvious. Or you could give `clone` a `--no-fetch` option, which would give the user a time to intervene between setting up the basic clone config and actually fetching objects.

Michael
[1] It seems like
        git clone --config remote.origin.fetch='+refs/*:refs/*' clone ...
    might do it, but that actually ends up setting up two refspecs and
only honoring `+refs/heads/*:refs/remotes/origin/*` for the initial
fetch. Plus it is pretty obscure.
Mathias Rav· Jan 22, 2018, 10:03 UTC · re: Michael Haggerty · lore

Re: [PATCH] files_initial_transaction_commit(): only unlock if locked

2018-01-22 10:25 +0100 Michael Haggerty <mhagger@alum.mit.edu>:
Show 72 quoted lines
> On 01/19/2018 11:14 PM, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:
> >   
> >> On Thu, Jan 18, 2018 at 02:38:41PM +0100, Mathias Rav wrote:
> >>  
> >>> Running git clone --single-branch --mirror -b TAGNAME previously
> >>> triggered the following error message:
> >>>
> >>> 	fatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.
> >>>
> >>> This error condition is handled in files_initial_transaction_commit().
> >>>
> >>> 42c7f7ff9 ("commit_packed_refs(): remove call to `packed_refs_unlock()`", 2017-06-23)
> >>> introduced incorrect unlocking in the error path of this function,
> >>> which changes the error message to
> >>>
> >>> 	fatal: BUG: packed_refs_unlock() called when not locked
> >>>
> >>> Move the call to packed_refs_unlock() above the "cleanup:" label
> >>> since the unlocking should only be done in the last error path.  
> >>
> >> Thanks, this solution looks correct to me. It's pretty low-impact since
> >> the locking is the second-to-last thing in the function, so we don't
> >> have to re-add the unlock to a bunch of error code paths. But one
> >> alternative would be to just do:
> >>
> >>   if (packed_refs_is_locked(refs))
> >> 	packed_refs_unlock(refs->packed_ref_store);
> >>
> >> in the cleanup section.  
> > 
> > Yeah, that may be a more future-proof alternative, and just as you
> > said the patch as posted would be sufficient, too.  
> 
> Either solution LGTM. Thanks for finding and fixing this bug.
> 
> But let's also take a step back. The invocation
> 
>     git clone --single-branch --mirror -b TAGNAME
> 
> seems curious. Does it even make sense to use `--mirror` and
> `--single-branch` at the same time? What should it do?
> 
> Normally `--mirror` implies (aside from `--bare`) that the remote
> references should be converted 1:1 to local references and should be
> overwritten at every fetch; i.e., the refspec should be set to
> `+refs/*:refs/*`.
> 
> To me the most plausible interpretation of `--mirror --single-branch -b
> BRANCHNAME` would be that the single branch should be fetched and made
> the HEAD, and the refspec should be set to
> `+refs/heads/BRANCHNAME:refs/heads/BRANCHNAME`. It also wouldn't be very
> surprising if it were forbidden to use these options together.
> 
> Currently, we do neither of those things. Instead we fetch that one
> reference (as `refs/heads/BRANCHNAME`) but set the refspec to
> `+refs/*:refs/*`; i.e., the next fetch would fetch all of the history.
> 
> It's even more mind-bending if `-b` is passed a `TAGNAME` rather than a
> `BRANCHNAME`. The documentation says that `-b TAGNAME` "detaches the
> HEAD at that commit in the resulting repository". If `--single-branch -b
> TAGNAME` is used, then the refspec is set to
> `+refs/tags/TAGNAME:refs/tags/TAGNAME`. But what if `--mirror` is also used?
> 
> Currently, this fails, apparently because `--mirror` and `-b TAGNAME`
> each independently try to set `refs/tags/TAGNAME` (presumably to the
> same value). *If* this is a useful use case, we could fix it so that it
> doesn't fail. If not, maybe we should prohibit it explicitly and emit a
> clearer error message.
> 
> Mathias: if you encountered this problem in the real world, what were
> you trying to accomplish? What behavior would you have expected?

I wanted a shallow, single-commit clone of a single tag into a bare repo. Concretely, I wanted to change the Arch Linux build script for linux-tools to make a shallow clone of the Linux kernel rather than an ordinary clone, for a tagname corresponding to a released kernel version. (Of course, it would have been better to change the build script to download a tarball instead of using git, but without knowledge of the build script it seemed easier for me to just change the git invocation.)

Currently, Arch Linux's build script uses `git clone --mirror "$url" "$dir"` to clone a remote repo; a StackExchange post [1] suggested changing this to `git clone --mirror --depth 1 "$url" "$dir"`. (The post also adds `--single-branch`, but this is implied by `--depth`.)

[1] https://unix.stackexchange.com/a/203335/220010

Naively I added `-b TAGNAME` to fetch just a single tag, but this resulted in the error.

If I remove `--mirror`, i.e. invoke `git clone --depth 1 -b TAGNAME`, then the refspec is `+refs/tags/TAGNAME:refs/tags/TAGNAME` as might be expected, but this results in a non-bare repo.

If instead I change `--mirror` to `--bare`, i.e. invoke `git clone --bare --depth 1 -b TAGNAME`, this results in a cloned repository with no `remote.origin.fetch` refspec at all. I would expect the refspec in this case to be set to `+refs/tags/TAGNAME:refs/tags/TAGNAME` (just as with `--mirror`).

/Mathias

← back to recent threads