From: Nguyen Thai Ngoc Duy Date: Fri, 06 Jan 2012 11:09:16 GMT Subject: Re: [PATCH] clone: allow detached checkout when --branch takes a tag Message-ID: In-Reply-To: <20120105141844.GA26771@sigill.intra.peff.net> 2012/1/5 Jeff King : >> @@ -721,6 +722,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix) >>                       strbuf_release(&head); >> >>                       if (!our_head_points_at) { >> +                             strbuf_addstr(&head, "refs/tags/"); >> +                             strbuf_addstr(&head, option_branch); >> +                             our_head_points_at = >> +                                     find_ref_by_name(mapped_refs, head.buf); >> +                             strbuf_release(&head); >> +                     } >> + >> +                     if (!our_head_points_at) { > > Hmm. The context just above your patch that got snipped does this: > >    strbuf_addstr(&head, src_ref_prefix); >    strbuf_addstr(&head, option_branch); >    our_head_points_at = >        find_ref_by_name(mapped_refs, head.buf); > > where src_ref_prefix typically is "refs/heads/", and clearly you are > meaning to do the same thing for tags. But the use of "src_ref_prefix" > is interesting. > > It is always "refs/heads/" unless we are cloning into a bare mirror, in > which case it is "refs/". So with your patch in the non-mirror case, > doing "--branch=foo" would try "refs/heads/foo" followed by > "refs/tags/foo". Which makes sense. But in the mirror case, it will try > "refs/foo" followed by "refs/tags/foo", which is kind of odd. > > I wonder, though, if the original code makes any sense. By using > "refs/", I would have to say "--branch=heads/foo", which is kind of > weird and undocumented. I think it should probably always be > "refs/heads/", no matter if we are mirroring or not. --branch should not be used with --mirror in my opinion. --branch changes HEAD so it's no longer an exact mirror. -- Duy