[PATCH] files_initial_transaction_commit(): only unlock if locked

[PATCH] files_initial_transaction_commit(): only unlock if locked

[Mathias Rav]

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(-)

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

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

[Jeff King]

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.

-Peff

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

[Junio C Hamano]

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.

Thanks.

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

[Michael Haggerty]

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?

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.

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

[Mathias Rav]

2018-01-22 10:25 +0100 Michael Haggerty <mhagger@alum.mit.edu>:
> 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