On Thu, Jan 05, 2012 at 08:49:40PM +0700, Nguyen Thai Ngoc Duy wrote:
> This allows you to do "git clone --branch=v1.7.8 git.git" and work
> right away from there. No big deal, just one more convenient step, I
> think. --branch taking a tag may be confusing though.
>
> We can still have master in this case instead of detached HEAD, which
> may make more sense because we use --branch. I don't care much which
> way should be used.
>
> Like? Dislike?
Seems like a reasonable goal to me. I agree that "--branch=v1.7.8" is a
little confusing, but not the end of the world. If we were designing it
from scratch, I might call it "--head" or "--checkout" or something to
indicate that it is what we are putting in HEAD. But I don't know that
it is worth renaming the option or adding a new option.
> @@ -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.
> @@ -750,7 +759,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> reflog_msg.buf);
> }
>
> - if (our_head_points_at) {
> + if (our_head_points_at &&
> + !prefixcmp(our_head_points_at->name, "refs/tags/")) {
I think I would prefer this check to be:
prefixcmp(our_head_points_at->name, "refs/heads/")
which more closely matches the rules for what is allowed to go in HEAD
as a symbolic ref. It's pretty hard to get something other than heads or
tags, but you can do it with "git clone --bare --mirror --branch=foo/bar".
I did argue above for doing away with that "feature", but I still think
it future-proofs this section of code to handle anything.
> + const struct ref *ref = our_head_points_at;
> + struct object *o;
> +
> + /* Detached HEAD */
> + o = deref_tag(parse_object(ref->old_sha1), NULL, 0);
> + update_ref(reflog_msg.buf, "HEAD", o->sha1, NULL,
> + REF_NODEREF, DIE_ON_ERR);
It's unlikely, but deref_tag can return NULL, in which case this will
segfault (ditto with parse_object, I think). I suspect that is a problem
in lots of places, though. I wonder if deref_tag should simply die if we
have a missing object (and we can add a _gently form for things like
fsck which want to handle the error condition).
Also, any reason the "warn" flag to deref_tag should not be 1?
Other than those minor complaints, the patch looks good to me.
-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