Re: bug? in checkout with ambiguous refnames

[Jeff King <peff@peff.net>]

On Fri, Jan 07, 2011 at 03:17:22PM -0800, Junio C Hamano wrote:

> ... 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:

> +		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