Re: bug? in checkout with ambiguous refnames
- From
Jeff King <peff@peff.net>
- Date
- Jan 11, 2011, 06:52 UTC
- Message-ID
- <20110111065207.GF10094@sigill.intra.peff.net>
- In-Reply-To
- <7vipy0483h.fsf@alter.siamese.dyndns.org>
On Fri, Jan 07, 2011 at 03:17:22PM -0800, Junio C Hamano wrote:
Show 6 quoted lines
> ... And this comes on top (should probably be squashed into one) to really > favor a branch over a tag. > > builtin/checkout.c | 26 ++++++++++---------------- > t/t2019-checkout-amiguous-ref.sh | 2 +- > 2 files changed, 11 insertions(+), 17 deletions(-)
Yeah, that looks sane to me (assuming all three patches squashed together). It took me a minute to figure out one subtlety, though:
Show 5 quoted lines
> + if ((check_ref_format(new.path) != CHECK_REF_FORMAT_OK) ||
> + !resolve_ref(new.path, rev, 1, NULL))
> + new.path = NULL; /* not an existing branch */
> +
> + if (!(new.commit = lookup_commit_reference_gently(rev, 1))) {We are relying on the fact that resolve_ref leaves "rev" alone in the case that it does not find anything. Which is mostly true (the only exception seems to be if you have a ref with non-hex garbage in it, in which case you will get some bogus sha1 in the output). I dunno if it is worth making it more explicit, like:
diff --git a/builtin/checkout.c b/builtin/checkout.c index f6f6172..afff56f 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -678,7 +678,7 @@ static const char *unique_tracking_name(const char *name) int cmd_checkout(int argc, const char **argv, const char *prefix) { struct checkout_opts opts; - unsigned char rev[20]; + unsigned char rev[20], branch_rev[20]; const char *arg; struct branch_info new; struct tree *source_tree = NULL; @@ -834,8 +834,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) new.name = arg; setup_branch_path(&new); - if ((check_ref_format(new.path) != CHECK_REF_FORMAT_OK) || - !resolve_ref(new.path, rev, 1, NULL)) + if ((check_ref_format(new.path) == CHECK_REF_FORMAT_OK) && + resolve_ref(new.path, branch_rev, 1, NULL)) + hashcpy(rev, branch_rev); + else new.path = NULL; /* not an existing branch */ if (!(new.commit = lookup_commit_reference_gently(rev, 1))) { My version somehow looks uglier, but I just worry about resolve_ref violating this undocumented subtlety sometime in the future. Also, one other question while we are on the subject. I think we all agree that "git checkout $foo" should prefer $foo as a branch. But what about "git checkout -b $branch $start_point"? Should $start_point follow the same "prefer branches" rule, or should it use the usual ref lookup rules? I was surprised to find that the current behavior is to die(), due to an explicit case in branch.c:create_branch. -Peff