{"thread":{"id":"35025","subject":"[PATCH v3 2/2] checkout: proper error message on 'git checkout foo bar --'","startedAt":"2013-09-26T09:08:05Z","lastAt":"2013-10-17T18:09:37Z","messageCount":3,"participants":["Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"228267","messageId":"1380186486-8220-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"35025","inReplyTo":null,"subject":"[PATCH v3 1/2] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-09-26T09:08:05Z","receivedAt":"2013-09-26T09:08:05Z","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 v2: essentially Jonathan's comments.\n\n builtin/checkout.c       | 70 ++++++++++++++++++++++++++++++++++--------------\n t/t2024-checkout-dwim.sh | 21 +++++++++++++++\n 2 files changed, 71 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 0f57397..9edd9c3 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -863,6 +863,13 @@ 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+\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@@ -885,20 +892,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 *   (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 *   (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 *   (c) Otherwise, if \"--\" is present, treat it like case (1).\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 *   (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 *   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 * case 4: git checkout <something> <paths>\n \t *\n-\t *   Otherwise <something> shall not be ambiguous.\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,19 +934,32 @@ 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\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\t/* DWIMmed to create local branch, case (3).(b) */\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 \n@@ -958,7 +988,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.474.g128a96c\n"},{"id":"228266","messageId":"1380186486-8220-2-git-send-email-Matthieu.Moy@imag.fr","threadId":"35025","inReplyTo":"1380186486-8220-1-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH v3 2/2] checkout: proper error message on 'git checkout foo bar --'","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-09-26T09:08:06Z","receivedAt":"2013-09-26T09:08:06Z","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---\nAgain, Jonathan's comments (partly) applied since v2.\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 9edd9c3..ef6b56a 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -880,7 +880,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@@ -924,11 +926,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.474.g128a96c\n"},{"id":"229125","messageId":"xmqqr4bj22hq.fsf@gitster.dls.corp.google.com","threadId":"35025","inReplyTo":"1380186486-8220-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH v3 1/2] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-17T18:09:37Z","receivedAt":"2013-10-17T18:09:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 0f57397..9edd9c3 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -863,6 +863,13 @@ 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> +\treturn argcount;\n> +}\n\nThis is somewhat unfortunate; it pretends to be a reusable helper by\nbeing a separate function, but it is not very reusable (see below).\n\n> @@ -917,19 +934,32 @@ static int parse_branchname_arg(int argc, const char **argv,\n>  \t\targ = \"@{-1}\";\n>  \n>  \tif (get_sha1_mb(arg, rev)) {\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\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\nUp to this point, the updated code makes very good sense.\n\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\nThe original that returned \"argcount\" from here were unnecessarily\nmisleading in the first place. It saw \"git checkout foo\" where \"foo\"\ndoes not refer to an object nor a filesystem entity and there was no\nunique \"refs/remotes/*/foo\"; it wanted to return 0 to tell the\ncaller that it consumed zero arguments as branch names.\n\nAnd the updated code is even more obscure.  This calling site makes\nit look as if it is an error to have no unique \"refs/remotes/*/foo\"\nat this point of the code by naming the helper function \"error_*()\",\nbut it is an error in some case and not in others.\n\n                if (!remote) {\n                        if (has_dash_dash)\n                                die(_(\"...\"));\n                        return 0;\n                }\n\nwould be a lot more understandable.\n\t\t\nThe only reason you have conditional die() here (and on the \"else\"\nside of this \"if\" statement) is because you delayed the die that was\nat a much earlier point in the original.  And the only reason you\ncreated the unfortunate helper function is because you need to deal\nwith that delayed decision to die now in two places.\n\nSo it may be even cleaner to read if you did it this way:\n\n\tif (get_sha1_mb(...)) {\n\t\t/*\n\t\t * The first token is not a valid rev; we should\n\t\t * ordinarily error out if \"git checkout foo --\"\n\t\t * if foo is not a valid rev, but first see if\n\t\t * we can auto-create foo to continue...\n\t\t */\n\t\tint recover_with_dwim = dwim_new_local_branch_ok;\n\n\t\t... decide if we want to recover_with_dwim ...\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\t*new_branch = arg;\n\t\t\t\targ = remote;\n\t\t\t} else {\n\t\t\t\t/* no; arg cannot be salvaged */\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 ref %s\", arg);\n\t\t\treturn 0; /* we saw no branch/commit */\n\t\t}\n\t\t/* otherwise we made a successful recovery */\n\t}\n"}]}