From: Jeff King <peff@peff.net>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] clone: allow detached checkout when --branch takes a tag
Date: Thu, 5 Jan 2012 09:18:44 -0500 [thread overview]
Message-ID: <20120105141844.GA26771@sigill.intra.peff.net> (raw)
In-Reply-To: <1325771380-18862-1-git-send-email-pclouds@gmail.com>
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
next prev parent reply other threads:[~2012-01-05 14:18 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-01-05 13:49 [PATCH] clone: allow detached checkout when --branch takes a tag Nguyễn Thái Ngọc Duy
2012-01-05 14:18 ` Jeff King [this message]
2012-01-06 11:09 ` Nguyen Thai Ngoc Duy
2012-01-06 14:42 ` Jeff King
2012-01-05 16:22 ` Junio C Hamano
2012-01-06 7:35 ` Nguyen Thai Ngoc Duy
2012-01-08 11:46 ` [PATCH v2 1/6] t5601: add && Nguyễn Thái Ngọc Duy
2012-01-08 11:46 ` [PATCH v2 2/6] clone: write detached HEAD in bare repositories Nguyễn Thái Ngọc Duy
2012-01-08 11:46 ` [PATCH v2 3/6] clone: factor out checkout code Nguyễn Thái Ngọc Duy
2012-01-10 0:32 ` Junio C Hamano
2012-01-10 2:01 ` Nguyen Thai Ngoc Duy
2012-01-10 4:59 ` Junio C Hamano
2012-01-10 5:57 ` Nguyen Thai Ngoc Duy
2012-01-08 11:46 ` [PATCH v2 4/6] clone: --branch=<branch> always means refs/heads/<branch> Nguyễn Thái Ngọc Duy
2012-01-10 0:33 ` Junio C Hamano
2012-01-08 11:46 ` [PATCH v2 5/6] clone: allow --branch to take a tag Nguyễn Thái Ngọc Duy
2012-01-08 11:46 ` [PATCH v2 6/6] clone: print advice on checking out detached HEAD Nguyễn Thái Ngọc Duy
2012-01-10 0:36 ` Junio C Hamano
2012-01-10 1:54 ` Nguyen Thai Ngoc Duy
2012-01-10 4:49 ` Junio C Hamano
2012-01-10 5:54 ` Nguyen Thai Ngoc Duy
2012-01-10 9:56 ` [PATCH v3 00/10] nd/clone-detached Nguyễn Thái Ngọc Duy
2012-01-13 7:21 ` [PATCH v4 " Nguyễn Thái Ngọc Duy
2012-01-13 19:52 ` Junio C Hamano
2012-01-14 4:48 ` Nguyen Thai Ngoc Duy
2012-01-14 6:53 ` Junio C Hamano
2012-01-14 7:40 ` Nguyen Thai Ngoc Duy
2012-01-15 2:34 ` Junio C Hamano
2012-01-16 9:46 ` [PATCH v5 00/10] nd/clone-detached rebase onto nd/clone-single-branch Nguyễn Thái Ngọc Duy
2012-01-16 9:46 ` [PATCH v5 01/10] t5601: add missing && cascade Nguyễn Thái Ngọc Duy
2012-01-16 9:46 ` [PATCH v5 02/10] clone: write detached HEAD in bare repositories Nguyễn Thái Ngọc Duy
2012-01-16 9:46 ` [PATCH v5 03/10] clone: factor out checkout code Nguyễn Thái Ngọc Duy
2012-01-16 9:46 ` [PATCH v5 04/10] clone: factor out HEAD update code Nguyễn Thái Ngọc Duy
2012-01-16 9:46 ` [PATCH v5 05/10] clone: factor out remote ref writing Nguyễn Thái Ngọc Duy
2012-01-16 9:46 ` [PATCH v5 06/10] clone: delay cloning until after remote HEAD checking Nguyễn Thái Ngọc Duy
2012-01-16 9:46 ` [PATCH v5 07/10] clone: --branch=<branch> always means refs/heads/<branch> Nguyễn Thái Ngọc Duy
2012-01-16 9:46 ` [PATCH v5 08/10] clone: refuse to clone if --branch points to bogus ref Nguyễn Thái Ngọc Duy
2012-01-16 9:46 ` [PATCH v5 09/10] clone: allow --branch to take a tag Nguyễn Thái Ngọc Duy
2012-01-16 9:46 ` [PATCH v5 10/10] clone: print advice on checking out detached HEAD Nguyễn Thái Ngọc Duy
2012-01-13 7:21 ` [PATCH v4 01/10] t5601: add missing && cascade Nguyễn Thái Ngọc Duy
2012-01-13 7:21 ` [PATCH v4 02/10] clone: write detached HEAD in bare repositories Nguyễn Thái Ngọc Duy
2012-01-13 7:21 ` [PATCH v4 03/10] clone: factor out checkout code Nguyễn Thái Ngọc Duy
2012-01-13 7:21 ` [PATCH v4 04/10] clone: factor out HEAD update code Nguyễn Thái Ngọc Duy
2012-01-13 7:21 ` [PATCH v4 05/10] clone: factor out remote ref writing Nguyễn Thái Ngọc Duy
2012-01-13 7:21 ` [PATCH v4 06/10] clone: delay cloning until after remote HEAD checking Nguyễn Thái Ngọc Duy
2012-01-13 7:21 ` [PATCH v4 07/10] clone: --branch=<branch> always means refs/heads/<branch> Nguyễn Thái Ngọc Duy
2012-01-13 7:22 ` [PATCH v4 08/10] clone: refuse to clone if --branch points to bogus ref Nguyễn Thái Ngọc Duy
2012-01-13 7:22 ` [PATCH v4 09/10] clone: allow --branch to take a tag Nguyễn Thái Ngọc Duy
2012-01-13 7:22 ` [PATCH v4 10/10] clone: print advice on checking out detached HEAD Nguyễn Thái Ngọc Duy
2012-01-10 9:56 ` [PATCH v3 01/10] t5601: add missing && cascade Nguyễn Thái Ngọc Duy
2012-01-10 9:56 ` [PATCH v3 02/10] clone: write detached HEAD in bare repositories Nguyễn Thái Ngọc Duy
2012-01-10 9:57 ` [PATCH v3 03/10] clone: factor out checkout code Nguyễn Thái Ngọc Duy
2012-01-10 9:57 ` [PATCH v3 04/10] clone: factor out HEAD update code Nguyễn Thái Ngọc Duy
2012-01-10 9:57 ` [PATCH v3 05/10] clone: factor out remote ref writing Nguyễn Thái Ngọc Duy
2012-01-10 9:57 ` [PATCH v3 06/10] clone: delay cloning until after remote HEAD checking Nguyễn Thái Ngọc Duy
2012-01-11 19:36 ` Junio C Hamano
2012-01-12 1:27 ` Nguyen Thai Ngoc Duy
2012-01-24 0:15 ` Junio C Hamano
2012-01-24 0:34 ` Junio C Hamano
2012-01-24 0:39 ` Junio C Hamano
2012-01-24 7:01 ` Nguyen Thai Ngoc Duy
2012-01-10 9:57 ` [PATCH v3 07/10] clone: --branch=<branch> always means refs/heads/<branch> Nguyễn Thái Ngọc Duy
2012-01-11 20:01 ` Junio C Hamano
2012-01-12 3:00 ` Nguyen Thai Ngoc Duy
2012-01-10 9:57 ` [PATCH v3 08/10] clone: refuse to clone if --branch points to bogus ref Nguyễn Thái Ngọc Duy
2012-01-10 9:57 ` [PATCH v3 09/10] clone: allow --branch to take a tag Nguyễn Thái Ngọc Duy
2012-01-11 23:57 ` Junio C Hamano
2012-01-10 9:57 ` [PATCH v3 10/10] clone: print advice on checking out detached HEAD Nguyễn Thái Ngọc Duy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120105141844.GA26771@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=pclouds@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).