{"thread":{"id":"49792","subject":"[PATCH] checkout: disambiguate dwim tracking branches and local files","startedAt":"2018-11-10T12:07:16Z","lastAt":"2018-11-13T17:52:37Z","messageCount":6,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"362890","messageId":"20181110120707.25846-1-pclouds@gmail.com","threadId":"49792","inReplyTo":null,"subject":"[PATCH] checkout: disambiguate dwim tracking branches and local files","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-10T12:07:07Z","receivedAt":"2018-11-10T12:07:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When checkout dwim is added in [1], it is restricted to only dwim when\ncertain conditions are met and fall back to default checkout behavior\notherwise. It turns out falling back could be confusing. One of the\nconditions to turn\n\n    git checkout frotz\n\nto\n\n    git checkout -b frotz origin/frotz\n\nis that frotz must not exist as a file. But when the user comes to\nexpect \"git checkout frotz\" to create the branch \"frotz\" and there\nhappens to be a file named \"frotz\", git's silently reverting \"frotz\"\nfile content is not helping. This is reported in Git mailing list [2]\nand even used as an example of \"Git is bad\" elsewhere [3].\n\nWe normally try to do the right thing, but when there are multiple\n\"right things\" to do, it's best to leave it to the user to decide.\nCheck this case, ask the user to use \"--\" to disambiguate.\n\n[1] 70c9ac2f19 (DWIM \"git checkout frotz\" to \"git checkout -b frotz\n    origin/frotz\" - 2009-10-18)\n[2] https://public-inbox.org/git/CACsJy8B2TVr1g+k+eSQ=pBEO3WN4_LtgLo9gpur8X7Z9GOFL_A@mail.gmail.com/\n[3] https://news.ycombinator.com/item?id=18230655\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/checkout.c       | 15 ++++++++++++---\n t/t2024-checkout-dwim.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex acdafc6e4c..17d48166d1 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1079,9 +1079,15 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t */\n \t\tint recover_with_dwim = dwim_new_local_branch_ok;\n \n-\t\tif (!has_dash_dash &&\n-\t\t    (check_filename(opts->prefix, arg) || !no_wildcard(arg)))\n-\t\t\trecover_with_dwim = 0;\n+\t\t/*\n+\t\t * If both refs/remotes/origin/master and the file\n+\t\t * 'master'. Don't blindly go for 'master' file\n+\t\t * because it's ambiguous. Leave it for the user to\n+\t\t * decide.\n+\t\t */\n+\t\tint disambi_local_file = !has_dash_dash &&\n+\t\t\t(check_filename(opts->prefix, arg) || !no_wildcard(arg));\n+\n \t\t/*\n \t\t * Accept \"git checkout foo\" and \"git checkout foo --\"\n \t\t * as candidates for dwim.\n@@ -1094,6 +1100,9 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t\tconst char *remote = unique_tracking_name(arg, rev,\n \t\t\t\t\t\t\t\t  dwim_remotes_matched);\n \t\t\tif (remote) {\n+\t\t\t\tif (disambi_local_file)\n+\t\t\t\t\tdie(_(\"'%s' could be both a local file and a tracking branch.\\n\"\n+\t\t\t\t\t      \"Please use -- to disambiguate\"), arg);\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) */\ndiff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\nindex 69b6774d10..fa0718c730 100755\n--- a/t/t2024-checkout-dwim.sh\n+++ b/t/t2024-checkout-dwim.sh\n@@ -278,4 +278,35 @@ test_expect_success 'loosely defined local base branch is reported correctly' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'reject when arg could be part of dwim branch' '\n+\tgit remote add foo file://non-existent-place &&\n+\tgit update-ref refs/remotes/foo/dwim-arg HEAD &&\n+\techo foo >dwim-arg &&\n+\tgit add dwim-arg &&\n+\techo bar >dwim-arg &&\n+\ttest_must_fail git checkout dwim-arg &&\n+\ttest_must_fail git rev-parse refs/heads/dwim-arg -- &&\n+\tgrep bar dwim-arg\n+'\n+\n+test_expect_success 'disambiguate dwim branch and checkout path (1)' '\n+\tgit update-ref refs/remotes/foo/dwim-arg1 HEAD &&\n+\techo foo >dwim-arg1 &&\n+\tgit add dwim-arg1 &&\n+\techo bar >dwim-arg1 &&\n+\tgit checkout -- dwim-arg1 &&\n+\ttest_must_fail git rev-parse refs/heads/dwim-arg1 -- &&\n+\tgrep foo dwim-arg1\n+'\n+\n+test_expect_success 'disambiguate dwim branch and checkout path (2)' '\n+\tgit update-ref refs/remotes/foo/dwim-arg2 HEAD &&\n+\techo foo >dwim-arg2 &&\n+\tgit add dwim-arg2 &&\n+\techo bar >dwim-arg2 &&\n+\tgit checkout dwim-arg2 -- &&\n+\tgit rev-parse refs/heads/dwim-arg2 -- &&\n+\tgrep bar dwim-arg2\n+'\n+\n test_done\n-- \n2.19.1.1231.g84aef82467\n\n"},{"id":"362967","messageId":"xmqq36s63idd.fsf@gitster-ct.c.googlers.com","threadId":"49792","inReplyTo":"20181110120707.25846-1-pclouds@gmail.com","subject":"Re: [PATCH] checkout: disambiguate dwim tracking branches and local files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-12T06:44:14Z","receivedAt":"2018-11-12T06:44:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> @@ -1079,9 +1079,15 @@ static int parse_branchname_arg(int argc, const char **argv,\n>  \t\t */\n>  \t\tint recover_with_dwim = dwim_new_local_branch_ok;\n>  \n> -\t\tif (!has_dash_dash &&\n> -\t\t    (check_filename(opts->prefix, arg) || !no_wildcard(arg)))\n> -\t\t\trecover_with_dwim = 0;\n> +\t\t/*\n> +\t\t * If both refs/remotes/origin/master and the file\n> +\t\t * 'master'. Don't blindly go for 'master' file\n> +\t\t * because it's ambiguous. Leave it for the user to\n> +\t\t * decide.\n> +\t\t */\n> +\t\tint disambi_local_file = !has_dash_dash &&\n> +\t\t\t(check_filename(opts->prefix, arg) || !no_wildcard(arg));\n\nWhat you are computing on the right hand side is if the arg is\nambiguous.  And the code that looks at this variable does not\ndisambiguate, but it just punts and makes it responsibility to the\nuser (which is by the way the correct thing to do).\n\nWhen a file with exact name is in the working tree, we do not\ndeclare it is a pathspec and not a rev, as we may be allowed to dwim\nand create a rev with that name and at that point we'd be in an\nambigous situation.  If the arg _has_ wildcard, however, it is a\nstrong sign that it *is* a pathspec, isn't it?  It is iffy that a\nsingle variable that cannot be used to distinguish these two cases\nis introduced---one of these cases will be mishandled.\n\nAlso how does the patch ensures that this new logic does not kick in\nfor those who refuse to let the command dwim to create a new branch?\n\n>  \t\t/*\n>  \t\t * Accept \"git checkout foo\" and \"git checkout foo --\"\n>  \t\t * as candidates for dwim.\n> @@ -1094,6 +1100,9 @@ static int parse_branchname_arg(int argc, const char **argv,\n>  \t\t\tconst char *remote = unique_tracking_name(arg, rev,\n>  \t\t\t\t\t\t\t\t  dwim_remotes_matched);\n>  \t\t\tif (remote) {\n> +\t\t\t\tif (disambi_local_file)\n> +\t\t\t\t\tdie(_(\"'%s' could be both a local file and a tracking branch.\\n\"\n> +\t\t\t\t\t      \"Please use -- to disambiguate\"), arg);\n\nAh, the only user of this variable triggers when recover_with_dwim\nis true, so that is how dwim-less case is handled, OK.\n\nThat still leaves the question if it is OK to handle these two cases\nthe same way in a repository without 'next' branch whose origin has\none:\n\n    $ >next; git checkout --guess next\n    $ >next; git checkout --guess 'n??t'\n\nPerhaps the variable should be named \"local_file_crashes_with_rev\"\nand its the scope narrowed to the same block as \"remote\" is\ncomputed.  Or just remove the variable and check the condition right\nthere where you need to.  I.e.\n\n\tif (remote) {\n\t\tif (!has_dash_dash &&\n\t\t    check_filename(opts->prefix, arg))\n\t\t\tdie(\"did you mean a branch or path?: '%s'\", arg);\n\t\t...\n\n"},{"id":"363063","messageId":"CACsJy8DZy2hozNypxizF=LmJn8gw9z5HGhV0paiMuM9+gxYVvg@mail.gmail.com","threadId":"49792","inReplyTo":"xmqq36s63idd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] checkout: disambiguate dwim tracking branches and local files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-12T17:26:15Z","receivedAt":"2018-11-12T17:26:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Nov 12, 2018 at 7:44 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n> > @@ -1079,9 +1079,15 @@ static int parse_branchname_arg(int argc, const char **argv,\n> >                */\n> >               int recover_with_dwim = dwim_new_local_branch_ok;\n> >\n> > -             if (!has_dash_dash &&\n> > -                 (check_filename(opts->prefix, arg) || !no_wildcard(arg)))\n> > -                     recover_with_dwim = 0;\n> > +             /*\n> > +              * If both refs/remotes/origin/master and the file\n> > +              * 'master'. Don't blindly go for 'master' file\n> > +              * because it's ambiguous. Leave it for the user to\n> > +              * decide.\n> > +              */\n> > +             int disambi_local_file = !has_dash_dash &&\n> > +                     (check_filename(opts->prefix, arg) || !no_wildcard(arg));\n>\n> What you are computing on the right hand side is if the arg is\n> ambiguous.  And the code that looks at this variable does not\n> disambiguate, but it just punts and makes it responsibility to the\n> user (which is by the way the correct thing to do).\n>\n> When a file with exact name is in the working tree, we do not\n> declare it is a pathspec and not a rev, as we may be allowed to dwim\n> and create a rev with that name and at that point we'd be in an\n> ambigous situation.  If the arg _has_ wildcard, however, it is a\n> strong sign that it *is* a pathspec, isn't it?  It is iffy that a\n> single variable that cannot be used to distinguish these two cases\n> is introduced---one of these cases will be mishandled.\n\nIs it that different between an exact path name and a pathspec?\nSuppose it is a pathspec (with wildcards) that matches some paths, and\nwe happen to have the remote branch origin/<that-pathspec>, then it is\nstill ambiguous whether we should go create branch\n\"<that-pathspec>\" or go check out the paths matched by the pathspec.\n\n> Also how does the patch ensures that this new logic does not kick in\n> for those who refuse to let the command dwim to create a new branch?\n\nI would hope that it would be \"--\" to settle all disputes. But it\nlooks like \"git checkout foo --\" is explicitly a candidate for dwim.\nWe have a hidden option --no-guess to disable dwim. Maybe it's a good\nidea to make that one visible. It's at least clearer than using \"--\"\neven if \"--\" worked on this case.\n\n>\n> >               /*\n> >                * Accept \"git checkout foo\" and \"git checkout foo --\"\n> >                * as candidates for dwim.\n> > @@ -1094,6 +1100,9 @@ static int parse_branchname_arg(int argc, const char **argv,\n> >                       const char *remote = unique_tracking_name(arg, rev,\n> >                                                                 dwim_remotes_matched);\n> >                       if (remote) {\n> > +                             if (disambi_local_file)\n> > +                                     die(_(\"'%s' could be both a local file and a tracking branch.\\n\"\n> > +                                           \"Please use -- to disambiguate\"), arg);\n>\n> Ah, the only user of this variable triggers when recover_with_dwim\n> is true, so that is how dwim-less case is handled, OK.\n>\n> That still leaves the question if it is OK to handle these two cases\n> the same way in a repository without 'next' branch whose origin has\n> one:\n>\n>     $ >next; git checkout --guess next\n>     $ >next; git checkout --guess 'n??t'\n>\n> Perhaps the variable should be named \"local_file_crashes_with_rev\"\n> and its the scope narrowed to the same block as \"remote\" is\n> computed.  Or just remove the variable and check the condition right\n> there where you need to.  I.e.\n>\n>         if (remote) {\n>                 if (!has_dash_dash &&\n>                     check_filename(opts->prefix, arg))\n>                         die(\"did you mean a branch or path?: '%s'\", arg);\n>                 ...\n>\n\nI still think handing both cases the same way is right. In the second\ncase we would not find 'origin/n??t' so we fall back to checking out\npathspec 'n??t' anyway (expected from you, I think). But just in case\nthe remote branch 'origin/n??t' exists, we kick and scream.\n\nI think you see 'n??t' as a pathspec, but I'm thinking about a user\nwho sees 'n??t' as a branch name, not pathspec and he would have a\ndifferent expectation.\n-- \nDuy\n"},{"id":"363078","messageId":"xmqq36s613p8.fsf@gitster-ct.c.googlers.com","threadId":"49792","inReplyTo":"CACsJy8DZy2hozNypxizF=LmJn8gw9z5HGhV0paiMuM9+gxYVvg@mail.gmail.com","subject":"Re: [PATCH] checkout: disambiguate dwim tracking branches and local files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-12T19:44:03Z","receivedAt":"2018-11-12T19:44:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> Is it that different between an exact path name and a pathspec?\n> Suppose it is a pathspec (with wildcards) that matches some paths, and\n> we happen to have the remote branch origin/<that-pathspec>, then it is\n> still ambiguous whether we should go create branch\n> \"<that-pathspec>\" or go check out the paths matched by the pathspec.\n:\nA huge difference between these two\n\n\t$ echo Hello >'n??t' && git add 'n??t'\n\t$ git branch 'n??t'\n\nis that the former is taken and the latter is rejected, even though\nneither of them is particularly a sane or a likely thing.\n\nIsn't that a good enough reason why\n\n\t$ git checkout 'n??t'\n\ncannot mean checking 'n??t' branch out, be it either an existing\nlocal one or auto-vivified one out of a remote-tracking branch?\n\n> I think you see 'n??t' as a pathspec, but I'm thinking about a user\n> who sees 'n??t' as a branch name, not pathspec and he would have a\n> different expectation.\n\nSee above for the reason why I think there is no room for different\nexpectations to come in the picture in this case.\n\n"},{"id":"363180","messageId":"20181113175226.25635-1-pclouds@gmail.com","threadId":"49792","inReplyTo":"20181110120707.25846-1-pclouds@gmail.com","subject":"[PATCH v2 0/1] disambiguate dwim tracking branches and local files","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-13T17:52:25Z","receivedAt":"2018-11-13T17:52:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v2 leaves \"pathspec with wildcard\" case alone. The behavior in this\ncase remains as before.\n\n--no-guess is now made visible in \"git checkout -h\" and the man page.\n\n\nPS. Based on git-checkout.txt I don't think any user can work out that\n\"git checkout branch --\" can be used to disambiguate. And updating the\ndoc to show that makes it a lot uglier.\n\nPerhaps it's time we add two new semi-aliases, switch-branch and\nrestore-path. They will have the same syntax as checkout in their\nrespective use case, without ambiguation. Semi-aliases because like\nbuiltin commands, they cannot be overriden by user aliases.\n\nNguyễn Thái Ngọc Duy (1):\n  checkout: disambiguate dwim tracking branches and local files\n\n Documentation/git-checkout.txt |  4 ++++\n builtin/checkout.c             | 18 +++++++++++++-----\n t/t2024-checkout-dwim.sh       | 31 +++++++++++++++++++++++++++++++\n t/t9902-completion.sh          |  3 ++-\n 4 files changed, 50 insertions(+), 6 deletions(-)\n\nRange-diff against v1:\n1:  530f5d8f03 ! 1:  0408fdde4d checkout: disambiguate dwim tracking branches and local files\n    @@ -21,12 +21,36 @@\n     \n         We normally try to do the right thing, but when there are multiple\n         \"right things\" to do, it's best to leave it to the user to decide.\n    -    Check this case, ask the user to use \"--\" to disambiguate.\n    +    Check this case, ask the user to to disambiguate:\n    +\n    +    - \"git checkout -- foo\" will check out path \"foo\"\n    +    - \"git checkout foo --\" will dwim and create branch \"foo\" [4]\n    +\n    +    For users who do not want dwim, use --no-guess. It's useless in this\n    +    particular case because \"git checkout --no-guess foo --\" will just\n    +    fail. But it could be used by scripts.\n     \n         [1] 70c9ac2f19 (DWIM \"git checkout frotz\" to \"git checkout -b frotz\n             origin/frotz\" - 2009-10-18)\n         [2] https://public-inbox.org/git/CACsJy8B2TVr1g+k+eSQ=pBEO3WN4_LtgLo9gpur8X7Z9GOFL_A@mail.gmail.com/\n         [3] https://news.ycombinator.com/item?id=18230655\n    +    [4] a047fafc78 (checkout: allow dwim for branch creation for \"git\n    +        checkout $branch --\" - 2013-10-18)\n    +\n    + diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt\n    + --- a/Documentation/git-checkout.txt\n    + +++ b/Documentation/git-checkout.txt\n    +@@\n    + \tJust like linkgit:git-submodule[1], this will detach the\n    + \tsubmodules HEAD.\n    + \n    ++--no-guess::\n    ++\tDo not attempt to create a branch if a remote tracking branch\n    ++\tof the same name exists.\n    ++\n    + <branch>::\n    + \tBranch to checkout; if it refers to a branch (i.e., a name that,\n    + \twhen prepended with \"refs/heads/\", is a valid ref), then that\n     \n      diff --git a/builtin/checkout.c b/builtin/checkout.c\n      --- a/builtin/checkout.c\n    @@ -37,9 +61,11 @@\n      \n     -\t\tif (!has_dash_dash &&\n     -\t\t    (check_filename(opts->prefix, arg) || !no_wildcard(arg)))\n    --\t\t\trecover_with_dwim = 0;\n     +\t\tint could_be_checkout_paths = !has_dash_dash &&\n    -+\t\t\t(check_filename(opts->prefix, arg) || !no_wildcard(arg));\n    ++\t\t\tcheck_filename(opts->prefix, arg);\n    ++\n    ++\t\tif (!has_dash_dash && !no_wildcard(arg))\n    + \t\t\trecover_with_dwim = 0;\n     +\n      \t\t/*\n      \t\t * Accept \"git checkout foo\" and \"git checkout foo --\"\n    @@ -50,10 +76,39 @@\n      \t\t\tif (remote) {\n     +\t\t\t\tif (could_be_checkout_paths)\n     +\t\t\t\t\tdie(_(\"'%s' could be both a local file and a tracking branch.\\n\"\n    -+\t\t\t\t\t      \"Please use -- to disambiguate\"), arg);\n    ++\t\t\t\t\t      \"Please use -- (and optionally --no-guess) to disambiguate\"),\n    ++\t\t\t\t\t    arg);\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    +@@\n    + \tstruct checkout_opts opts;\n    + \tstruct branch_info new_branch_info;\n    + \tchar *conflict_style = NULL;\n    +-\tint dwim_new_local_branch = 1;\n    ++\tint dwim_new_local_branch, no_dwim_new_local_branch = 0;\n    + \tint dwim_remotes_matched = 0;\n    + \tstruct option options[] = {\n    + \t\tOPT__QUIET(&opts.quiet, N_(\"suppress progress reporting\")),\n    +@@\n    + \t\tOPT_BOOL('p', \"patch\", &opts.patch_mode, N_(\"select hunks interactively\")),\n    + \t\tOPT_BOOL(0, \"ignore-skip-worktree-bits\", &opts.ignore_skipworktree,\n    + \t\t\t N_(\"do not limit pathspecs to sparse entries only\")),\n    +-\t\tOPT_HIDDEN_BOOL(0, \"guess\", &dwim_new_local_branch,\n    +-\t\t\t\tN_(\"second guess 'git checkout <no-such-branch>'\")),\n    ++\t\tOPT_BOOL(0, \"no-guess\", &no_dwim_new_local_branch,\n    ++\t\t\t N_(\"do not second guess 'git checkout <no-such-branch>'\")),\n    + \t\tOPT_BOOL(0, \"ignore-other-worktrees\", &opts.ignore_other_worktrees,\n    + \t\t\t N_(\"do not check if another worktree is holding the given ref\")),\n    + \t\t{ OPTION_CALLBACK, 0, \"recurse-submodules\", NULL,\n    +@@\n    + \targc = parse_options(argc, argv, prefix, options, checkout_usage,\n    + \t\t\t     PARSE_OPT_KEEP_DASHDASH);\n    + \n    ++\tdwim_new_local_branch = !no_dwim_new_local_branch;\n    + \tif (opts.show_progress < 0) {\n    + \t\tif (opts.quiet)\n    + \t\t\topts.show_progress = 0;\n     \n      diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\n      --- a/t/t2024-checkout-dwim.sh\n    @@ -94,3 +149,17 @@\n     +'\n     +\n      test_done\n    +\n    + diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n    + --- a/t/t9902-completion.sh\n    + +++ b/t/t9902-completion.sh\n    +@@\n    + \t--ignore-other-worktrees Z\n    + \t--recurse-submodules Z\n    + \t--progress Z\n    +-\t--no-quiet Z\n    ++\t--guess Z\n    ++\t--no-guess Z\n    + \t--no-... Z\n    + \tEOF\n    + '\n-- \n2.19.1.1318.g5295c6727d\n\n"},{"id":"363181","messageId":"20181113175226.25635-2-pclouds@gmail.com","threadId":"49792","inReplyTo":"20181113175226.25635-1-pclouds@gmail.com","subject":"[PATCH v2 1/1] checkout: disambiguate dwim tracking branches and local files","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-13T17:52:26Z","receivedAt":"2018-11-13T17:52:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When checkout dwim is added in [1], it is restricted to only dwim when\ncertain conditions are met and fall back to default checkout behavior\notherwise. It turns out falling back could be confusing. One of the\nconditions to turn\n\n    git checkout frotz\n\nto\n\n    git checkout -b frotz origin/frotz\n\nis that frotz must not exist as a file. But when the user comes to\nexpect \"git checkout frotz\" to create the branch \"frotz\" and there\nhappens to be a file named \"frotz\", git's silently reverting \"frotz\"\nfile content is not helping. This is reported in Git mailing list [2]\nand even used as an example of \"Git is bad\" elsewhere [3].\n\nWe normally try to do the right thing, but when there are multiple\n\"right things\" to do, it's best to leave it to the user to decide.\nCheck this case, ask the user to to disambiguate:\n\n- \"git checkout -- foo\" will check out path \"foo\"\n- \"git checkout foo --\" will dwim and create branch \"foo\" [4]\n\nFor users who do not want dwim, use --no-guess. It's useless in this\nparticular case because \"git checkout --no-guess foo --\" will just\nfail. But it could be used by scripts.\n\n[1] 70c9ac2f19 (DWIM \"git checkout frotz\" to \"git checkout -b frotz\n    origin/frotz\" - 2009-10-18)\n[2] https://public-inbox.org/git/CACsJy8B2TVr1g+k+eSQ=pBEO3WN4_LtgLo9gpur8X7Z9GOFL_A@mail.gmail.com/\n[3] https://news.ycombinator.com/item?id=18230655\n[4] a047fafc78 (checkout: allow dwim for branch creation for \"git\n    checkout $branch --\" - 2013-10-18)\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/git-checkout.txt |  4 ++++\n builtin/checkout.c             | 18 +++++++++++++-----\n t/t2024-checkout-dwim.sh       | 31 +++++++++++++++++++++++++++++++\n t/t9902-completion.sh          |  3 ++-\n 4 files changed, 50 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt\nindex 801de2f764..6acc3d98e7 100644\n--- a/Documentation/git-checkout.txt\n+++ b/Documentation/git-checkout.txt\n@@ -276,6 +276,10 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode.\n \tJust like linkgit:git-submodule[1], this will detach the\n \tsubmodules HEAD.\n \n+--no-guess::\n+\tDo not attempt to create a branch if a remote tracking branch\n+\tof the same name exists.\n+\n <branch>::\n \tBranch to checkout; if it refers to a branch (i.e., a name that,\n \twhen prepended with \"refs/heads/\", is a valid ref), then that\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex acdafc6e4c..4744e8c0a7 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1079,9 +1079,12 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t */\n \t\tint recover_with_dwim = dwim_new_local_branch_ok;\n \n-\t\tif (!has_dash_dash &&\n-\t\t    (check_filename(opts->prefix, arg) || !no_wildcard(arg)))\n+\t\tint could_be_checkout_paths = !has_dash_dash &&\n+\t\t\tcheck_filename(opts->prefix, arg);\n+\n+\t\tif (!has_dash_dash && !no_wildcard(arg))\n \t\t\trecover_with_dwim = 0;\n+\n \t\t/*\n \t\t * Accept \"git checkout foo\" and \"git checkout foo --\"\n \t\t * as candidates for dwim.\n@@ -1094,6 +1097,10 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t\tconst char *remote = unique_tracking_name(arg, rev,\n \t\t\t\t\t\t\t\t  dwim_remotes_matched);\n \t\t\tif (remote) {\n+\t\t\t\tif (could_be_checkout_paths)\n+\t\t\t\t\tdie(_(\"'%s' could be both a local file and a tracking branch.\\n\"\n+\t\t\t\t\t      \"Please use -- (and optionally --no-guess) to disambiguate\"),\n+\t\t\t\t\t    arg);\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@@ -1228,7 +1235,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tstruct checkout_opts opts;\n \tstruct branch_info new_branch_info;\n \tchar *conflict_style = NULL;\n-\tint dwim_new_local_branch = 1;\n+\tint dwim_new_local_branch, no_dwim_new_local_branch = 0;\n \tint dwim_remotes_matched = 0;\n \tstruct option options[] = {\n \t\tOPT__QUIET(&opts.quiet, N_(\"suppress progress reporting\")),\n@@ -1258,8 +1265,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL('p', \"patch\", &opts.patch_mode, N_(\"select hunks interactively\")),\n \t\tOPT_BOOL(0, \"ignore-skip-worktree-bits\", &opts.ignore_skipworktree,\n \t\t\t N_(\"do not limit pathspecs to sparse entries only\")),\n-\t\tOPT_HIDDEN_BOOL(0, \"guess\", &dwim_new_local_branch,\n-\t\t\t\tN_(\"second guess 'git checkout <no-such-branch>'\")),\n+\t\tOPT_BOOL(0, \"no-guess\", &no_dwim_new_local_branch,\n+\t\t\t N_(\"do not second guess 'git checkout <no-such-branch>'\")),\n \t\tOPT_BOOL(0, \"ignore-other-worktrees\", &opts.ignore_other_worktrees,\n \t\t\t N_(\"do not check if another worktree is holding the given ref\")),\n \t\t{ OPTION_CALLBACK, 0, \"recurse-submodules\", NULL,\n@@ -1282,6 +1289,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, checkout_usage,\n \t\t\t     PARSE_OPT_KEEP_DASHDASH);\n \n+\tdwim_new_local_branch = !no_dwim_new_local_branch;\n \tif (opts.show_progress < 0) {\n \t\tif (opts.quiet)\n \t\t\topts.show_progress = 0;\ndiff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\nindex 69b6774d10..fa0718c730 100755\n--- a/t/t2024-checkout-dwim.sh\n+++ b/t/t2024-checkout-dwim.sh\n@@ -278,4 +278,35 @@ test_expect_success 'loosely defined local base branch is reported correctly' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'reject when arg could be part of dwim branch' '\n+\tgit remote add foo file://non-existent-place &&\n+\tgit update-ref refs/remotes/foo/dwim-arg HEAD &&\n+\techo foo >dwim-arg &&\n+\tgit add dwim-arg &&\n+\techo bar >dwim-arg &&\n+\ttest_must_fail git checkout dwim-arg &&\n+\ttest_must_fail git rev-parse refs/heads/dwim-arg -- &&\n+\tgrep bar dwim-arg\n+'\n+\n+test_expect_success 'disambiguate dwim branch and checkout path (1)' '\n+\tgit update-ref refs/remotes/foo/dwim-arg1 HEAD &&\n+\techo foo >dwim-arg1 &&\n+\tgit add dwim-arg1 &&\n+\techo bar >dwim-arg1 &&\n+\tgit checkout -- dwim-arg1 &&\n+\ttest_must_fail git rev-parse refs/heads/dwim-arg1 -- &&\n+\tgrep foo dwim-arg1\n+'\n+\n+test_expect_success 'disambiguate dwim branch and checkout path (2)' '\n+\tgit update-ref refs/remotes/foo/dwim-arg2 HEAD &&\n+\techo foo >dwim-arg2 &&\n+\tgit add dwim-arg2 &&\n+\techo bar >dwim-arg2 &&\n+\tgit checkout dwim-arg2 -- &&\n+\tgit rev-parse refs/heads/dwim-arg2 -- &&\n+\tgrep bar dwim-arg2\n+'\n+\n test_done\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 175f83d704..aa92f85230 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1434,7 +1434,8 @@ test_expect_success 'double dash \"git checkout\"' '\n \t--ignore-other-worktrees Z\n \t--recurse-submodules Z\n \t--progress Z\n-\t--no-quiet Z\n+\t--guess Z\n+\t--no-guess Z\n \t--no-... Z\n \tEOF\n '\n-- \n2.19.1.1318.g5295c6727d\n\n"}]}