{"thread":{"id":"35021","subject":"[PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --'","startedAt":"2013-09-25T19:31:10Z","lastAt":"2013-09-26T09:03:24Z","messageCount":7,"participants":["Matthieu Moy","Jonathan Nieder"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"228229","messageId":"1380137471-26972-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"35021","inReplyTo":null,"subject":"[PATCH v2 1/2] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-09-25T19:31:10Z","receivedAt":"2013-09-25T19:31:10Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"The \"--\" notation disambiguates files and branches, but as a side-effect\nof the previous implementation, also disabled the branch auto-creation\nwhen $branch does not exist.\n\nA possible scenario is then:\n\ngit checkout $branch\n=> fails if $branch is both a ref and a file, and suggests --\n\ngit checkout $branch --\n=> refuses to create the $branch\n\nThis patch allows the second form to create $branch, and since the -- is\nprovided, it does not look for file named $branch.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\nSince v1: added a paragraph in the block comment.\n\n builtin/checkout.c       | 38 ++++++++++++++++++++++++++++++--------\n t/t2024-checkout-dwim.sh | 22 ++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 0f57397..a5a12f6 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -863,6 +863,14 @@ static const char *unique_tracking_name(const char *name, unsigned char *sha1)\n \treturn NULL;\n }\n \n+static int error_invalid_ref(const char *arg, int has_dash_dash, int argcount)\n+{\n+\tif (has_dash_dash)\n+\t\tdie(_(\"invalid reference: %s\"), arg);\n+\telse\n+\t\treturn argcount;\n+}\n+\n static int parse_branchname_arg(int argc, const char **argv,\n \t\t\t\tint dwim_new_local_branch_ok,\n \t\t\t\tstruct branch_info *new,\n@@ -881,6 +889,12 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t *   <ref> must be a valid tree, everything after the '--' must be\n \t *   a path.\n \t *\n+\t *   A sub-case of (1) is \"git checkout <ref> --\". In this\n+\t *   case, checkout behaves like case (3), except that it does\n+\t *   not attempt to understand <ref> as a file (hence, the\n+\t *   short-hand to create branch <ref> works even if <ref>\n+\t *   exists as a filename).\n+\t *\n \t * case 2: git checkout -- [<paths>]\n \t *\n \t *   everything after the '--' must be paths.\n@@ -916,20 +930,28 @@ static int parse_branchname_arg(int argc, const char **argv,\n \tif (!strcmp(arg, \"-\"))\n \t\targ = \"@{-1}\";\n \n-\tif (get_sha1_mb(arg, rev)) {\n-\t\tif (has_dash_dash)          /* case (1) */\n-\t\t\tdie(_(\"invalid reference: %s\"), arg);\n-\t\tif (dwim_new_local_branch_ok &&\n-\t\t    !check_filename(NULL, arg) &&\n-\t\t    argc == 1) {\n+\tif (get_sha1_mb(arg, rev)) { /* case (1)? */\n+\t\tint try_dwim = dwim_new_local_branch_ok;\n+\n+\t\tif (check_filename(NULL, arg) && !has_dash_dash)\n+\t\t\ttry_dwim = 0;\n+\t\t/*\n+\t\t * Accept \"git checkout foo\" and \"git checkout foo --\"\n+\t\t * as candidates for dwim.\n+\t\t */\n+\t\tif (!(argc == 1 && !has_dash_dash) &&\n+\t\t    !(argc == 2 && has_dash_dash))\n+\t\t\ttry_dwim = 0;\n+\n+\t\tif (try_dwim) {\n \t\t\tconst char *remote = unique_tracking_name(arg, rev);\n \t\t\tif (!remote)\n-\t\t\t\treturn argcount;\n+\t\t\t\treturn error_invalid_ref(arg, has_dash_dash, argcount);\n \t\t\t*new_branch = arg;\n \t\t\targ = remote;\n \t\t\t/* DWIMmed to create local branch */\n \t\t} else {\n-\t\t\treturn argcount;\n+\t\t\treturn error_invalid_ref(arg, has_dash_dash, argcount);\n \t\t}\n \t}\n \ndiff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\nindex 094b92e..d9afdb2 100755\n--- a/t/t2024-checkout-dwim.sh\n+++ b/t/t2024-checkout-dwim.sh\n@@ -164,4 +164,26 @@ test_expect_success 'checkout of branch from a single remote succeeds #4' '\n \ttest_branch_upstream eggs repo_d eggs\n '\n \n+test_expect_success 'checkout of branch with a file having the same name fails' '\n+\tgit checkout -B master &&\n+\ttest_might_fail git branch -D spam &&\n+\n+\t>spam &&\n+\ttest_must_fail git checkout spam &&\n+\ttest_must_fail git checkout spam &&\n+\ttest_must_fail git rev-parse --verify refs/heads/spam &&\n+\ttest_branch master\n+'\n+\n+test_expect_success 'checkout <branch> -- succeeds, even if a file with the same name exists' '\n+\tgit checkout -B master &&\n+\ttest_might_fail git branch -D spam &&\n+\n+\t>spam &&\n+\tgit checkout spam -- &&\n+\ttest_branch spam &&\n+\ttest_cmp_rev refs/remotes/extra_dir/repo_c/extra_dir/spam HEAD &&\n+\ttest_branch_upstream spam repo_c spam\n+'\n+\n test_done\n-- \n1.8.4.474.g128a96c\n"},{"id":"228228","messageId":"1380137471-26972-2-git-send-email-Matthieu.Moy@imag.fr","threadId":"35021","inReplyTo":"1380137471-26972-1-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --'","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-09-25T19:31:11Z","receivedAt":"2013-09-25T19:31:11Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"The previous code was detecting the presence of \"--\" by looking only at\nargument 1. As a result, \"git checkout foo bar --\" was interpreted as an\nambiguous file/revision list, and errored out with:\n\nerror: pathspec 'foo' did not match any file(s) known to git.\nerror: pathspec 'bar' did not match any file(s) known to git.\nerror: pathspec '--' did not match any file(s) known to git.\n\nThis patch fixes it by walking through the argument list to find the\n\"--\", and now complains about the number of references given.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n> > +               die(\"only one reference expected, %d given.\", has_dash_dash);\n> \n> The translator in me says this string should be marked for translation\n> like others in git-checkout...\n\nIndeed. Sorry, it's not like I didn't knoW about _(\"...\") /o\\.\n\n builtin/checkout.c            | 11 ++++++++++-\n t/t2010-checkout-ambiguous.sh |  6 ++++++\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a5a12f6..3dd45cc 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -882,6 +882,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \tunsigned char branch_rev[20];\n \tconst char *arg;\n \tint has_dash_dash;\n+\tint i;\n \n \t/*\n \t * case 1: git checkout <ref> -- [<paths>]\n@@ -925,7 +926,15 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\treturn 1;\n \n \targ = argv[0];\n-\thas_dash_dash = (argc > 1) && !strcmp(argv[1], \"--\");\n+\thas_dash_dash = 0;\n+\tfor (i = 0; i < argc; i++) {\n+\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\thas_dash_dash = i;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\tif (has_dash_dash >= 2)\n+\t\tdie(_(\"only one reference expected, %d given.\"), has_dash_dash);\n \n \tif (!strcmp(arg, \"-\"))\n \t\targ = \"@{-1}\";\ndiff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh\nindex 7cc0a35..ce6b6e2 100755\n--- a/t/t2010-checkout-ambiguous.sh\n+++ b/t/t2010-checkout-ambiguous.sh\n@@ -47,4 +47,10 @@ test_expect_success 'disambiguate checking out from a tree-ish' '\n \tgit diff --exit-code --quiet\n '\n \n+test_expect_success C_LOCALE_OUTPUT 'accurate error message with more than one ref' '\n+\ttest_must_fail git checkout HEAD master -- 2>actual &&\n+\techo \"fatal: only one reference expected, 2 given.\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.4.474.g128a96c\n"},{"id":"228235","messageId":"20130925223334.GB9464@google.com","threadId":"35021","inReplyTo":"1380137471-26972-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH v2 1/2] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-25T22:33:34Z","receivedAt":"2013-09-25T22:33:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMatthieu Moy wrote:\n\n> The \"--\" notation disambiguates files and branches, but as a side-effect\n> of the previous implementation, also disabled the branch auto-creation\n> when $branch does not exist.\n\nHm.  I am not sure that was just an implementation side-effect.\n\nNormally 'git checkout <branch> --' means \"Check out that branch,\nand I mean it!\".  'git checkout -- <pattern>' means \"Check out\nthese paths from the index, and I mean it!\"  'git checkout <blah>'\nmeans \"Do what I mean\".\n\nOn the other hand, if I want to do 'git checkout <branch> --'\nwhile disabling the \"set up master to track origin/master\" magic,\nI can use 'git checkout --no-track <branch> --'.  So I think this\nis a good change.\n\n[...]\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -863,6 +863,14 @@ static const char *unique_tracking_name(const char *name, unsigned char *sha1)\n>  \treturn NULL;\n>  }\n>  \n> +static int error_invalid_ref(const char *arg, int has_dash_dash, int argcount)\n> +{\n> +\tif (has_dash_dash)\n> +\t\tdie(_(\"invalid reference: %s\"), arg);\n> +\telse\n> +\t\treturn argcount;\n> +}\n\nStyle: I'd leave out the 'else'\n\n\tif (has_dash_dash)\n\t\t...\n\treturn argcount;\n\nMore importantly, what's the contract behind this function?  Is there\na simpler explanation than \"If argument #2 is true, print a certain\nmessage depending on argument #1; otherwise, return argument #3?\".\nIf not, it might be clearer to inline it.\n\n[...]\n> @@ -881,6 +889,12 @@ static int parse_branchname_arg(int argc, const char **argv,\n>  \t *   <ref> must be a valid tree, everything after the '--' must be\n>  \t *   a path.\n>  \t *\n> +\t *   A sub-case of (1) is \"git checkout <ref> --\". In this\n> +\t *   case, checkout behaves like case (3), except that it does\n> +\t *   not attempt to understand <ref> as a file (hence, the\n> +\t *   short-hand to create branch <ref> works even if <ref>\n> +\t *   exists as a filename).\n\nMaybe simpler to explain as a separate case?\n\n\tcase 1: git checkout <ref> -- <paths>\n\tcase 2: git checkout -- [<paths>]\n\tcase 3: git checkout <something> [--]\n\n\t  If <something> is a commit, [...]\n\n\t  If <something> is _not_ a commit, either \"--\" is present or\n\t  <something> is not a path, no -t nor -b was given, and [...]\n\n\t  Otherwise, if \"--\" is present, treat it like case (1).\n\n\t  Otherwise behave like case (4).\n\n\tcase 4: git checkout <something> <paths>\n\n\t  The first argument must not be ambiguous.\n\t  - If it's *only* a reference, [...]\n\n\n[...]\n> @@ -916,20 +930,28 @@ static int parse_branchname_arg(int argc, const char **argv,\n>  \tif (!strcmp(arg, \"-\"))\n>  \t\targ = \"@{-1}\";\n>  \n> -\tif (get_sha1_mb(arg, rev)) {\n> +\tif (get_sha1_mb(arg, rev)) { /* case (1)? */\n\nThe check means that we are most likely not in case (1), since arg isn't\na commit name, right?\n\n> -\t\tif (has_dash_dash)          /* case (1) */\n> -\t\t\tdie(_(\"invalid reference: %s\"), arg);\n> -\t\tif (dwim_new_local_branch_ok &&\n> -\t\t    !check_filename(NULL, arg) &&\n> -\t\t    argc == 1) {\n> -\t\t\tconst char *remote = unique_tracking_name(arg, rev);\n> -\t\t\tif (!remote)\n> -\t\t\t\treturn argcount;\n> +\t\tint try_dwim = dwim_new_local_branch_ok;\n> +\n> +\t\tif (check_filename(NULL, arg) && !has_dash_dash)\n> +\t\t\ttry_dwim = 0;\n> +\t\t/*\n> +\t\t * Accept \"git checkout foo\" and \"git checkout foo --\"\n> +\t\t * as candidates for dwim.\n> +\t\t */\n> +\t\tif (!(argc == 1 && !has_dash_dash) &&\n> +\t\t    !(argc == 2 && has_dash_dash))\n> +\t\t\ttry_dwim = 0;\n> +\n> +\t\tif (try_dwim) {\n> +\t\t\tconst char *remote = unique_tracking_name(arg, rev);\n> +\t\t\tif (!remote)\n> +\t\t\t\treturn error_invalid_ref(arg, has_dash_dash, argcount);\n\nThis could be simplified by eliminating try_dwim local.\n\nWe are trying case (3) first:\n\n\t\tif (dwim_new_local_branch_ok &&\n\t\t    (argc == 1 || (argc == 2 && has_dash_dash)) &&\n\t\t    (has_dash_dash || !check_filename(NULL, arg))) {\n\t\t\t...\n\nThen can come the \"invalid reference\" check for case (1):\n\n\t\t} else if (has_dash_dash)\t/* case (1) */\n\t\t\tdie(...);\n\nThen case (4).\n\n\t\telse\t/* case (4) */\n\t\t\treturn argcount;\n\n[...]\n> --- a/t/t2024-checkout-dwim.sh\n> +++ b/t/t2024-checkout-dwim.sh\n> @@ -164,4 +164,26 @@ test_expect_success 'checkout of branch from a single remote succeeds #4' '\n>  \ttest_branch_upstream eggs repo_d eggs\n>  '\n>  \n> +test_expect_success 'checkout of branch with a file having the same name fails' '\n> +\tgit checkout -B master &&\n> +\ttest_might_fail git branch -D spam &&\n> +\n> +\t>spam &&\n> +\ttest_must_fail git checkout spam &&\n> +\ttest_must_fail git checkout spam &&\n\nWhy twice?\n\n> +\ttest_must_fail git rev-parse --verify refs/heads/spam &&\n> +\ttest_branch master\n> +'\n> +\n> +test_expect_success 'checkout <branch> -- succeeds, even if a file with the same name exists' '\n> +\tgit checkout -B master &&\n> +\ttest_might_fail git branch -D spam &&\n> +\n> +\t>spam &&\n> +\tgit checkout spam -- &&\n> +\ttest_branch spam &&\n> +\ttest_cmp_rev refs/remotes/extra_dir/repo_c/extra_dir/spam HEAD &&\n> +\ttest_branch_upstream spam repo_c spam\n\nNice.\n\nDo we check that \"git checkout --no-track spam --\" avoids Dscho's\nDWIM?\n\nThanks, and hope that helps,\nJonathan\n"},{"id":"228236","messageId":"20130925224306.GC9464@google.com","threadId":"35021","inReplyTo":"1380137471-26972-2-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --'","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-25T22:43:06Z","receivedAt":"2013-09-25T22:43:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMatthieu Moy wrote:\n\n> error: pathspec 'foo' did not match any file(s) known to git.\n> error: pathspec 'bar' did not match any file(s) known to git.\n> error: pathspec '--' did not match any file(s) known to git.\n>\n> This patch fixes it by walking through the argument list to find the\n> \"--\", and now complains about the number of references given.\n\nGood catch.  Just some nits below.\n\n[...]\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -882,6 +882,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n>  \tunsigned char branch_rev[20];\n>  \tconst char *arg;\n>  \tint has_dash_dash;\n> +\tint i;\n>  \n>  \t/*\n>  \t * case 1: git checkout <ref> -- [<paths>]\n> @@ -925,7 +926,15 @@ static int parse_branchname_arg(int argc, const char **argv,\n>  \t\treturn 1;\n>  \n>  \targ = argv[0];\n> -\thas_dash_dash = (argc > 1) && !strcmp(argv[1], \"--\");\n> +\thas_dash_dash = 0;\n> +\tfor (i = 0; i < argc; i++) {\n> +\t\tif (!strcmp(argv[i], \"--\")) {\n> +\t\t\thas_dash_dash = i;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\tif (has_dash_dash >= 2)\n> +\t\tdie(_(\"only one reference expected, %d given.\"), has_dash_dash);\n\n(The argv[0] == \"--\" case is handled a few lines above.)\n\nAt first I skipped the loop and read this as \"if (there are two or more\n'--' arguments)\".  How about doing one of the following to make it\neasier to read quickly?\n\n (a) rename has_dash_dash here to dash_dash_pos, or\n (b) put the check in the loop, like so:\n\n\thas_dash_dash = 0;\n\tfor (i = 0; i < ...) {\n\t\tif (strcmp(argv[i], \"--\"))\n\t\t\tcontinue;\n\t\tif (i == 0)\t/* case (2) */\n\t\t\treturn 1;\n\t\tif (i > 1)\n\t\t\tdie(_(\"only one reference expected ...);\n\n\t\thas_dash_dash = 1;\n\t\tbreak;\n\t}\n\n[...]\n> --- a/t/t2010-checkout-ambiguous.sh\n> +++ b/t/t2010-checkout-ambiguous.sh\n> @@ -47,4 +47,10 @@ test_expect_success 'disambiguate checking out from a tree-ish' '\n>  \tgit diff --exit-code --quiet\n>  '\n>  \n> +test_expect_success C_LOCALE_OUTPUT 'accurate error message with more than one ref' '\n> +\ttest_must_fail git checkout HEAD master -- 2>actual &&\n> +\techo \"fatal: only one reference expected, 2 given.\" >expect &&\n> +\ttest_cmp expect actual\n\nNits:\n\n - if we change this from 'fatal' to 'error' or reword the message as\n   part of a libification some day, it would be a nuisance to have to\n   update this test.  Maybe a 'grep' could make it more flexible.\n\n - most of the test (though not the interesting part) can run in the\n   !C_LOCALE_OUTPUT case if you use test_i18ncmp or test_i18ngrep\n\n - even the check for \"2\" can run in the !C_LOCALE_OUTPUT case. :)\n   e.g. something like\n\n\ttest_must_fail ... 2>actual &&\n\tgrep 2 actual &&\n\ttest_i18ngrep \"one reference expected, 2 given\" actual\n\nThanks,\nJonathan\n"},{"id":"228261","messageId":"vpq38osm2l6.fsf@anie.imag.fr","threadId":"35021","inReplyTo":"20130925223334.GB9464@google.com","subject":"Re: [PATCH v2 1/2] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-09-26T08:03:49Z","receivedAt":"2013-09-26T08:03:49Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Hi,\n>\n> Matthieu Moy wrote:\n>\n>> The \"--\" notation disambiguates files and branches, but as a side-effect\n>> of the previous implementation, also disabled the branch auto-creation\n>> when $branch does not exist.\n>\n> Hm.  I am not sure that was just an implementation side-effect.\n>\n> Normally 'git checkout <branch> --' means \"Check out that branch,\n> and I mean it!\".  'git checkout -- <pattern>' means \"Check out\n> these paths from the index, and I mean it!\"  'git checkout <blah>'\n> means \"Do what I mean\".\n\nI'm not sure what was the intent, but I to me 'git checkout <branch> --'\nmeans \"I know <branch> is a ref, so don't try to interpret it\notherwise\".\n\n> On the other hand, if I want to do 'git checkout <branch> --'\n> while disabling the \"set up master to track origin/master\" magic,\n> I can use 'git checkout --no-track <branch> --'.  So I think this\n> is a good change.\n\nThere's also --no-guess for that (purposely undocumented according to\nthe commit message of 46148dd7ea).\n\n> More importantly, what's the contract behind this function?  Is there\n> a simpler explanation than \"If argument #2 is true, print a certain\n> message depending on argument #1; otherwise, return argument #3?\".\n> If not, it might be clearer to inline it.\n\nI made a function just because I needed the same pattern twice, and\nhaving a function avoided overly nested if statements.\n\n>> -\tif (get_sha1_mb(arg, rev)) {\n>> +\tif (get_sha1_mb(arg, rev)) { /* case (1)? */\n>\n> The check means that we are most likely not in case (1), since arg isn't\n> a commit name, right?\n\nNot really. We're likely in an incorrect use of case 1, and want to give\nan accurate error message (like \"invalid reference\").\n\n>> +\t\tint try_dwim = dwim_new_local_branch_ok;\n>> +\n>> +\t\tif (check_filename(NULL, arg) && !has_dash_dash)\n>> +\t\t\ttry_dwim = 0;\n>> +\t\t/*\n>> +\t\t * Accept \"git checkout foo\" and \"git checkout foo --\"\n>> +\t\t * as candidates for dwim.\n>> +\t\t */\n>> +\t\tif (!(argc == 1 && !has_dash_dash) &&\n>> +\t\t    !(argc == 2 && has_dash_dash))\n>> +\t\t\ttry_dwim = 0;\n>> +\n>> +\t\tif (try_dwim) {\n>> +\t\t\tconst char *remote = unique_tracking_name(arg, rev);\n>> +\t\t\tif (!remote)\n>> +\t\t\t\treturn error_invalid_ref(arg, has_dash_dash, argcount);\n>\n> This could be simplified by eliminating try_dwim local.\n>\n> We are trying case (3) first:\n>\n> \t\tif (dwim_new_local_branch_ok &&\n> \t\t    (argc == 1 || (argc == 2 && has_dash_dash)) &&\n> \t\t    (has_dash_dash || !check_filename(NULL, arg))) {\n\nI disagree that this is simpler. I introduced try_dwim precisely to\navoid this kind of monster boolean expression, and to leave room for\ncomments in the code.\n\nYou'd also need a \"if (has_dash_dash)\" inside this branch of the if to\ngive an accurate error message for \"git checkout <non-branch> --\".\n\n> [...]\n>> --- a/t/t2024-checkout-dwim.sh\n>> +++ b/t/t2024-checkout-dwim.sh\n>> @@ -164,4 +164,26 @@ test_expect_success 'checkout of branch from a single remote succeeds #4' '\n>>  \ttest_branch_upstream eggs repo_d eggs\n>>  '\n>>  \n>> +test_expect_success 'checkout of branch with a file having the same name fails' '\n>> +\tgit checkout -B master &&\n>> +\ttest_might_fail git branch -D spam &&\n>> +\n>> +\t>spam &&\n>> +\ttest_must_fail git checkout spam &&\n>> +\ttest_must_fail git checkout spam &&\n>\n> Why twice?\n\nOops, fixed.\n\n> Do we check that \"git checkout --no-track spam --\" avoids Dscho's\n> DWIM?\n\nCould be done in addition, but I have no time for this, sorry.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"228263","messageId":"vpqk3i4klgh.fsf@anie.imag.fr","threadId":"35021","inReplyTo":"20130925224306.GC9464@google.com","subject":"Re: [PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --'","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-09-26T08:59:10Z","receivedAt":"2013-09-26T08:59:10Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>  (a) rename has_dash_dash here to dash_dash_pos, or\n>  (b) put the check in the loop, like so:\n\nI agree with (a), but not with (b). I think separating the computation\nof the position and the diagnosis makes it clearer.\n\nI reworked the code a bit, the diagnosis part now looks like\n\n\tif (dash_dash_pos == 0)\n\t\treturn 1; /* case (2) */\n\telse if (dash_dash_pos == 1)\n\t\thas_dash_dash = 1; /* case (3) or (1) */\n\telse if (dash_dash_pos >= 2)\n\t\tdie(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n\n>> +test_expect_success C_LOCALE_OUTPUT 'accurate error message with more than one ref' '\n>> +\ttest_must_fail git checkout HEAD master -- 2>actual &&\n>> +\techo \"fatal: only one reference expected, 2 given.\" >expect &&\n>> +\ttest_cmp expect actual\n>\n> Nits:\n>\n>  - if we change this from 'fatal' to 'error' or reword the message as\n>    part of a libification some day, it would be a nuisance to have to\n>    update this test.  Maybe a 'grep' could make it more flexible.\n>\n>  - most of the test (though not the interesting part) can run in the\n>    !C_LOCALE_OUTPUT case if you use test_i18ncmp or test_i18ngrep\n>\n>  - even the check for \"2\" can run in the !C_LOCALE_OUTPUT case. :)\n>    e.g. something like\n>\n> \ttest_must_fail ... 2>actual &&\n> \tgrep 2 actual &&\n> \ttest_i18ngrep \"one reference expected, 2 given\" actual\n\nOK, I buy your version. Thanks,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"228265","messageId":"vpqa9j0kl9f.fsf@anie.imag.fr","threadId":"35021","inReplyTo":"20130925223334.GB9464@google.com","subject":"Re: [PATCH v2 1/2] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-09-26T09:03:24Z","receivedAt":"2013-09-26T09:03:24Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> \tcase 3: git checkout <something> [--]\n>\n> \t  If <something> is a commit, [...]\n>\n> \t  If <something> is _not_ a commit, either \"--\" is present or\n> \t  <something> is not a path, no -t nor -b was given, and [...]\n>\n> \t  Otherwise, if \"--\" is present, treat it like case (1).\n>\n> \t  Otherwise behave like case (4).\n\nActually, no. Below, we have\n\n\t\t/*\n\t\t * Do not complain the most common case\n\t\t *\tgit checkout branch\n\t\t * even if there happen to be a file called 'branch';\n\t\t * it would be extremely annoying.\n\t\t */\n\nWhich is a subcase of (3). The guess done in (3) is permissive, and the\none done with more arguments (4) is strict.\n\n> \tcase 4: git checkout <something> <paths>\n>\n> \t  The first argument must not be ambiguous.\n\nI wasn't very convinced by your version at first, but I gave it a try.\nI'm not sure the comment is actually better now, but I think it exposes\nthe complexity of the guess, and the difficulty to map the code and the\ncomment more. Other people should take this as an incentive to improve\nthe situation (but I'm really getting short of Git time budget, sorry).\n\n> Then can come the \"invalid reference\" check for case (1):\n>\n> \t\t} else if (has_dash_dash)\t/* case (1) */\n> \t\t\tdie(...);\n>\n> Then case (4).\n>\n> \t\telse\t/* case (4) */\n> \t\t\treturn argcount;\n\nActually, the comments are wrong here. The comment <-> code mapping is\nmore complex than this. At this point of the code, you know that the\nfirst argument is not a commit, and that dwim has been ruled-out. But\nthat may be case 3 also:\n\ngit checkout --no-guess <non-branch> [--]\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}