On 05/02/2012 12:19 AM, Junio C Hamano wrote:
This ought to work:
$ git checkout HEAD^0 ;# make sure we are on detached HEAD
$ git fetch $somewhere +refs/*:refs/*
if you want to pull down all the branches from somewhere, potentially
nuking the refs we currently have.
However, if $somewhere has a stash, i.e. refs/stash, even though our
"fetch" sees it in the "ls-remote $somewhere" output and tries to make
sure that the object comes over the wire, we never show "want" line for
refs/stash, because we silently drop it with check_ref_format().
As you obviously know (but for the information of other readers), the
reason for the failure is that "stash" (not "refs/stash") is passed to
check_ref_format(). "stash" is not a valid refname because it has only
a single level (i.e., does not contain a '/'). check_ref_format() would
accept the refname if it were passed the REFNAME_ALLOW_ONELEVEL option,
but passing it the full refname (as your patch does) is better.
I have run out concentration before digging this through, but the attached
single liner patch fixes it. The thing is, this function is given a list
of refs and drops refs/stash (which is not what I want in the context of
the above +refs/*:refs/*), and modifies the caller's list of refs, and
then the caller (i.e. do_fetch_pack() that called everything_local()) then
uses updated list (i.e. without refs/stash) to run find_common() and
get_pack(), but the layer higher above (namely, do_fetch() that calls
fetch_refs() that in turn calls store_updated_refs(), all in
builtin/fetch.c) is _not_ aware of this filtering and expects that the
code at this layer *must* have asked for and obtained all the objects
reachable from what was listed in ls-remote output, leading to an
inconsistent state.
[Michael CC'ed as he seems to be dead set tightening check_ref_format()]
The specific failure of "refs/stash" is fixed with this patch, but I think
this call to check_ref_format() in the filter_refs() should be removed.
The function is trying to see what we _asked_ against what the remote side
said they have, and if we tried to recover objects from a broken remote by
doing:
git fetch $somewhere refs/heads/a..b:refs/heads/a_dot_dot_b
>
and the remote side advertised that it has such a ref (note that a..b is
an illegal path component), we should be able to do so without a misguided
call to check_refname_format() getting in the way.
I agree; if the remote reference name never gets used as a local
reference name, there is no reason to call check_ref_format() on it at
all. The local reference name that is derived from the remote reference
name (even if it is derived via an identity operation) *should* be
checked using check_ref_format(); presumably that is already the case?
Optimally the local refnames should be checked *before* the transfer to
avoid wasting the user's time.
It is the same story if the name advertised by a broken remote were
"refs/head/../heads/master". As long as the RHS of the refspec
(i.e. refs/heads/a_dot_dot_b) is kosher, we must allow such a request to
succeed, so that people can interoperate in less than perfect world.
A slightly more awkward question is if the broken remote advertises many
references including "refs/head/../heads/master" and we fetch refspec
"+refs/*:refs/*". In this case it is pretty clear that we should fetch
all of the valid references; otherwise, working around the problem would
be quite awkward. But what to do about "refs/head/../heads/master"? I
think we should emit a warning naming the reference that will not be
fetched "because it is formatted incorrectly", and not include it in the
"want" lines. If the user really wants the reference, he can fetch it
to another name using an explicit "from:to" refspec.
Michael
--
Michael Haggerty
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html