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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html