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