{"thread":{"id":"35016","subject":"[RFC/PATCH] checkout: allow dwim for branch creation for \"git checkout $branch --\"","startedAt":"2013-09-25T12:49:09Z","lastAt":"2013-10-17T20:47:53Z","messageCount":7,"participants":["Matthieu Moy","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"228201","messageId":"1380113349-19838-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"35016","inReplyTo":null,"subject":"[RFC/PATCH] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-09-25T12:49:09Z","receivedAt":"2013-09-25T12:49:09Z","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 builtin/checkout.c       | 32 ++++++++++++++++++++++++--------\n t/t2024-checkout-dwim.sh | 22 ++++++++++++++++++++++\n 2 files changed, 46 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 0f57397..f1f5915 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@@ -916,20 +924,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.475.g703937e.dirty\n"},{"id":"228203","messageId":"CACsJy8AbBrVSz=p+ARxvR_QXtku1BxbtQPv7pz_QCveeUEtwCQ@mail.gmail.com","threadId":"35016","inReplyTo":"1380113349-19838-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [RFC/PATCH] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-25T13:22:39Z","receivedAt":"2013-09-25T13:22:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 25, 2013 at 7:49 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:\n>  static int parse_branchname_arg(int argc, const char **argv,\n>                                 int dwim_new_local_branch_ok,\n>                                 struct branch_info *new,\n\nYou may want to update the big comment block about all cases at the\nbeginning of this function.\n\n> @@ -916,20 +924,28 @@ static int parse_branchname_arg(int argc, const char **argv,\n>         if (!strcmp(arg, \"-\"))\n>                 arg = \"@{-1}\";\n>\n> -       if (get_sha1_mb(arg, rev)) {\n> -               if (has_dash_dash)          /* case (1) */\n> -                       die(_(\"invalid reference: %s\"), arg);\n> -               if (dwim_new_local_branch_ok &&\n> -                   !check_filename(NULL, arg) &&\n> -                   argc == 1) {\n> +       if (get_sha1_mb(arg, rev)) { /* case (1)? */\n> +               int try_dwim = dwim_new_local_branch_ok;\n> +\n> +               if (check_filename(NULL, arg) && !has_dash_dash)\n> +                       try_dwim = 0;\n> +               /*\n> +                * Accept \"git checkout foo\" and \"git checkout foo --\"\n> +                * as candidates for dwim.\n> +                */\n> +               if (!(argc == 1 && !has_dash_dash) &&\n\nhas_dash_dash is calculated as (argc > 1) && !strcmp(argv[1], \"--\"),\nso when argc == 1, the has_dash_dash must be zero, the \"&&\n!has_dash_dash\" is redundant. But it makes me wonder what we do with\n\"git checkout abc def -- xyz\". I think both old and new code do not\nsee \"--\" as a separator like it should (and should also error about\nmultiple refs given). Maybe something worth fixing too while you're\nupdating this code.\n\n> +                   !(argc == 2 && has_dash_dash))\n> +                       try_dwim = 0;\n> +\n-- \nDuy\n"},{"id":"228206","messageId":"vpqob7huhyw.fsf@anie.imag.fr","threadId":"35016","inReplyTo":"CACsJy8AbBrVSz=p+ARxvR_QXtku1BxbtQPv7pz_QCveeUEtwCQ@mail.gmail.com","subject":"Re: [RFC/PATCH] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-09-25T13:52:07Z","receivedAt":"2013-09-25T13:52:07Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> has_dash_dash is calculated as (argc > 1) && !strcmp(argv[1], \"--\"),\n> so when argc == 1, the has_dash_dash must be zero, the \"&&\n> !has_dash_dash\" is redundant.\n\nYes, but I'd rather not have to read the detailed definition of\nhas_dash_dash to understand the code. With my version, the name of the\nvariable is actually sufficient to understand.\n\n> But it makes me wonder what we do with \"git checkout abc def -- xyz\".\n\nOuch. Both old and new say\n\n$ git checkout foo bar --\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\nFortunately, this is easy to fix, by adding this on top (or before, it\ndoesn't matter):\n\ncommit 060cf582f5e848c5ca57231d293943a5489c5433 (HEAD, master)\nAuthor: Matthieu Moy <Matthieu.Moy@imag.fr>\nDate:   Wed Sep 25 15:41:34 2013 +0200\n\n    checkout: proper error message on 'git checkout foo bar --'\n    \n    The previous code was detecting the presence of \"--\" by looking only at\n    argument 1. As a result, \"git checkout foo bar --\" was interpreted as an\n    ambiguous file/revision list, and errored out with:\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\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a5a12f6..d958d60 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        unsigned char branch_rev[20];\n        const char *arg;\n        int has_dash_dash;\n+       int i;\n \n        /*\n         * case 1: git checkout <ref> -- [<paths>]\n@@ -925,7 +926,15 @@ static int parse_branchname_arg(int argc, const char **argv,\n                return 1;\n \n        arg = argv[0];\n-       has_dash_dash = (argc > 1) && !strcmp(argv[1], \"--\");\n+       has_dash_dash = 0;\n+       for (i = 0; i < argc; i++) {\n+               if (!strcmp(argv[i], \"--\")) {\n+                       has_dash_dash = i;\n+                       break;\n+               }\n+       }\n+       if (has_dash_dash >= 2)\n+               die(\"only one reference expected, %d given.\", has_dash_dash);\n \n        if (!strcmp(arg, \"-\"))\n                arg = \"@{-1}\";\ndiff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh\nindex 7cc0a35..2836a3e 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        git diff --exit-code --quiet\n '\n \n+test_expect_success 'accurate error message with more than one ref' '\n+       test_must_fail git checkout HEAD master -- 2>actual &&\n+       echo \"fatal: only one reference expected, 2 given.\" >expect &&\n+       test_cmp expect actual\n+'\n+\n test_done\n\nI'll resend, together with tweaks to the first patch.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"228224","messageId":"CACsJy8Aj0HtuWn6iW4B3y4ZCeZYGOPN=vNGsC85YXTVjbwivBQ@mail.gmail.com","threadId":"35016","inReplyTo":"vpqob7huhyw.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-25T19:11:43Z","receivedAt":"2013-09-25T19:11:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 25, 2013 at 8:52 PM, Matthieu Moy\n<Matthieu.Moy@grenoble-inp.fr> wrote:\n> @@ -925,7 +926,15 @@ static int parse_branchname_arg(int argc, const char **argv,\n>                 return 1;\n>\n>         arg = argv[0];\n> -       has_dash_dash = (argc > 1) && !strcmp(argv[1], \"--\");\n> +       has_dash_dash = 0;\n> +       for (i = 0; i < argc; i++) {\n> +               if (!strcmp(argv[i], \"--\")) {\n> +                       has_dash_dash = i;\n> +                       break;\n> +               }\n> +       }\n> +       if (has_dash_dash >= 2)\n> +               die(\"only one reference expected, %d given.\", has_dash_dash);\n\nThe translator in me says this string should be marked for translation\nlike others in git-checkout...\n\n>\n>         if (!strcmp(arg, \"-\"))\n>                 arg = \"@{-1}\";\n> diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh\n> index 7cc0a35..2836a3e 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>         git diff --exit-code --quiet\n>  '\n>\n> +test_expect_success 'accurate error message with more than one ref' '\n> +       test_must_fail git checkout HEAD master -- 2>actual &&\n> +       echo \"fatal: only one reference expected, 2 given.\" >expect &&\n> +       test_cmp expect actual\n> +'\n> +\n>  test_done\n\nwhich makes C_LOCALE_OUTPUT a prerequisite for this test because it\nneeds the untranslated version of the string.\n\n>\n> I'll resend, together with tweaks to the first patch.\n>\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n\n\n\n-- \nDuy\n"},{"id":"229093","messageId":"xmqq7gdc6gl8.fsf@gitster.dls.corp.google.com","threadId":"35016","inReplyTo":"vpqob7huhyw.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-16T21:39:15Z","receivedAt":"2013-10-16T21:39:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> has_dash_dash is calculated as (argc > 1) && !strcmp(argv[1], \"--\"),\n>> so when argc == 1, the has_dash_dash must be zero, the \"&&\n>> !has_dash_dash\" is redundant.\n>\n> Yes, but I'd rather not have to read the detailed definition of\n> has_dash_dash to understand the code. With my version, the name of the\n> variable is actually sufficient to understand.\n>\n>> But it makes me wonder what we do with \"git checkout abc def -- xyz\".\n>\n> Ouch. Both old and new say\n> ...\n> I'll resend, together with tweaks to the first patch.\n\nDid anything further happen to this discussion?  Is v3 the version\nwith agreement among the list members I just should pick up?\n"},{"id":"229117","messageId":"vpqr4bkqhfg.fsf@anie.imag.fr","threadId":"35016","inReplyTo":"xmqq7gdc6gl8.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-10-17T11:13:07Z","receivedAt":"2013-10-17T11:13:07Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Did anything further happen to this discussion?  Is v3 the version\n> with agreement among the list members I just should pick up?\n\nv3\n( http://thread.gmane.org/gmane.comp.version-control.git/235409/focus=235408 )\nis the last version I sent, and I got no feedback on it, so I guess it's\nready for you to pick.\n\nThanks,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"229139","messageId":"xmqqvc0vy686.fsf@gitster.dls.corp.google.com","threadId":"35016","inReplyTo":"vpqr4bkqhfg.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH] checkout: allow dwim for branch creation for \"git checkout $branch --\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-17T20:47:53Z","receivedAt":"2013-10-17T20:47:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> v3\n> ( http://thread.gmane.org/gmane.comp.version-control.git/235409/focus=235408 )\n> is the last version I sent, and I got no feedback on it, so I guess it's\n> ready for you to pick.\n\nThanks; done with s/pick/nitpick/ ;-).\n"}]}