Hi, Matthieu Moy wrote:
> 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. Hm. I am not sure that was just an implementation side-effect. Normally 'git checkout <branch> --' means "Check out that branch, and I mean it!". 'git checkout -- <pattern>' means "Check out these paths from the index, and I mean it!" 'git checkout <blah>' means "Do what I mean". On the other hand, if I want to do 'git checkout <branch> --' while disabling the "set up master to track origin/master" magic, I can use 'git checkout --no-track <branch> --'. So I think this is a good change. [...] > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -863,6 +863,14 @@ 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); > + else > + return argcount; > +} Style: I'd leave out the 'else' if (has_dash_dash) ... return argcount; More importantly, what's the contract behind this function? Is there a simpler explanation than "If argument #2 is true, print a certain message depending on argument #1; otherwise, return argument #3?". If not, it might be clearer to inline it. [...] > @@ -881,6 +889,12 @@ static int parse_branchname_arg(int argc, const char > **argv, > * <ref> must be a valid tree, everything after the '--' must be > * a path. > * > + * A sub-case of (1) is "git checkout <ref> --". In this > + * case, checkout behaves like case (3), except that it does > + * not attempt to understand <ref> as a file (hence, the > + * short-hand to create branch <ref> works even if <ref> > + * exists as a filename). Maybe simpler to explain as a separate case? case 1: git checkout <ref> -- <paths> case 2: git checkout -- [<paths>] case 3: git checkout <something> [--] If <something> is a commit, [...] If <something> is _not_ a commit, either "--" is present or <something> is not a path, no -t nor -b was given, and [...] Otherwise, if "--" is present, treat it like case (1). Otherwise behave like case (4). case 4: git checkout <something> <paths> The first argument must not be ambiguous. - If it's *only* a reference, [...] [...] > @@ -916,20 +930,28 @@ static int parse_branchname_arg(int argc, const char > **argv, > if (!strcmp(arg, "-")) > arg = "@{-1}"; > > - if (get_sha1_mb(arg, rev)) { > + if (get_sha1_mb(arg, rev)) { /* case (1)? */ The check means that we are most likely not in case (1), since arg isn't a commit name, right? > - if (has_dash_dash) /* case (1) */ > - die(_("invalid reference: %s"), arg); > - if (dwim_new_local_branch_ok && > - !check_filename(NULL, arg) && > - argc == 1) { > - const char *remote = unique_tracking_name(arg, rev); > - if (!remote) > - return argcount; > + 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 error_invalid_ref(arg, has_dash_dash, > argcount); This could be simplified by eliminating try_dwim local. We are trying case (3) first: if (dwim_new_local_branch_ok && (argc == 1 || (argc == 2 && has_dash_dash)) && (has_dash_dash || !check_filename(NULL, arg))) { ... Then can come the "invalid reference" check for case (1): } else if (has_dash_dash) /* case (1) */ die(...); Then case (4). else /* case (4) */ return argcount; [...] > --- a/t/t2024-checkout-dwim.sh > +++ b/t/t2024-checkout-dwim.sh > @@ -164,4 +164,26 @@ test_expect_success 'checkout of branch from a single > remote succeeds #4' ' > test_branch_upstream eggs repo_d eggs > ' > > +test_expect_success 'checkout of branch with a file having the same name > fails' ' > + git checkout -B master && > + test_might_fail git branch -D spam && > + > + >spam && > + test_must_fail git checkout spam && > + test_must_fail git checkout spam && Why twice? > + test_must_fail git rev-parse --verify refs/heads/spam && > + test_branch master > +' > + > +test_expect_success 'checkout <branch> -- succeeds, even if a file with the > same name exists' ' > + git checkout -B master && > + test_might_fail git branch -D spam && > + > + >spam && > + git checkout spam -- && > + test_branch spam && > + test_cmp_rev refs/remotes/extra_dir/repo_c/extra_dir/spam HEAD && > + test_branch_upstream spam repo_c spam Nice. Do we check that "git checkout --no-track spam --" avoids Dscho's DWIM? Thanks, and hope that helps, Jonathan -- 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