Re: [PATCH v3 1/2] checkout: allow dwim for branch creation for git checkout $branch --

[Junio C Hamano]

Matthieu Moy matthieu@imag.fr writes:

 diff --git a/builtin/checkout.c b/builtin/checkout.c
 index 0f57397..9edd9c3 100644
 --- a/builtin/checkout.c
 +++ b/builtin/checkout.c
 @@ -863,6 +863,13 @@ static const char *unique_tracking_name(const char 
 *name, unsigned char *sha1)
   return NULL;
  }
  
 +static int error_invalid_ref(const char *arg, int has_dash_dash, int 
 argcount)
 +{
 + if (has_dash_dash)
 + die(_(invalid reference: %s), arg);
 + return argcount;
 +}

This is somewhat unfortunate; it pretends to be a reusable helper by
being a separate function, but it is not very reusable (see below).

 @@ -917,19 +934,32 @@ static int parse_branchname_arg(int argc, const char 
 **argv,
   arg = @{-1};
  
   if (get_sha1_mb(arg, rev)) {
 + /*
 +  * Either case (3) or (4), with something not being
 +  * a commit, or an attempt to use case (1) with an
 +  * invalid ref.
 +  */
 + int try_dwim = dwim_new_local_branch_ok;
 +
 + if (check_filename(NULL, arg)  !has_dash_dash)
 + try_dwim = 0;
 + /*
 +  * Accept git checkout foo and git checkout foo --
 +  * as candidates for dwim.
 +  */
 + if (!(argc == 1  !has_dash_dash) 
 + !(argc == 2  has_dash_dash))
 + try_dwim = 0;
 +
 + if (try_dwim) {
   const char *remote = unique_tracking_name(arg, rev);

Up to this point, the updated code makes very good sense.

   if (!remote)
 - return argcount;
 + return error_invalid_ref(arg, has_dash_dash, 
 argcount);

The original that returned argcount from here were unnecessarily
misleading in the first place. It saw git checkout foo where foo
does not refer to an object nor a filesystem entity and there was no
unique refs/remotes/*/foo; it wanted to return 0 to tell the
caller that it consumed zero arguments as branch names.

And the updated code is even more obscure.  This calling site makes
it look as if it is an error to have no unique refs/remotes/*/foo
at this point of the code by naming the helper function error_*(),
but it is an error in some case and not in others.

if (!remote) {
if (has_dash_dash)
die(_(...));
return 0;
}

would be a lot more understandable.

The only reason you have conditional die() here (and on the else
side of this if statement) is because you delayed the die that was
at a much earlier point in the original.  And the only reason you
created the unfortunate helper function is because you need to deal
with that delayed decision to die now in two places.

So it may be even cleaner to read if you did it this way:

if (get_sha1_mb(...)) {
/*
 * The first token is not a valid rev; we should
 * ordinarily error out if git checkout foo --
 * if foo is not a valid rev, but first see if
 * we can auto-create foo to continue...
 */
int recover_with_dwim = dwim_new_local_branch_ok;

... decide if we want to recover_with_dwim ...

if (recover_with_dwim) {
const char *remote = unique_tracking_name(arg, rev);
if (remote) {
*new_branch = arg;
arg = remote;
} else {
/* no; arg cannot be salvaged */
recover_with_dwim = 0;
}
}

if (!recover_with_dwim) {
if (has_dash_dash)
die(_(invalid ref %s, arg);
return 0; /* we saw no branch/commit */
}
/* otherwise we made a successful recovery */
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/2] checkout: allow dwim for branch creation for git checkout $branch --

[Matthieu Moy]

The -- notation disambiguates files and branches, but as a side-effect
of the previous implementation, also disabled the branch auto-creation
when $branch does not exist.

A possible scenario is then:

git checkout $branch
= fails if $branch is both a ref and a file, and suggests --

git checkout $branch --
= refuses to create the $branch

This patch allows the second form to create $branch, and since the -- is
provided, it does not look for file named $branch.

Signed-off-by: Matthieu Moy matthieu@imag.fr
---
Since v2: essentially Jonathan's comments.

 builtin/checkout.c   | 70 ++--
 t/t2024-checkout-dwim.sh | 21 +++
 2 files changed, 71 insertions(+), 20 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0f57397..9edd9c3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -863,6 +863,13 @@ static const char *unique_tracking_name(const char *name, 
unsigned char *sha1)
return NULL;
 }
 
+static int error_invalid_ref(const char *arg, int has_dash_dash, int argcount)
+{
+   if (has_dash_dash)
+   die(_(invalid reference: %s), arg);
+   return argcount;
+}
+
 static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new,
@@ -885,20 +892,30 @@ static int parse_branchname_arg(int argc, const char 
**argv,
 *
 *   everything after the '--' must be paths.
 *
-* case 3: git checkout something [paths]
+* case 3: git checkout something [--]
+*
+*   (a) If something is a commit, that is to
+*   switch to the branch or detach HEAD at it.  As a special case,
+*   if something is A...B (missing A or B means HEAD but you can
+*   omit at most one side), and if there is a unique merge base
+*   between A and B, A...B names that merge base.
+*
+*   (b) If something is _not_ a commit, either -- is present
+*   or something is not a path, no -t nor -b was given, and
+*   and there is a tracking branch whose name is something
+*   in one and only one remote, then this is a short-hand to
+*   fork local something from that remote-tracking branch.
+*
+*   (c) Otherwise, if -- is present, treat it like case (1).
 *
-*   With no paths, if something is a commit, that is to
-*   switch to the branch or detach HEAD at it.  As a special case,
-*   if something is A...B (missing A or B means HEAD but you can
-*   omit at most one side), and if there is a unique merge base
-*   between A and B, A...B names that merge base.
+*   (d) Otherwise :
+*   - if it's a reference, treat it like case (1)
+*   - else if it's a path, treat it like case (2)
+*   - else: fail.
 *
-*   With no paths, if something is _not_ a commit, no -t nor -b
-*   was given, and there is a tracking branch whose name is
-*   something in one and only one remote, then this is a short-hand
-*   to fork local something from that remote-tracking branch.
+* case 4: git checkout something paths
 *
-*   Otherwise something shall not be ambiguous.
+*   The first argument must not be ambiguous.
 *   - If it's *only* a reference, treat it like case (1).
 *   - If it's only a path, treat it like case (2).
 *   - else: fail.
@@ -917,19 +934,32 @@ static int parse_branchname_arg(int argc, const char 
**argv,
arg = @{-1};
 
if (get_sha1_mb(arg, rev)) {
-   if (has_dash_dash)  /* case (1) */
-   die(_(invalid reference: %s), arg);
-   if (dwim_new_local_branch_ok 
-   !check_filename(NULL, arg) 
-   argc == 1) {
+   /*
+* Either case (3) or (4), with something not being
+* a commit, or an attempt to use case (1) with an
+* invalid ref.
+*/
+   int try_dwim = dwim_new_local_branch_ok;
+
+   if (check_filename(NULL, arg)  !has_dash_dash)
+   try_dwim = 0;
+   /*
+* Accept git checkout foo and git checkout foo --
+* as candidates for dwim.
+*/
+   if (!(argc == 1  !has_dash_dash) 
+   !(argc == 2  has_dash_dash))
+   try_dwim = 0;
+
+   if (try_dwim) {
const char *remote = unique_tracking_name(arg, rev);
if (!remote)
-   return argcount;
+   return error_invalid_ref(arg, has_dash_dash, 
argcount);
*new_branch = arg;