On Mon, Jun 06, 2011 at 01:31:54PM -0700, Junio C Hamano wrote:
> > static struct ref *wanted_peer_refs(const struct ref *refs,
> > struct refspec *refspec)
> > {
> > - struct ref *local_refs = NULL;
> > - struct ref **tail = &local_refs;
> > + struct ref *head = get_remote_ref(refs, "HEAD");
>
> The rest of the patch looked quite sane but I wonder if this should be
> using get_remote_ref() that calls find_ref_by_name_abbrev() which in turn
> would hit "refs/heads/HEAD" if the remote side didn't give you "HEAD".
> Shouldn't it be using find_ref_by_name() directly?
Ick, yeah, that was just me blindly cutting down what get_fetch_map was
doing to the bit that I wanted, and thinking get_remote_ref was it. I
didn't even notice the fact that it was using the _abbrev form of
find_ref_by_name.
It should definitely be an exact match. I'll fix it in my re-roll.
> > @@ -357,8 +358,11 @@ static void write_remote_refs(const struct ref *local_refs)
> > {
> > const struct ref *r;
> >
> > - for (r = local_refs; r; r = r->next)
> > + for (r = local_refs; r; r = r->next) {
> > + if (!r->peer_ref)
> > + continue;
>
> As this is part of this patch, I presume this test reliably catch "HEAD"
> and only "HEAD", but what is it that gives us this guarantee? Is it that
> in all three possible configurations (i.e. traditional no-separate remote
> layout, separate remote layout, or mirrored layout), we never map anything
> outside refs/heads/* and refs/tags/* namespace, hence things like HEAD
> will never have peer_ref defined?
Until now, nobody fed us a ref without peer_ref mapped, as doing so
would segfault. So this gives a NULL peer ref the meaning "don't bother
saving this into a ref". Which is really the only sensible thing it can
mean, since the peer ref is the local bit that says where to put it.
> This is not a complaint but is an honest question. I am wondering how
> future possible enhancements to "clone" (like the rumored "track only this
> branch") will affect codepaths around this area.
I think it's a good question to ask. The answer is that if you are
cloning only a subset of the remote refs (or even a single ref), then
either:
1. You will want to write that subset according to a refspec mapping
(e.g., cloning "jk/*" to "remotes/origin/jk/*"). In that case you
would have set the peer_ref to show where to put it.
2. You will not want to write out these refs, because you are handling
them separately. For example, in this case, we are already handling
the detached HEAD case separately when we generally figure out what
HEAD is. And in this case, the code does what you want by silently
skipping.
I tried to think of another case where you would want this, but I
really couldn't come up with one. Even if you are cloning a "track
only this branch" ref, you would still have its peer_ref pointing
to its local counterpart (even if it's "refs/heads/master" both
locally and on the remote).
-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