On Mon, Jan 06, 2014 at 12:17:21PM -0800, Junio C Hamano wrote:
> > I am fine with rejecting it with a warning, but we should not then
> > complain that the other side did not send us the object, since we should
> > not be asking for it at all. I also do not see us complaining about the
> > funny ref anywhere. So there is definitely _a_ bug here. :)
>
> Oh, no question about that. I was just pointing somebody who
> already has volunteered to take a look in a direction I recall was
> where the issue was ;-)
Oh, crud, did I volunteer? :)
So I found the problem, but I'm really not sure what to make of it. We
do check the refname format when evaluating the refspecs, in:
do_fetch
get_ref_map
get_fetch_map
check_refname_format
Before calling it, we check that it starts with "refs/", and then pass
the _whole_ refname into check_refname_format. So the latter sees
"refs/stash". And that's acceptable, as it's not a single-level ref.
Thus we do not get the "funny ref" message.
The code looks like this:
if (!starts_with((*rmp)->peer_ref->name, "refs/") ||
check_refname_format((*rmp)->peer_ref->name, 0)) {
/* print funny ref and ignore */
Then we ask fetch_refs_via_pack to get the actual objects for us. And
it checks our refs again, with this call chain:
do_fetch
fetch_refs
transport_fetch_refs
fetch_refs_via_pack
fetch_pack
do_fetch_pack
everything_local
filter_refs
check_refname_format
Here, the code looks like this:
if (!memcmp(ref->name, "refs/", 5) &&
check_refname_format(ref->name + 5, 0))
; /* trash */
At first I thought we are doing the same check (is it syntactically
valid, and is it in "refs/"), but we're not. We are actually checking
the format _only_ of stuff in "refs/", and ignoring the rest. Which
really makes no sense to me.
If it were "memcmp(...) || check_refname_format()", then it would be
roughly the same check. But it would still be wrong, because note that
we pass only the bits under "refs/" to check_refname_format. So it sees
only "stash", and then complains that it is single-level.
So the symptom we are seeing is because we are filtering with two
different rulesets in different places. But I'm really not sure how to
resolve it. The one in filter_refs seems nonsensical to me.
Checking _only_ things under refs/ doesn't make sense. And even if that
was sensible, feeding half a ref to check_refname_format does not work.
In addition to the single-level check, it has other rules that want
to see the whole ref (e.g., the ref "@" is not valid, but "refs/@" is
OK; it cannot distinguish them without seeing the prefix).
So I can see two options:
1. Make the check in filter_refs look like the one in get_fetch_map.
That at least makes them the same, which alleviates the symptom.
But we still are running two checks, and if they ever get out of
sync, it causes problems.
2. Abolish the check in filter_refs. I think this makes the most sense
from the perspective of fetch, because we will already have cleaned up
the ref list. But we might need to leave the filtering in place for
people who call fetch-pack as a bare plumbing command.
It's really not clear to me what the check in filter_refs was trying to
do. It dates all the way back to 1baaae5 (Make maximal use of the remote
refs, 2005-10-28), but there is not much explanation. I haven't dug into
the list around that time to see if there's any discussion.
-Peff
--
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