{"thread":{"id":"35162","subject":"[PATCH v4 2/2] checkout: proper error message on 'git checkout foo bar --'","startedAt":"2013-10-18T09:25:57Z","lastAt":"2013-10-18T09:25:58Z","messageCount":2,"participants":["Matthieu Moy"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"229159","messageId":"1382088358-2213-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"35162","inReplyTo":null,"subject":"[PATCH v4 1/2] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-10-18T09:25:57Z","receivedAt":"2013-10-18T09:25:57Z","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---\n> So it may be even cleaner to read if you did it this way:\n\nIndeed. The code reads better now.\n\n builtin/checkout.c       | 78 ++++++++++++++++++++++++++++++++++--------------\n t/t2024-checkout-dwim.sh | 21 +++++++++++++\n 2 files changed, 76 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 0f57397..2003795 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -885,20 +885,30 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t *\n \t *   everything after the '--' must be paths.\n \t *\n-\t * case 3: git checkout <something> [<paths>]\n+\t * case 3: git checkout <something> [--]\n \t *\n-\t *   With no paths, if <something> is a commit, that is to\n-\t *   switch to the branch or detach HEAD at it.  As a special case,\n-\t *   if <something> is A...B (missing A or B means HEAD but you can\n-\t *   omit at most one side), and if there is a unique merge base\n-\t *   between A and B, A...B names that merge base.\n+\t *   (a) If <something> is a commit, that is to\n+\t *       switch to the branch or detach HEAD at it.  As a special case,\n+\t *       if <something> is A...B (missing A or B means HEAD but you can\n+\t *       omit at most one side), and if there is a unique merge base\n+\t *       between A and B, A...B names that merge base.\n \t *\n-\t *   With no paths, if <something> is _not_ a commit, no -t nor -b\n-\t *   was given, and there is a tracking branch whose name is\n-\t *   <something> in one and only one remote, then this is a short-hand\n-\t *   to fork local <something> from that remote-tracking branch.\n+\t *   (b) If <something> is _not_ a commit, either \"--\" is present\n+\t *       or <something> is not a path, no -t nor -b was given, and\n+\t *       and there is a tracking branch whose name is <something>\n+\t *       in one and only one remote, then this is a short-hand to\n+\t *       fork local <something> from that remote-tracking branch.\n \t *\n-\t *   Otherwise <something> shall not be ambiguous.\n+\t *   (c) Otherwise, if \"--\" is present, treat it like case (1).\n+\t *\n+\t *   (d) Otherwise :\n+\t *       - if it's a reference, treat it like case (1)\n+\t *       - else if it's a path, treat it like case (2)\n+\t *       - else: fail.\n+\t *\n+\t * case 4: git checkout <something> <paths>\n+\t *\n+\t *   The first argument must not be ambiguous.\n \t *   - If it's *only* a reference, treat it like case (1).\n \t *   - If it's only a path, treat it like case (2).\n \t *   - else: fail.\n@@ -917,18 +927,40 @@ static int parse_branchname_arg(int argc, const char **argv,\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+\t\t/*\n+\t\t * Either case (3) or (4), with <something> not being\n+\t\t * a commit, or an attempt to use case (1) with an\n+\t\t * invalid ref.\n+\t\t *\n+\t\t * It's likely an error, but we need to find out if\n+\t\t * we should auto-create the branch, case (3).(b).\n+\t\t */\n+\t\tint recover_with_dwim = dwim_new_local_branch_ok;\n+\n+\t\tif (check_filename(NULL, arg) && !has_dash_dash)\n+\t\t\trecover_with_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\trecover_with_dwim = 0;\n+\n+\t\tif (recover_with_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*new_branch = arg;\n-\t\t\targ = remote;\n-\t\t\t/* DWIMmed to create local branch */\n-\t\t} else {\n+\t\t\tif (remote) {\n+\t\t\t\t*new_branch = arg;\n+\t\t\t\targ = remote;\n+\t\t\t\t/* DWIMmed to create local branch, case (3).(b) */\n+\t\t\t} else {\n+\t\t\t\trecover_with_dwim = 0;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (!recover_with_dwim) {\n+\t\t\tif (has_dash_dash)\n+\t\t\t\tdie(_(\"invalid reference: %s\"), arg);\n \t\t\treturn argcount;\n \t\t}\n \t}\n@@ -958,7 +990,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \n \tif (!*source_tree)                   /* case (1): want a tree */\n \t\tdie(_(\"reference is not a tree: %s\"), arg);\n-\tif (!has_dash_dash) {/* case (3 -> 1) */\n+\tif (!has_dash_dash) {/* case (3).(d) -> (1) */\n \t\t/*\n \t\t * Do not complain the most common case\n \t\t *\tgit checkout branch\ndiff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\nindex 094b92e..6ecb559 100755\n--- a/t/t2024-checkout-dwim.sh\n+++ b/t/t2024-checkout-dwim.sh\n@@ -164,4 +164,25 @@ 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 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.479.g0ed768e\n"},{"id":"229158","messageId":"1382088358-2213-2-git-send-email-Matthieu.Moy@imag.fr","threadId":"35162","inReplyTo":"1382088358-2213-1-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH v4 2/2] checkout: proper error message on 'git checkout foo bar --'","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-10-18T09:25:58Z","receivedAt":"2013-10-18T09:25:58Z","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---\nUnchanged since v3.\n\n builtin/checkout.c            | 21 ++++++++++++++++-----\n t/t2010-checkout-ambiguous.sh |  6 ++++++\n 2 files changed, 22 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2003795..54f80bd 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -873,7 +873,9 @@ static int parse_branchname_arg(int argc, const char **argv,\n \tint argcount = 0;\n \tunsigned char branch_rev[20];\n \tconst char *arg;\n-\tint has_dash_dash;\n+\tint dash_dash_pos;\n+\tint has_dash_dash = 0;\n+\tint i;\n \n \t/*\n \t * case 1: git checkout <ref> -- [<paths>]\n@@ -917,11 +919,20 @@ static int parse_branchname_arg(int argc, const char **argv,\n \tif (!argc)\n \t\treturn 0;\n \n-\tif (!strcmp(argv[0], \"--\"))\t/* case (2) */\n-\t\treturn 1;\n-\n \targ = argv[0];\n-\thas_dash_dash = (argc > 1) && !strcmp(argv[1], \"--\");\n+\tdash_dash_pos = -1;\n+\tfor (i = 0; i < argc; i++) {\n+\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\tdash_dash_pos = i;\n+\t\t\tbreak;\n+\t\t}\n+\t}\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 \tif (!strcmp(arg, \"-\"))\n \t\targ = \"@{-1}\";\ndiff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh\nindex 7cc0a35..87bdf9c 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 'accurate error message with more than one ref' '\n+\ttest_must_fail git checkout HEAD master -- 2>actual &&\n+\tgrep 2 actual &&\n+\ttest_i18ngrep \"one reference expected, 2 given\" actual\n+'\n+\n test_done\n-- \n1.8.4.479.g0ed768e\n"}]}