* [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --"
@ 2013-09-25 19:31 Matthieu Moy
2013-09-25 19:31 ` [PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --' Matthieu Moy
2013-09-25 22:33 ` [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Jonathan Nieder
0 siblings, 2 replies; 7+ messages in thread
From: Matthieu Moy @ 2013-09-25 19:31 UTC (permalink / raw
To: git, gitster; +Cc: pclouds, jc, 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.Moy@imag.fr>
---
Since v1: added a paragraph in the block comment.
builtin/checkout.c | 38 ++++++++++++++++++++++++++++++--------
t/t2024-checkout-dwim.sh | 22 ++++++++++++++++++++++
2 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0f57397..a5a12f6 100644
--- 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;
+}
+
static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new,
@@ -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).
+ *
* case 2: git checkout -- [<paths>]
*
* everything after the '--' must be paths.
@@ -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 (has_dash_dash) /* case (1) */
- die(_("invalid reference: %s"), arg);
- if (dwim_new_local_branch_ok &&
- !check_filename(NULL, arg) &&
- argc == 1) {
+ if (get_sha1_mb(arg, rev)) { /* case (1)? */
+ 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;
arg = remote;
/* DWIMmed to create local branch */
} else {
- return argcount;
+ return error_invalid_ref(arg, has_dash_dash, argcount);
}
}
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 094b92e..d9afdb2 100755
--- 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 &&
+ 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
+'
+
test_done
--
1.8.4.474.g128a96c
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --'
2013-09-25 19:31 [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Matthieu Moy
@ 2013-09-25 19:31 ` Matthieu Moy
2013-09-25 22:43 ` Jonathan Nieder
2013-09-25 22:33 ` [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Jonathan Nieder
1 sibling, 1 reply; 7+ messages in thread
From: Matthieu Moy @ 2013-09-25 19:31 UTC (permalink / raw
To: git, gitster; +Cc: pclouds, jc, Matthieu Moy
The previous code was detecting the presence of "--" by looking only at
argument 1. As a result, "git checkout foo bar --" was interpreted as an
ambiguous file/revision list, and errored out with:
error: pathspec 'foo' did not match any file(s) known to git.
error: pathspec 'bar' did not match any file(s) known to git.
error: pathspec '--' did not match any file(s) known to git.
This patch fixes it by walking through the argument list to find the
"--", and now complains about the number of references given.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
> > + die("only one reference expected, %d given.", has_dash_dash);
>
> The translator in me says this string should be marked for translation
> like others in git-checkout...
Indeed. Sorry, it's not like I didn't knoW about _("...") /o\.
builtin/checkout.c | 11 ++++++++++-
t/t2010-checkout-ambiguous.sh | 6 ++++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a5a12f6..3dd45cc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -882,6 +882,7 @@ static int parse_branchname_arg(int argc, const char **argv,
unsigned char branch_rev[20];
const char *arg;
int has_dash_dash;
+ int i;
/*
* case 1: git checkout <ref> -- [<paths>]
@@ -925,7 +926,15 @@ static int parse_branchname_arg(int argc, const char **argv,
return 1;
arg = argv[0];
- has_dash_dash = (argc > 1) && !strcmp(argv[1], "--");
+ has_dash_dash = 0;
+ for (i = 0; i < argc; i++) {
+ if (!strcmp(argv[i], "--")) {
+ has_dash_dash = i;
+ break;
+ }
+ }
+ if (has_dash_dash >= 2)
+ die(_("only one reference expected, %d given."), has_dash_dash);
if (!strcmp(arg, "-"))
arg = "@{-1}";
diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
index 7cc0a35..ce6b6e2 100755
--- a/t/t2010-checkout-ambiguous.sh
+++ b/t/t2010-checkout-ambiguous.sh
@@ -47,4 +47,10 @@ test_expect_success 'disambiguate checking out from a tree-ish' '
git diff --exit-code --quiet
'
+test_expect_success C_LOCALE_OUTPUT 'accurate error message with more than one ref' '
+ test_must_fail git checkout HEAD master -- 2>actual &&
+ echo "fatal: only one reference expected, 2 given." >expect &&
+ test_cmp expect actual
+'
+
test_done
--
1.8.4.474.g128a96c
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --"
2013-09-25 19:31 [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Matthieu Moy
2013-09-25 19:31 ` [PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --' Matthieu Moy
@ 2013-09-25 22:33 ` Jonathan Nieder
2013-09-26 8:03 ` Matthieu Moy
2013-09-26 9:03 ` Matthieu Moy
1 sibling, 2 replies; 7+ messages in thread
From: Jonathan Nieder @ 2013-09-25 22:33 UTC (permalink / raw
To: Matthieu Moy; +Cc: git, gitster, pclouds, jc
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --'
2013-09-25 19:31 ` [PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --' Matthieu Moy
@ 2013-09-25 22:43 ` Jonathan Nieder
2013-09-26 8:59 ` Matthieu Moy
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Nieder @ 2013-09-25 22:43 UTC (permalink / raw
To: Matthieu Moy; +Cc: git, gitster, pclouds, jc
Hi,
Matthieu Moy wrote:
> error: pathspec 'foo' did not match any file(s) known to git.
> error: pathspec 'bar' did not match any file(s) known to git.
> error: pathspec '--' did not match any file(s) known to git.
>
> This patch fixes it by walking through the argument list to find the
> "--", and now complains about the number of references given.
Good catch. Just some nits below.
[...]
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -882,6 +882,7 @@ static int parse_branchname_arg(int argc, const char **argv,
> unsigned char branch_rev[20];
> const char *arg;
> int has_dash_dash;
> + int i;
>
> /*
> * case 1: git checkout <ref> -- [<paths>]
> @@ -925,7 +926,15 @@ static int parse_branchname_arg(int argc, const char **argv,
> return 1;
>
> arg = argv[0];
> - has_dash_dash = (argc > 1) && !strcmp(argv[1], "--");
> + has_dash_dash = 0;
> + for (i = 0; i < argc; i++) {
> + if (!strcmp(argv[i], "--")) {
> + has_dash_dash = i;
> + break;
> + }
> + }
> + if (has_dash_dash >= 2)
> + die(_("only one reference expected, %d given."), has_dash_dash);
(The argv[0] == "--" case is handled a few lines above.)
At first I skipped the loop and read this as "if (there are two or more
'--' arguments)". How about doing one of the following to make it
easier to read quickly?
(a) rename has_dash_dash here to dash_dash_pos, or
(b) put the check in the loop, like so:
has_dash_dash = 0;
for (i = 0; i < ...) {
if (strcmp(argv[i], "--"))
continue;
if (i == 0) /* case (2) */
return 1;
if (i > 1)
die(_("only one reference expected ...);
has_dash_dash = 1;
break;
}
[...]
> --- a/t/t2010-checkout-ambiguous.sh
> +++ b/t/t2010-checkout-ambiguous.sh
> @@ -47,4 +47,10 @@ test_expect_success 'disambiguate checking out from a tree-ish' '
> git diff --exit-code --quiet
> '
>
> +test_expect_success C_LOCALE_OUTPUT 'accurate error message with more than one ref' '
> + test_must_fail git checkout HEAD master -- 2>actual &&
> + echo "fatal: only one reference expected, 2 given." >expect &&
> + test_cmp expect actual
Nits:
- if we change this from 'fatal' to 'error' or reword the message as
part of a libification some day, it would be a nuisance to have to
update this test. Maybe a 'grep' could make it more flexible.
- most of the test (though not the interesting part) can run in the
!C_LOCALE_OUTPUT case if you use test_i18ncmp or test_i18ngrep
- even the check for "2" can run in the !C_LOCALE_OUTPUT case. :)
e.g. something like
test_must_fail ... 2>actual &&
grep 2 actual &&
test_i18ngrep "one reference expected, 2 given" actual
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --"
2013-09-25 22:33 ` [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Jonathan Nieder
@ 2013-09-26 8:03 ` Matthieu Moy
2013-09-26 9:03 ` Matthieu Moy
1 sibling, 0 replies; 7+ messages in thread
From: Matthieu Moy @ 2013-09-26 8:03 UTC (permalink / raw
To: Jonathan Nieder; +Cc: git, gitster, pclouds, jc
Jonathan Nieder <jrnieder@gmail.com> writes:
> 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".
I'm not sure what was the intent, but I to me 'git checkout <branch> --'
means "I know <branch> is a ref, so don't try to interpret it
otherwise".
> 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.
There's also --no-guess for that (purposely undocumented according to
the commit message of 46148dd7ea).
> 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.
I made a function just because I needed the same pattern twice, and
having a function avoided overly nested if statements.
>> - 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?
Not really. We're likely in an incorrect use of case 1, and want to give
an accurate error message (like "invalid reference").
>> + 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))) {
I disagree that this is simpler. I introduced try_dwim precisely to
avoid this kind of monster boolean expression, and to leave room for
comments in the code.
You'd also need a "if (has_dash_dash)" inside this branch of the if to
give an accurate error message for "git checkout <non-branch> --".
> [...]
>> --- 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?
Oops, fixed.
> Do we check that "git checkout --no-track spam --" avoids Dscho's
> DWIM?
Could be done in addition, but I have no time for this, sorry.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --'
2013-09-25 22:43 ` Jonathan Nieder
@ 2013-09-26 8:59 ` Matthieu Moy
0 siblings, 0 replies; 7+ messages in thread
From: Matthieu Moy @ 2013-09-26 8:59 UTC (permalink / raw
To: Jonathan Nieder; +Cc: git, gitster, pclouds, jc
Jonathan Nieder <jrnieder@gmail.com> writes:
> (a) rename has_dash_dash here to dash_dash_pos, or
> (b) put the check in the loop, like so:
I agree with (a), but not with (b). I think separating the computation
of the position and the diagnosis makes it clearer.
I reworked the code a bit, the diagnosis part now looks like
if (dash_dash_pos == 0)
return 1; /* case (2) */
else if (dash_dash_pos == 1)
has_dash_dash = 1; /* case (3) or (1) */
else if (dash_dash_pos >= 2)
die(_("only one reference expected, %d given."), dash_dash_pos);
>> +test_expect_success C_LOCALE_OUTPUT 'accurate error message with more than one ref' '
>> + test_must_fail git checkout HEAD master -- 2>actual &&
>> + echo "fatal: only one reference expected, 2 given." >expect &&
>> + test_cmp expect actual
>
> Nits:
>
> - if we change this from 'fatal' to 'error' or reword the message as
> part of a libification some day, it would be a nuisance to have to
> update this test. Maybe a 'grep' could make it more flexible.
>
> - most of the test (though not the interesting part) can run in the
> !C_LOCALE_OUTPUT case if you use test_i18ncmp or test_i18ngrep
>
> - even the check for "2" can run in the !C_LOCALE_OUTPUT case. :)
> e.g. something like
>
> test_must_fail ... 2>actual &&
> grep 2 actual &&
> test_i18ngrep "one reference expected, 2 given" actual
OK, I buy your version. Thanks,
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --"
2013-09-25 22:33 ` [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Jonathan Nieder
2013-09-26 8:03 ` Matthieu Moy
@ 2013-09-26 9:03 ` Matthieu Moy
1 sibling, 0 replies; 7+ messages in thread
From: Matthieu Moy @ 2013-09-26 9:03 UTC (permalink / raw
To: Jonathan Nieder; +Cc: git, gitster, pclouds, jc
Jonathan Nieder <jrnieder@gmail.com> writes:
> 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).
Actually, no. Below, we have
/*
* Do not complain the most common case
* git checkout branch
* even if there happen to be a file called 'branch';
* it would be extremely annoying.
*/
Which is a subcase of (3). The guess done in (3) is permissive, and the
one done with more arguments (4) is strict.
> case 4: git checkout <something> <paths>
>
> The first argument must not be ambiguous.
I wasn't very convinced by your version at first, but I gave it a try.
I'm not sure the comment is actually better now, but I think it exposes
the complexity of the guess, and the difficulty to map the code and the
comment more. Other people should take this as an incentive to improve
the situation (but I'm really getting short of Git time budget, sorry).
> 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;
Actually, the comments are wrong here. The comment <-> code mapping is
more complex than this. At this point of the code, you know that the
first argument is not a commit, and that dwim has been ruled-out. But
that may be case 3 also:
git checkout --no-guess <non-branch> [--]
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-09-26 9:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-25 19:31 [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Matthieu Moy
2013-09-25 19:31 ` [PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --' Matthieu Moy
2013-09-25 22:43 ` Jonathan Nieder
2013-09-26 8:59 ` Matthieu Moy
2013-09-25 22:33 ` [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Jonathan Nieder
2013-09-26 8:03 ` Matthieu Moy
2013-09-26 9:03 ` Matthieu Moy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.