{"thread":{"id":"52356","subject":"[PATCH 1/2] parse_branchname_arg(): extract part as new function","startedAt":"2019-11-27T09:49:22Z","lastAt":"2019-12-05T15:34:25Z","messageCount":13,"participants":["Alexandr Miloslavskiy via GitGitGadget","Derrick Stolee","Ævar Arnfjörð Bjarmason","Alexandr Miloslavskiy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"387136","messageId":"c5b92ce3ed629ccd5efddb9d5c7a754ed2e8ddbe.1574848137.git.gitgitgadget@gmail.com","threadId":"52356","inReplyTo":"pull.477.git.1574848137.gitgitgadget@gmail.com","subject":"[PATCH 1/2] parse_branchname_arg(): extract part as new function","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-27T09:48:56Z","receivedAt":"2019-11-27T09:49:22Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>\n\nThis is done for the next commit to avoid crazy 7x tab code padding.\n\nSigned-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>\n---\n builtin/checkout.c | 25 +++++++++++++++++++------\n 1 file changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3634a3dac1..e1b9df1543 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1113,6 +1113,22 @@ static void setup_new_branch_info_and_source_tree(\n \t}\n }\n \n+static const char *parse_remote_branch(const char *arg,\n+\t\t\t\t       struct object_id *rev,\n+\t\t\t\t       int could_be_checkout_paths,\n+\t\t\t\t       int *dwim_remotes_matched)\n+{\n+\tconst char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);\n+\t\n+\tif (remote && could_be_checkout_paths) {\n+\t\tdie(_(\"'%s' could be both a local file and a tracking branch.\\n\"\n+\t\t\t\"Please use -- (and optionally --no-guess) to disambiguate\"),\n+\t\t    arg);\n+\t}\n+\n+\treturn remote;\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_branch_info,\n@@ -1223,13 +1239,10 @@ static int parse_branchname_arg(int argc, const char **argv,\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\t\t\t\t\t\t  dwim_remotes_matched);\n+\t\t\tconst char *remote = parse_remote_branch(arg, rev,\n+\t\t\t\t\t\t\t\t could_be_checkout_paths,\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-- \ngitgitgadget\n\n"},{"id":"387137","messageId":"7dde1a3b4e4e76cd1a820b5277f694fdfad3a922.1574848137.git.gitgitgadget@gmail.com","threadId":"52356","inReplyTo":"pull.477.git.1574848137.gitgitgadget@gmail.com","subject":"[PATCH 2/2] checkout: die() on ambiguous tracking branches","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-27T09:48:57Z","receivedAt":"2019-11-27T09:49:22Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>\n\nBefore this patch, when there were multiple DWIM candidates for remote\nbranch, git decided to try the argument as pathspec instead. I believe\nthat such behavior is a surprise: adding another remote suddenly causes\ngit to discard file contents, because it was unsure which branch to\npick. There was an incomplete attempt to prevent that in [3].\n\nI understand that this was never intended:\n\n  [1] introduces the unexpected behavior. Before, there was fallback\n  from not-a-ref to pathspec. This is reasonable DWIM. After, there is\n  another fallback from ambiguous-remote to pathspec. I understand that\n  it was kind of copy&paste oversight.\n\n  [2] noticed the unexpected behavior but chose to semi-document it\n  instead of forbidding, because the goal of the patch series was\n  focused on something else.\n\n  [3] adds `die()` when there is ambiguity between branch and file. The\n  case of multiple tracking branches is seemingly overlooked.\n\nChange to complain about ambiguity instead of doing unexpected things.\n\n[1] Commit 70c9ac2f (\"DWIM \"git checkout frotz\" to \"git checkout -b frotz origin/frotz\"\" 2009-10-18)\n    https://public-inbox.org/git/7vaazpxha4.fsf_-_@alter.siamese.dyndns.org/\n[2] Commit ad8d5104 (\"checkout: add advice for ambiguous \"checkout <branch>\"\", 2018-06-05)\n    https://public-inbox.org/git/20180502105452.17583-1-avarab@gmail.com/\n[3] Commit be4908f1 (\"checkout: disambiguate dwim tracking branches and local files\", 2018-11-13)\n    https://public-inbox.org/git/20181110120707.25846-1-pclouds@gmail.com/\n\nSigned-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>\n---\n builtin/checkout.c       | 56 ++++++++++++++++++----------------------\n t/t2024-checkout-dwim.sh | 23 +++++++++++++++--\n 2 files changed, 46 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex e1b9df1543..6fb427990f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1115,10 +1115,10 @@ static void setup_new_branch_info_and_source_tree(\n \n static const char *parse_remote_branch(const char *arg,\n \t\t\t\t       struct object_id *rev,\n-\t\t\t\t       int could_be_checkout_paths,\n-\t\t\t\t       int *dwim_remotes_matched)\n+\t\t\t\t       int could_be_checkout_paths)\n {\n-\tconst char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);\n+\tint num_matches = 0;\n+\tconst char *remote = unique_tracking_name(arg, rev, &num_matches);\n \t\n \tif (remote && could_be_checkout_paths) {\n \t\tdie(_(\"'%s' could be both a local file and a tracking branch.\\n\"\n@@ -1126,6 +1126,22 @@ static const char *parse_remote_branch(const char *arg,\n \t\t    arg);\n \t}\n \n+\tif (!remote && (num_matches > 1)) {\n+\t    if (advice_checkout_ambiguous_remote_branch_name) {\n+\t\t    advise(_(\"If you meant to check out a remote tracking branch on, e.g. 'origin',\\n\"\n+\t\t\t     \"you can do so by fully qualifying the name with the --track option:\\n\"\n+\t\t\t     \"\\n\"\n+\t\t\t     \"    git checkout --track origin/<name>\\n\"\n+\t\t\t     \"\\n\"\n+\t\t\t     \"If you'd like to always have checkouts of an ambiguous <name> prefer\\n\"\n+\t\t\t     \"one remote, e.g. the 'origin' remote, consider setting\\n\"\n+\t\t\t     \"checkout.defaultRemote=origin in your config.\"));\n+\t    }\n+\n+\t    die(_(\"'%s' matched multiple (%d) remote tracking branches\"),\n+\t\targ, num_matches);\n+\t}\n+\n \treturn remote;\n }\n \n@@ -1133,8 +1149,7 @@ 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_branch_info,\n \t\t\t\tstruct checkout_opts *opts,\n-\t\t\t\tstruct object_id *rev,\n-\t\t\t\tint *dwim_remotes_matched)\n+\t\t\t\tstruct object_id *rev)\n {\n \tconst char **new_branch = &opts->new_branch;\n \tint argcount = 0;\n@@ -1240,8 +1255,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \n \t\tif (recover_with_dwim) {\n \t\t\tconst char *remote = parse_remote_branch(arg, rev,\n-\t\t\t\t\t\t\t\t could_be_checkout_paths,\n-\t\t\t\t\t\t\t\t dwim_remotes_matched);\n+\t\t\t\t\t\t\t\t could_be_checkout_paths);\n \t\t\tif (remote) {\n \t\t\t\t*new_branch = arg;\n \t\t\t\targ = remote;\n@@ -1505,7 +1519,6 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t const char * const usagestr[])\n {\n \tstruct branch_info new_branch_info;\n-\tint dwim_remotes_matched = 0;\n \tint parseopt_flags = 0;\n \n \tmemset(&new_branch_info, 0, sizeof(new_branch_info));\n@@ -1613,8 +1626,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\topts->track == BRANCH_TRACK_UNSPECIFIED &&\n \t\t\t!opts->new_branch;\n \t\tint n = parse_branchname_arg(argc, argv, dwim_ok,\n-\t\t\t\t\t     &new_branch_info, opts, &rev,\n-\t\t\t\t\t     &dwim_remotes_matched);\n+\t\t\t\t\t     &new_branch_info, opts, &rev);\n \t\targv += n;\n \t\targc -= n;\n \t} else if (!opts->accept_ref && opts->from_treeish) {\n@@ -1672,28 +1684,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t}\n \n \tUNLEAK(opts);\n-\tif (opts->patch_mode || opts->pathspec.nr) {\n-\t\tint ret = checkout_paths(opts, new_branch_info.name);\n-\t\tif (ret && dwim_remotes_matched > 1 &&\n-\t\t    advice_checkout_ambiguous_remote_branch_name)\n-\t\t\tadvise(_(\"'%s' matched more than one remote tracking branch.\\n\"\n-\t\t\t\t \"We found %d remotes with a reference that matched. So we fell back\\n\"\n-\t\t\t\t \"on trying to resolve the argument as a path, but failed there too!\\n\"\n-\t\t\t\t \"\\n\"\n-\t\t\t\t \"If you meant to check out a remote tracking branch on, e.g. 'origin',\\n\"\n-\t\t\t\t \"you can do so by fully qualifying the name with the --track option:\\n\"\n-\t\t\t\t \"\\n\"\n-\t\t\t\t \"    git checkout --track origin/<name>\\n\"\n-\t\t\t\t \"\\n\"\n-\t\t\t\t \"If you'd like to always have checkouts of an ambiguous <name> prefer\\n\"\n-\t\t\t\t \"one remote, e.g. the 'origin' remote, consider setting\\n\"\n-\t\t\t\t \"checkout.defaultRemote=origin in your config.\"),\n-\t\t\t       argv[0],\n-\t\t\t       dwim_remotes_matched);\n-\t\treturn ret;\n-\t} else {\n+\tif (opts->patch_mode || opts->pathspec.nr)\n+\t\treturn checkout_paths(opts, new_branch_info.name);\n+\telse\n \t\treturn checkout_branch(opts, &new_branch_info);\n-\t}\n }\n \n int cmd_checkout(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\nindex fa0718c730..707c88ceba 100755\n--- a/t/t2024-checkout-dwim.sh\n+++ b/t/t2024-checkout-dwim.sh\n@@ -37,7 +37,9 @@ test_expect_success 'setup' '\n \t\tgit checkout -b foo &&\n \t\ttest_commit a_foo &&\n \t\tgit checkout -b bar &&\n-\t\ttest_commit a_bar\n+\t\ttest_commit a_bar &&\n+\t\tgit checkout -b ambiguous_branch_and_file &&\n+\t\ttest_commit a_ambiguous_branch_and_file\n \t) &&\n \tgit init repo_b &&\n \t(\n@@ -46,7 +48,9 @@ test_expect_success 'setup' '\n \t\tgit checkout -b foo &&\n \t\ttest_commit b_foo &&\n \t\tgit checkout -b baz &&\n-\t\ttest_commit b_baz\n+\t\ttest_commit b_baz &&\n+\t\tgit checkout -b ambiguous_branch_and_file &&\n+\t\ttest_commit b_ambiguous_branch_and_file\n \t) &&\n \tgit remote add repo_a repo_a &&\n \tgit remote add repo_b repo_b &&\n@@ -75,6 +79,21 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '\n \ttest_branch master\n '\n \n+test_expect_success 'when arg matches multiple remotes, do not fallback to interpreting as pathspec' '\n+\tgit checkout -b t_ambiguous_branch_and_file &&\n+\t>ambiguous_branch_and_file &&\n+\tgit add ambiguous_branch_and_file &&\n+\tgit commit -m \"ambiguous_branch_and_file\" &&\n+\n+\ttest_when_finished \"git checkout -- ambiguous_branch_and_file\" &&\n+\techo \"file contents\" >ambiguous_branch_and_file &&\n+\tcp ambiguous_branch_and_file expect &&\n+\n+\ttest_must_fail git checkout ambiguous_branch_and_file &&\n+\n+\ttest_cmp expect ambiguous_branch_and_file\n+'\n+\n test_expect_success 'checkout of branch from multiple remotes fails with advice' '\n \tgit checkout -B master &&\n \ttest_might_fail git branch -D foo &&\n-- \ngitgitgadget\n"},{"id":"387138","messageId":"pull.477.git.1574848137.gitgitgadget@gmail.com","threadId":"52356","inReplyTo":null,"subject":"[PATCH 0/2] checkout: die() on ambiguous tracking branches","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-27T09:48:55Z","receivedAt":"2019-11-27T09:49:23Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"Please refer to commit messages for details.\n\nRationale: this issue was standing in my way while working on my \n--pathspec-from-file patches. Since this looks like an oversight, I decided\nto fix it.\n\nAlexandr Miloslavskiy (2):\n  parse_branchname_arg(): extract part as new function\n  checkout: die() on ambiguous tracking branches\n\n builtin/checkout.c       | 71 ++++++++++++++++++++++------------------\n t/t2024-checkout-dwim.sh | 23 +++++++++++--\n 2 files changed, 60 insertions(+), 34 deletions(-)\n\n\nbase-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-477%2FSyntevoAlex%2F%230224(git)_deny_ambiguous_checkout-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-477/SyntevoAlex/#0224(git)_deny_ambiguous_checkout-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/477\n-- \ngitgitgadget\n"},{"id":"387160","messageId":"b6963515-1f28-74ab-42bc-a14a07856118@gmail.com","threadId":"52356","inReplyTo":"7dde1a3b4e4e76cd1a820b5277f694fdfad3a922.1574848137.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] checkout: die() on ambiguous tracking branches","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-11-27T14:46:33Z","receivedAt":"2019-11-27T14:46:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/27/2019 4:48 AM, Alexandr Miloslavskiy via GitGitGadget wrote:\n> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>\n> \n> Before this patch, when there were multiple DWIM candidates for remote\n> branch, git decided to try the argument as pathspec instead. I believe\n> that such behavior is a surprise: adding another remote suddenly causes\n> git to discard file contents, because it was unsure which branch to\n> pick. There was an incomplete attempt to prevent that in [3].\n> \n> I understand that this was never intended:\n> \n>   [1] introduces the unexpected behavior. Before, there was fallback\n>   from not-a-ref to pathspec. This is reasonable DWIM. After, there is\n>   another fallback from ambiguous-remote to pathspec. I understand that\n>   it was kind of copy&paste oversight.\n> \n>   [2] noticed the unexpected behavior but chose to semi-document it\n>   instead of forbidding, because the goal of the patch series was\n>   focused on something else.\n> \n>   [3] adds `die()` when there is ambiguity between branch and file. The\n>   case of multiple tracking branches is seemingly overlooked.\n> \n> Change to complain about ambiguity instead of doing unexpected things.\n> \n> [1] Commit 70c9ac2f (\"DWIM \"git checkout frotz\" to \"git checkout -b frotz origin/frotz\"\" 2009-10-18)\n>     https://public-inbox.org/git/7vaazpxha4.fsf_-_@alter.siamese.dyndns.org/\n> [2] Commit ad8d5104 (\"checkout: add advice for ambiguous \"checkout <branch>\"\", 2018-06-05)\n>     https://public-inbox.org/git/20180502105452.17583-1-avarab@gmail.com/\n> [3] Commit be4908f1 (\"checkout: disambiguate dwim tracking branches and local files\", 2018-11-13)\n>     https://public-inbox.org/git/20181110120707.25846-1-pclouds@gmail.com/\n> \n> Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>\n> ---\n>  builtin/checkout.c       | 56 ++++++++++++++++++----------------------\n>  t/t2024-checkout-dwim.sh | 23 +++++++++++++++--\n>  2 files changed, 46 insertions(+), 33 deletions(-)\n> \n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index e1b9df1543..6fb427990f 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1115,10 +1115,10 @@ static void setup_new_branch_info_and_source_tree(\n>  \n>  static const char *parse_remote_branch(const char *arg,\n>  \t\t\t\t       struct object_id *rev,\n> -\t\t\t\t       int could_be_checkout_paths,\n> -\t\t\t\t       int *dwim_remotes_matched)\n> +\t\t\t\t       int could_be_checkout_paths)\n>  {\n> -\tconst char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);\n> +\tint num_matches = 0;\n> +\tconst char *remote = unique_tracking_name(arg, rev, &num_matches);\n>  \t\n>  \tif (remote && could_be_checkout_paths) {\n>  \t\tdie(_(\"'%s' could be both a local file and a tracking branch.\\n\"\n> @@ -1126,6 +1126,22 @@ static const char *parse_remote_branch(const char *arg,\n>  \t\t    arg);\n>  \t}\n>  \n> +\tif (!remote && (num_matches > 1)) {\n\nI believe the parentheses around \"num_matched > 1\" are not needed here.\n\n> +\t    if (advice_checkout_ambiguous_remote_branch_name) {\n> +\t\t    advise(_(\"If you meant to check out a remote tracking branch on, e.g. 'origin',\\n\"\n> +\t\t\t     \"you can do so by fully qualifying the name with the --track option:\\n\"\n> +\t\t\t     \"\\n\"\n> +\t\t\t     \"    git checkout --track origin/<name>\\n\"\n> +\t\t\t     \"\\n\"\n> +\t\t\t     \"If you'd like to always have checkouts of an ambiguous <name> prefer\\n\"\n> +\t\t\t     \"one remote, e.g. the 'origin' remote, consider setting\\n\"\n> +\t\t\t     \"checkout.defaultRemote=origin in your config.\"));\n> +\t    }\n> +\n> +\t    die(_(\"'%s' matched multiple (%d) remote tracking branches\"),\n> +\t\targ, num_matches);\n> +\t}\n> +\n>  \treturn remote;\n>  }\n>  \n> @@ -1133,8 +1149,7 @@ 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_branch_info,\n>  \t\t\t\tstruct checkout_opts *opts,\n> -\t\t\t\tstruct object_id *rev,\n> -\t\t\t\tint *dwim_remotes_matched)\n> +\t\t\t\tstruct object_id *rev)\n>  {\n>  \tconst char **new_branch = &opts->new_branch;\n>  \tint argcount = 0;\n> @@ -1240,8 +1255,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n>  \n>  \t\tif (recover_with_dwim) {\n>  \t\t\tconst char *remote = parse_remote_branch(arg, rev,\n> -\t\t\t\t\t\t\t\t could_be_checkout_paths,\n> -\t\t\t\t\t\t\t\t dwim_remotes_matched);\n> +\t\t\t\t\t\t\t\t could_be_checkout_paths);\n>  \t\t\tif (remote) {\n>  \t\t\t\t*new_branch = arg;\n>  \t\t\t\targ = remote;\n> @@ -1505,7 +1519,6 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n>  \t\t\t const char * const usagestr[])\n>  {\n>  \tstruct branch_info new_branch_info;\n> -\tint dwim_remotes_matched = 0;\n>  \tint parseopt_flags = 0;\n>  \n>  \tmemset(&new_branch_info, 0, sizeof(new_branch_info));\n> @@ -1613,8 +1626,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n>  \t\t\topts->track == BRANCH_TRACK_UNSPECIFIED &&\n>  \t\t\t!opts->new_branch;\n>  \t\tint n = parse_branchname_arg(argc, argv, dwim_ok,\n> -\t\t\t\t\t     &new_branch_info, opts, &rev,\n> -\t\t\t\t\t     &dwim_remotes_matched);\n> +\t\t\t\t\t     &new_branch_info, opts, &rev);\n>  \t\targv += n;\n>  \t\targc -= n;\n>  \t} else if (!opts->accept_ref && opts->from_treeish) {\n> @@ -1672,28 +1684,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n>  \t}\n>  \n>  \tUNLEAK(opts);\n> -\tif (opts->patch_mode || opts->pathspec.nr) {\n> -\t\tint ret = checkout_paths(opts, new_branch_info.name);\n> -\t\tif (ret && dwim_remotes_matched > 1 &&\n> -\t\t    advice_checkout_ambiguous_remote_branch_name)\n> -\t\t\tadvise(_(\"'%s' matched more than one remote tracking branch.\\n\"\n> -\t\t\t\t \"We found %d remotes with a reference that matched. So we fell back\\n\"\n> -\t\t\t\t \"on trying to resolve the argument as a path, but failed there too!\\n\"\n> -\t\t\t\t \"\\n\"\n> -\t\t\t\t \"If you meant to check out a remote tracking branch on, e.g. 'origin',\\n\"\n> -\t\t\t\t \"you can do so by fully qualifying the name with the --track option:\\n\"\n> -\t\t\t\t \"\\n\"\n> -\t\t\t\t \"    git checkout --track origin/<name>\\n\"\n> -\t\t\t\t \"\\n\"\n> -\t\t\t\t \"If you'd like to always have checkouts of an ambiguous <name> prefer\\n\"\n> -\t\t\t\t \"one remote, e.g. the 'origin' remote, consider setting\\n\"\n> -\t\t\t\t \"checkout.defaultRemote=origin in your config.\"),\n> -\t\t\t       argv[0],\n> -\t\t\t       dwim_remotes_matched);\n> -\t\treturn ret;\n> -\t} else {\n> +\tif (opts->patch_mode || opts->pathspec.nr)\n> +\t\treturn checkout_paths(opts, new_branch_info.name);\n> +\telse\n>  \t\treturn checkout_branch(opts, &new_branch_info);\n> -\t}\n>  }\n>  \n>  int cmd_checkout(int argc, const char **argv, const char *prefix)\n> diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\n> index fa0718c730..707c88ceba 100755\n> --- a/t/t2024-checkout-dwim.sh\n> +++ b/t/t2024-checkout-dwim.sh\n> @@ -37,7 +37,9 @@ test_expect_success 'setup' '\n>  \t\tgit checkout -b foo &&\n>  \t\ttest_commit a_foo &&\n>  \t\tgit checkout -b bar &&\n> -\t\ttest_commit a_bar\n> +\t\ttest_commit a_bar &&\n> +\t\tgit checkout -b ambiguous_branch_and_file &&\n> +\t\ttest_commit a_ambiguous_branch_and_file\n>  \t) &&\n>  \tgit init repo_b &&\n>  \t(\n> @@ -46,7 +48,9 @@ test_expect_success 'setup' '\n>  \t\tgit checkout -b foo &&\n>  \t\ttest_commit b_foo &&\n>  \t\tgit checkout -b baz &&\n> -\t\ttest_commit b_baz\n> +\t\ttest_commit b_baz &&\n> +\t\tgit checkout -b ambiguous_branch_and_file &&\n> +\t\ttest_commit b_ambiguous_branch_and_file\n>  \t) &&\n>  \tgit remote add repo_a repo_a &&\n>  \tgit remote add repo_b repo_b &&\n> @@ -75,6 +79,21 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '\n>  \ttest_branch master\n>  '\n>  \n> +test_expect_success 'when arg matches multiple remotes, do not fallback to interpreting as pathspec' '\n> +\tgit checkout -b t_ambiguous_branch_and_file &&\n> +\t>ambiguous_branch_and_file &&\n> +\tgit add ambiguous_branch_and_file &&\n> +\tgit commit -m \"ambiguous_branch_and_file\" &&\n> +\n> +\ttest_when_finished \"git checkout -- ambiguous_branch_and_file\" &&\n> +\techo \"file contents\" >ambiguous_branch_and_file &&\n> +\tcp ambiguous_branch_and_file expect &&\n> +\n> +\ttest_must_fail git checkout ambiguous_branch_and_file &&\n> +\n> +\ttest_cmp expect ambiguous_branch_and_file\n> +'\n\nI like that you added a test for this non-obvious situation. One thing that\nwould help ensure that your test is checking the right thing is to redirect\nstderr to a file and grep for the die() message you included. Something like:\n\n\ttest_must_fail git checkout ambiguous_branch_and_file 2>err &&\n\ttest_i18ngrep \"matched multiple (2) remote tracking branches\" err &&\n\nThanks,\n-Stolee\n"},{"id":"387169","messageId":"87v9r5nx3w.fsf@evledraar.gmail.com","threadId":"52356","inReplyTo":"7dde1a3b4e4e76cd1a820b5277f694fdfad3a922.1574848137.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] checkout: die() on ambiguous tracking branches","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-11-27T15:43:31Z","receivedAt":"2019-11-27T15:43:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 27 2019, Alexandr Miloslavskiy via GitGitGadget wrote:\n\n> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>\n>\n> Before this patch, when there were multiple DWIM candidates for remote\n> branch, git decided to try the argument as pathspec instead. I believe\n> that such behavior is a surprise: adding another remote suddenly causes\n> git to discard file contents, because it was unsure which branch to\n> pick. There was an incomplete attempt to prevent that in [3].\n>\n> I understand that this was never intended:\n>\n>   [1] introduces the unexpected behavior. Before, there was fallback\n>   from not-a-ref to pathspec. This is reasonable DWIM. After, there is\n>   another fallback from ambiguous-remote to pathspec. I understand that\n>   it was kind of copy&paste oversight.\n>\n>   [2] noticed the unexpected behavior but chose to semi-document it\n>   instead of forbidding, because the goal of the patch series was\n>   focused on something else.\n>\n>   [3] adds `die()` when there is ambiguity between branch and file. The\n>   case of multiple tracking branches is seemingly overlooked.\n>\n> Change to complain about ambiguity instead of doing unexpected things.\n>\n> [1] Commit 70c9ac2f (\"DWIM \"git checkout frotz\" to \"git checkout -b frotz origin/frotz\"\" 2009-10-18)\n>     https://public-inbox.org/git/7vaazpxha4.fsf_-_@alter.siamese.dyndns.org/\n> [2] Commit ad8d5104 (\"checkout: add advice for ambiguous \"checkout <branch>\"\", 2018-06-05)\n>     https://public-inbox.org/git/20180502105452.17583-1-avarab@gmail.com/\n> [3] Commit be4908f1 (\"checkout: disambiguate dwim tracking branches and local files\", 2018-11-13)\n>     https://public-inbox.org/git/20181110120707.25846-1-pclouds@gmail.com/\n\nI'll reserve judgement on whether we really should do this for now, my\ncurrent opinion on the matter is undefined as I haven't re-paged this\nbehavior of checkout into my brain.\n\nBut a giant red flag here for me is that you say \"I understand that this\nwas never intended\".\n\nJust from a cursory look at this that's not true, for better or worse it\n*is* intended behavior. Most of the code you're moving around here is\nwhat I added in ad8d5104b4 (\"checkout: add advice for ambiguous\n\"checkout <branch>\"\", 2018-06-05), and the very start of that commit\nmessage refers to the checkout documentation we have that explicitly\ndocuments this edge case.\n\nDigging a bit further reveals that we've had this behavior (again,\nintended, not emergent) since 70c9ac2f19 (\"DWIM \"git checkout frotz\" to\n\"git checkout -b frotz origin/frotz\"\", 2009-10-18), and had it\ndocumented since 00bb4378c7 (\"Documentation/git-checkout.txt: document\n70c9ac2 behavior\", 2012-12-17).\n\nSo at the very least I'd say you need a v2 where you amend the relevant\ndocs & commit message to make a case to the effect of \"we've had this\nsince 2009, but it was never really all that important etc.\".\n\nSuch a change should also be changing the docs etc. added in 8d7b558bae\n(\"checkout & worktree: introduce checkout.defaultRemote\",\n2018-06-05). With this series our docs don't make a lot of sense anymore\n& don't describe the behavior with the patches applied.\n\n> Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>\n> ---\n>  builtin/checkout.c       | 56 ++++++++++++++++++----------------------\n>  t/t2024-checkout-dwim.sh | 23 +++++++++++++++--\n>  2 files changed, 46 insertions(+), 33 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index e1b9df1543..6fb427990f 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1115,10 +1115,10 @@ static void setup_new_branch_info_and_source_tree(\n>\n>  static const char *parse_remote_branch(const char *arg,\n>  \t\t\t\t       struct object_id *rev,\n> -\t\t\t\t       int could_be_checkout_paths,\n> -\t\t\t\t       int *dwim_remotes_matched)\n> +\t\t\t\t       int could_be_checkout_paths)\n>  {\n> -\tconst char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);\n> +\tint num_matches = 0;\n> +\tconst char *remote = unique_tracking_name(arg, rev, &num_matches);\n>\n>  \tif (remote && could_be_checkout_paths) {\n>  \t\tdie(_(\"'%s' could be both a local file and a tracking branch.\\n\"\n> @@ -1126,6 +1126,22 @@ static const char *parse_remote_branch(const char *arg,\n>  \t\t    arg);\n>  \t}\n>\n> +\tif (!remote && (num_matches > 1)) {\n> +\t    if (advice_checkout_ambiguous_remote_branch_name) {\n> +\t\t    advise(_(\"If you meant to check out a remote tracking branch on, e.g. 'origin',\\n\"\n> +\t\t\t     \"you can do so by fully qualifying the name with the --track option:\\n\"\n> +\t\t\t     \"\\n\"\n> +\t\t\t     \"    git checkout --track origin/<name>\\n\"\n> +\t\t\t     \"\\n\"\n> +\t\t\t     \"If you'd like to always have checkouts of an ambiguous <name> prefer\\n\"\n> +\t\t\t     \"one remote, e.g. the 'origin' remote, consider setting\\n\"\n> +\t\t\t     \"checkout.defaultRemote=origin in your config.\"));\n> +\t    }\n> +\n> +\t    die(_(\"'%s' matched multiple (%d) remote tracking branches\"),\n> +\t\targ, num_matches);\n> +\t}\n> +\n>  \treturn remote;\n>  }\n>\n> @@ -1133,8 +1149,7 @@ 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_branch_info,\n>  \t\t\t\tstruct checkout_opts *opts,\n> -\t\t\t\tstruct object_id *rev,\n> -\t\t\t\tint *dwim_remotes_matched)\n> +\t\t\t\tstruct object_id *rev)\n>  {\n>  \tconst char **new_branch = &opts->new_branch;\n>  \tint argcount = 0;\n> @@ -1240,8 +1255,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n>\n>  \t\tif (recover_with_dwim) {\n>  \t\t\tconst char *remote = parse_remote_branch(arg, rev,\n> -\t\t\t\t\t\t\t\t could_be_checkout_paths,\n> -\t\t\t\t\t\t\t\t dwim_remotes_matched);\n> +\t\t\t\t\t\t\t\t could_be_checkout_paths);\n>  \t\t\tif (remote) {\n>  \t\t\t\t*new_branch = arg;\n>  \t\t\t\targ = remote;\n> @@ -1505,7 +1519,6 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n>  \t\t\t const char * const usagestr[])\n>  {\n>  \tstruct branch_info new_branch_info;\n> -\tint dwim_remotes_matched = 0;\n>  \tint parseopt_flags = 0;\n>\n>  \tmemset(&new_branch_info, 0, sizeof(new_branch_info));\n> @@ -1613,8 +1626,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n>  \t\t\topts->track == BRANCH_TRACK_UNSPECIFIED &&\n>  \t\t\t!opts->new_branch;\n>  \t\tint n = parse_branchname_arg(argc, argv, dwim_ok,\n> -\t\t\t\t\t     &new_branch_info, opts, &rev,\n> -\t\t\t\t\t     &dwim_remotes_matched);\n> +\t\t\t\t\t     &new_branch_info, opts, &rev);\n>  \t\targv += n;\n>  \t\targc -= n;\n>  \t} else if (!opts->accept_ref && opts->from_treeish) {\n> @@ -1672,28 +1684,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n>  \t}\n>\n>  \tUNLEAK(opts);\n> -\tif (opts->patch_mode || opts->pathspec.nr) {\n> -\t\tint ret = checkout_paths(opts, new_branch_info.name);\n> -\t\tif (ret && dwim_remotes_matched > 1 &&\n> -\t\t    advice_checkout_ambiguous_remote_branch_name)\n> -\t\t\tadvise(_(\"'%s' matched more than one remote tracking branch.\\n\"\n> -\t\t\t\t \"We found %d remotes with a reference that matched. So we fell back\\n\"\n> -\t\t\t\t \"on trying to resolve the argument as a path, but failed there too!\\n\"\n> -\t\t\t\t \"\\n\"\n> -\t\t\t\t \"If you meant to check out a remote tracking branch on, e.g. 'origin',\\n\"\n> -\t\t\t\t \"you can do so by fully qualifying the name with the --track option:\\n\"\n> -\t\t\t\t \"\\n\"\n> -\t\t\t\t \"    git checkout --track origin/<name>\\n\"\n> -\t\t\t\t \"\\n\"\n> -\t\t\t\t \"If you'd like to always have checkouts of an ambiguous <name> prefer\\n\"\n> -\t\t\t\t \"one remote, e.g. the 'origin' remote, consider setting\\n\"\n> -\t\t\t\t \"checkout.defaultRemote=origin in your config.\"),\n> -\t\t\t       argv[0],\n> -\t\t\t       dwim_remotes_matched);\n> -\t\treturn ret;\n> -\t} else {\n> +\tif (opts->patch_mode || opts->pathspec.nr)\n> +\t\treturn checkout_paths(opts, new_branch_info.name);\n> +\telse\n>  \t\treturn checkout_branch(opts, &new_branch_info);\n> -\t}\n>  }\n>\n>  int cmd_checkout(int argc, const char **argv, const char *prefix)\n> diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\n> index fa0718c730..707c88ceba 100755\n> --- a/t/t2024-checkout-dwim.sh\n> +++ b/t/t2024-checkout-dwim.sh\n> @@ -37,7 +37,9 @@ test_expect_success 'setup' '\n>  \t\tgit checkout -b foo &&\n>  \t\ttest_commit a_foo &&\n>  \t\tgit checkout -b bar &&\n> -\t\ttest_commit a_bar\n> +\t\ttest_commit a_bar &&\n> +\t\tgit checkout -b ambiguous_branch_and_file &&\n> +\t\ttest_commit a_ambiguous_branch_and_file\n>  \t) &&\n>  \tgit init repo_b &&\n>  \t(\n> @@ -46,7 +48,9 @@ test_expect_success 'setup' '\n>  \t\tgit checkout -b foo &&\n>  \t\ttest_commit b_foo &&\n>  \t\tgit checkout -b baz &&\n> -\t\ttest_commit b_baz\n> +\t\ttest_commit b_baz &&\n> +\t\tgit checkout -b ambiguous_branch_and_file &&\n> +\t\ttest_commit b_ambiguous_branch_and_file\n>  \t) &&\n>  \tgit remote add repo_a repo_a &&\n>  \tgit remote add repo_b repo_b &&\n> @@ -75,6 +79,21 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '\n>  \ttest_branch master\n>  '\n>\n> +test_expect_success 'when arg matches multiple remotes, do not fallback to interpreting as pathspec' '\n> +\tgit checkout -b t_ambiguous_branch_and_file &&\n> +\t>ambiguous_branch_and_file &&\n> +\tgit add ambiguous_branch_and_file &&\n> +\tgit commit -m \"ambiguous_branch_and_file\" &&\n> +\n> +\ttest_when_finished \"git checkout -- ambiguous_branch_and_file\" &&\n> +\techo \"file contents\" >ambiguous_branch_and_file &&\n> +\tcp ambiguous_branch_and_file expect &&\n> +\n> +\ttest_must_fail git checkout ambiguous_branch_and_file &&\n> +\n> +\ttest_cmp expect ambiguous_branch_and_file\n> +'\n> +\n>  test_expect_success 'checkout of branch from multiple remotes fails with advice' '\n>  \tgit checkout -B master &&\n>  \ttest_might_fail git branch -D foo &&\n"},{"id":"387170","messageId":"08b8acc1-6925-ad40-faa2-88a16b8d135f@syntevo.com","threadId":"52356","inReplyTo":"87v9r5nx3w.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] checkout: die() on ambiguous tracking branches","fromName":"Alexandr Miloslavskiy","fromEmail":"alexandr.miloslavskiy@syntevo.com","sentAt":"2019-11-27T16:09:04Z","receivedAt":"2019-11-27T16:09:10Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"\n\nOn 27.11.2019 16:43, Ævar Arnfjörð Bjarmason wrote:\n> I'll reserve judgement on whether we really should do this for now, my\n> current opinion on the matter is undefined as I haven't re-paged this\n> behavior of checkout into my brain.\n> \n> But a giant red flag here for me is that you say \"I understand that this\n> was never intended\".\n> \n> Just from a cursory look at this that's not true, for better or worse it\n> *is* intended behavior. Most of the code you're moving around here is\n> what I added in ad8d5104b4 (\"checkout: add advice for ambiguous\n> \"checkout <branch>\"\", 2018-06-05), and the very start of that commit\n> message refers to the checkout documentation we have that explicitly\n> documents this edge case.\n> \n> Digging a bit further reveals that we've had this behavior (again,\n> intended, not emergent) since 70c9ac2f19 (\"DWIM \"git checkout frotz\" to\n> \"git checkout -b frotz origin/frotz\"\", 2009-10-18), and had it\n> documented since 00bb4378c7 (\"Documentation/git-checkout.txt: document\n> 70c9ac2 behavior\", 2012-12-17).\n> \n> So at the very least I'd say you need a v2 where you amend the relevant\n> docs & commit message to make a case to the effect of \"we've had this\n> since 2009, but it was never really all that important etc.\".\n> \n\nI'm sorry, can you please clarify?\n\nMy patch addresses the situation where there are *multiple* remote \ncandidates *and* a file with the same name.\n\nMy feeling is that in this case, reverting a file is an unintended surprise.\n\nI understand previous patches this way:\nad8d5104 - patch series is mostly for \"if there are *multiple* remotes,\n            disambiguate via checkout.defaultRemote\". This essentially\n            converts the case of multiple remotes into a single remote.\n            However, this also semi-documents what I'm now preventing.\n            Was that really intended?\n70c9ac2f - if there is *one* remote, DWIM it.\n            This isn't what I'm changing.\nbe4908f1 - if there is *one* remote *and* file, die().\n            This is what I'm extending further.\n\nIf the prevented behavior is documented, could you please quote it \nexplicitly?\n\n> Such a change should also be changing the docs etc. added in 8d7b558bae\n> (\"checkout & worktree: introduce checkout.defaultRemote\",\n> 2018-06-05). With this series our docs don't make a lot of sense anymore\n> & don't describe the behavior with the patches applied.\n\nTo my understanding, my patch doesn't affect those pieces of docs? \nPlease correct me if I'm wrong.\n"},{"id":"387176","messageId":"94480425-1827-48d6-b7d7-966acc536c6c@syntevo.com","threadId":"52356","inReplyTo":"b6963515-1f28-74ab-42bc-a14a07856118@gmail.com","subject":"Re: [PATCH 2/2] checkout: die() on ambiguous tracking branches","fromName":"Alexandr Miloslavskiy","fromEmail":"alexandr.miloslavskiy@syntevo.com","sentAt":"2019-11-27T16:42:58Z","receivedAt":"2019-11-27T16:43:03Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"On 27.11.2019 15:46, Derrick Stolee wrote:\n>> +\tif (!remote && (num_matches > 1)) {\n> \n> I believe the parentheses around \"num_matched > 1\" are not needed here.\n\nFixed in v2.\n\n> I like that you added a test for this non-obvious situation. One thing that\n> would help ensure that your test is checking the right thing is to redirect\n> stderr to a file and grep for the die() message you included. Something like:\n> \n> \ttest_must_fail git checkout ambiguous_branch_and_file 2>err &&\n> \ttest_i18ngrep \"matched multiple (2) remote tracking branches\" err &&\n\nThis is a good idea, thanks! Added in v2.\n"},{"id":"387177","messageId":"pull.477.v2.git.1574872991.gitgitgadget@gmail.com","threadId":"52356","inReplyTo":"pull.477.git.1574848137.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] checkout: die() on ambiguous tracking branches","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-27T16:43:08Z","receivedAt":"2019-11-27T16:43:15Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"Please refer to commit messages for details.\n\nRationale: this issue was standing in my way while working on my \n--pathspec-from-file patches. Since this looks like an oversight, I decided\nto fix it.\n\n\n----------------------------------------------------------------------------\n\nChanges from v1:\n\n1) As suggested, removed parentheses in '&& (num_matches > 1)' 2) As\nsuggested, added additional checks in test. 3) Added some code comments in\ntest\n\nAlexandr Miloslavskiy (2):\n  parse_branchname_arg(): extract part as new function\n  checkout: die() on ambiguous tracking branches\n\n builtin/checkout.c       | 71 ++++++++++++++++++++++------------------\n t/t2024-checkout-dwim.sh | 28 ++++++++++++++--\n 2 files changed, 65 insertions(+), 34 deletions(-)\n\n\nbase-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-477%2FSyntevoAlex%2F%230224(git)_deny_ambiguous_checkout-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-477/SyntevoAlex/#0224(git)_deny_ambiguous_checkout-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/477\n\nRange-diff vs v1:\n\n 1:  c5b92ce3ed = 1:  1e40cc485b parse_branchname_arg(): extract part as new function\n 2:  7dde1a3b4e ! 2:  575eeb97ba checkout: die() on ambiguous tracking branches\n     @@ -54,7 +54,7 @@\n       \t\t    arg);\n       \t}\n       \n     -+\tif (!remote && (num_matches > 1)) {\n     ++\tif (!remote && num_matches > 1) {\n      +\t    if (advice_checkout_ambiguous_remote_branch_name) {\n      +\t\t    advise(_(\"If you meant to check out a remote tracking branch on, e.g. 'origin',\\n\"\n      +\t\t\t     \"you can do so by fully qualifying the name with the --track option:\\n\"\n     @@ -174,17 +174,22 @@\n       '\n       \n      +test_expect_success 'when arg matches multiple remotes, do not fallback to interpreting as pathspec' '\n     ++\t# create a file with name matching remote branch name\n      +\tgit checkout -b t_ambiguous_branch_and_file &&\n      +\t>ambiguous_branch_and_file &&\n      +\tgit add ambiguous_branch_and_file &&\n      +\tgit commit -m \"ambiguous_branch_and_file\" &&\n      +\n     ++\t# modify file to verify that it will not be touched by checkout\n      +\ttest_when_finished \"git checkout -- ambiguous_branch_and_file\" &&\n      +\techo \"file contents\" >ambiguous_branch_and_file &&\n      +\tcp ambiguous_branch_and_file expect &&\n      +\n     -+\ttest_must_fail git checkout ambiguous_branch_and_file &&\n     ++\ttest_must_fail git checkout ambiguous_branch_and_file 2>err &&\n      +\n     ++\ttest_i18ngrep \"matched multiple (2) remote tracking branches\" err &&\n     ++\t\n     ++\t# file must not be altered\n      +\ttest_cmp expect ambiguous_branch_and_file\n      +'\n      +\n\n-- \ngitgitgadget\n"},{"id":"387178","messageId":"575eeb97bac2b1f5864aeb3c195547398f00db15.1574872991.git.gitgitgadget@gmail.com","threadId":"52356","inReplyTo":"pull.477.v2.git.1574872991.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] checkout: die() on ambiguous tracking branches","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-27T16:43:10Z","receivedAt":"2019-11-27T16:43:17Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>\n\nBefore this patch, when there were multiple DWIM candidates for remote\nbranch, git decided to try the argument as pathspec instead. I believe\nthat such behavior is a surprise: adding another remote suddenly causes\ngit to discard file contents, because it was unsure which branch to\npick. There was an incomplete attempt to prevent that in [3].\n\nI understand that this was never intended:\n\n  [1] introduces the unexpected behavior. Before, there was fallback\n  from not-a-ref to pathspec. This is reasonable DWIM. After, there is\n  another fallback from ambiguous-remote to pathspec. I understand that\n  it was kind of copy&paste oversight.\n\n  [2] noticed the unexpected behavior but chose to semi-document it\n  instead of forbidding, because the goal of the patch series was\n  focused on something else.\n\n  [3] adds `die()` when there is ambiguity between branch and file. The\n  case of multiple tracking branches is seemingly overlooked.\n\nChange to complain about ambiguity instead of doing unexpected things.\n\n[1] Commit 70c9ac2f (\"DWIM \"git checkout frotz\" to \"git checkout -b frotz origin/frotz\"\" 2009-10-18)\n    https://public-inbox.org/git/7vaazpxha4.fsf_-_@alter.siamese.dyndns.org/\n[2] Commit ad8d5104 (\"checkout: add advice for ambiguous \"checkout <branch>\"\", 2018-06-05)\n    https://public-inbox.org/git/20180502105452.17583-1-avarab@gmail.com/\n[3] Commit be4908f1 (\"checkout: disambiguate dwim tracking branches and local files\", 2018-11-13)\n    https://public-inbox.org/git/20181110120707.25846-1-pclouds@gmail.com/\n\nSigned-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>\n---\n builtin/checkout.c       | 56 ++++++++++++++++++----------------------\n t/t2024-checkout-dwim.sh | 28 ++++++++++++++++++--\n 2 files changed, 51 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex e1b9df1543..b847695d2b 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1115,10 +1115,10 @@ static void setup_new_branch_info_and_source_tree(\n \n static const char *parse_remote_branch(const char *arg,\n \t\t\t\t       struct object_id *rev,\n-\t\t\t\t       int could_be_checkout_paths,\n-\t\t\t\t       int *dwim_remotes_matched)\n+\t\t\t\t       int could_be_checkout_paths)\n {\n-\tconst char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);\n+\tint num_matches = 0;\n+\tconst char *remote = unique_tracking_name(arg, rev, &num_matches);\n \t\n \tif (remote && could_be_checkout_paths) {\n \t\tdie(_(\"'%s' could be both a local file and a tracking branch.\\n\"\n@@ -1126,6 +1126,22 @@ static const char *parse_remote_branch(const char *arg,\n \t\t    arg);\n \t}\n \n+\tif (!remote && num_matches > 1) {\n+\t    if (advice_checkout_ambiguous_remote_branch_name) {\n+\t\t    advise(_(\"If you meant to check out a remote tracking branch on, e.g. 'origin',\\n\"\n+\t\t\t     \"you can do so by fully qualifying the name with the --track option:\\n\"\n+\t\t\t     \"\\n\"\n+\t\t\t     \"    git checkout --track origin/<name>\\n\"\n+\t\t\t     \"\\n\"\n+\t\t\t     \"If you'd like to always have checkouts of an ambiguous <name> prefer\\n\"\n+\t\t\t     \"one remote, e.g. the 'origin' remote, consider setting\\n\"\n+\t\t\t     \"checkout.defaultRemote=origin in your config.\"));\n+\t    }\n+\n+\t    die(_(\"'%s' matched multiple (%d) remote tracking branches\"),\n+\t\targ, num_matches);\n+\t}\n+\n \treturn remote;\n }\n \n@@ -1133,8 +1149,7 @@ 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_branch_info,\n \t\t\t\tstruct checkout_opts *opts,\n-\t\t\t\tstruct object_id *rev,\n-\t\t\t\tint *dwim_remotes_matched)\n+\t\t\t\tstruct object_id *rev)\n {\n \tconst char **new_branch = &opts->new_branch;\n \tint argcount = 0;\n@@ -1240,8 +1255,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \n \t\tif (recover_with_dwim) {\n \t\t\tconst char *remote = parse_remote_branch(arg, rev,\n-\t\t\t\t\t\t\t\t could_be_checkout_paths,\n-\t\t\t\t\t\t\t\t dwim_remotes_matched);\n+\t\t\t\t\t\t\t\t could_be_checkout_paths);\n \t\t\tif (remote) {\n \t\t\t\t*new_branch = arg;\n \t\t\t\targ = remote;\n@@ -1505,7 +1519,6 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t const char * const usagestr[])\n {\n \tstruct branch_info new_branch_info;\n-\tint dwim_remotes_matched = 0;\n \tint parseopt_flags = 0;\n \n \tmemset(&new_branch_info, 0, sizeof(new_branch_info));\n@@ -1613,8 +1626,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\topts->track == BRANCH_TRACK_UNSPECIFIED &&\n \t\t\t!opts->new_branch;\n \t\tint n = parse_branchname_arg(argc, argv, dwim_ok,\n-\t\t\t\t\t     &new_branch_info, opts, &rev,\n-\t\t\t\t\t     &dwim_remotes_matched);\n+\t\t\t\t\t     &new_branch_info, opts, &rev);\n \t\targv += n;\n \t\targc -= n;\n \t} else if (!opts->accept_ref && opts->from_treeish) {\n@@ -1672,28 +1684,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t}\n \n \tUNLEAK(opts);\n-\tif (opts->patch_mode || opts->pathspec.nr) {\n-\t\tint ret = checkout_paths(opts, new_branch_info.name);\n-\t\tif (ret && dwim_remotes_matched > 1 &&\n-\t\t    advice_checkout_ambiguous_remote_branch_name)\n-\t\t\tadvise(_(\"'%s' matched more than one remote tracking branch.\\n\"\n-\t\t\t\t \"We found %d remotes with a reference that matched. So we fell back\\n\"\n-\t\t\t\t \"on trying to resolve the argument as a path, but failed there too!\\n\"\n-\t\t\t\t \"\\n\"\n-\t\t\t\t \"If you meant to check out a remote tracking branch on, e.g. 'origin',\\n\"\n-\t\t\t\t \"you can do so by fully qualifying the name with the --track option:\\n\"\n-\t\t\t\t \"\\n\"\n-\t\t\t\t \"    git checkout --track origin/<name>\\n\"\n-\t\t\t\t \"\\n\"\n-\t\t\t\t \"If you'd like to always have checkouts of an ambiguous <name> prefer\\n\"\n-\t\t\t\t \"one remote, e.g. the 'origin' remote, consider setting\\n\"\n-\t\t\t\t \"checkout.defaultRemote=origin in your config.\"),\n-\t\t\t       argv[0],\n-\t\t\t       dwim_remotes_matched);\n-\t\treturn ret;\n-\t} else {\n+\tif (opts->patch_mode || opts->pathspec.nr)\n+\t\treturn checkout_paths(opts, new_branch_info.name);\n+\telse\n \t\treturn checkout_branch(opts, &new_branch_info);\n-\t}\n }\n \n int cmd_checkout(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\nindex fa0718c730..c35d67b697 100755\n--- a/t/t2024-checkout-dwim.sh\n+++ b/t/t2024-checkout-dwim.sh\n@@ -37,7 +37,9 @@ test_expect_success 'setup' '\n \t\tgit checkout -b foo &&\n \t\ttest_commit a_foo &&\n \t\tgit checkout -b bar &&\n-\t\ttest_commit a_bar\n+\t\ttest_commit a_bar &&\n+\t\tgit checkout -b ambiguous_branch_and_file &&\n+\t\ttest_commit a_ambiguous_branch_and_file\n \t) &&\n \tgit init repo_b &&\n \t(\n@@ -46,7 +48,9 @@ test_expect_success 'setup' '\n \t\tgit checkout -b foo &&\n \t\ttest_commit b_foo &&\n \t\tgit checkout -b baz &&\n-\t\ttest_commit b_baz\n+\t\ttest_commit b_baz &&\n+\t\tgit checkout -b ambiguous_branch_and_file &&\n+\t\ttest_commit b_ambiguous_branch_and_file\n \t) &&\n \tgit remote add repo_a repo_a &&\n \tgit remote add repo_b repo_b &&\n@@ -75,6 +79,26 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '\n \ttest_branch master\n '\n \n+test_expect_success 'when arg matches multiple remotes, do not fallback to interpreting as pathspec' '\n+\t# create a file with name matching remote branch name\n+\tgit checkout -b t_ambiguous_branch_and_file &&\n+\t>ambiguous_branch_and_file &&\n+\tgit add ambiguous_branch_and_file &&\n+\tgit commit -m \"ambiguous_branch_and_file\" &&\n+\n+\t# modify file to verify that it will not be touched by checkout\n+\ttest_when_finished \"git checkout -- ambiguous_branch_and_file\" &&\n+\techo \"file contents\" >ambiguous_branch_and_file &&\n+\tcp ambiguous_branch_and_file expect &&\n+\n+\ttest_must_fail git checkout ambiguous_branch_and_file 2>err &&\n+\n+\ttest_i18ngrep \"matched multiple (2) remote tracking branches\" err &&\n+\t\n+\t# file must not be altered\n+\ttest_cmp expect ambiguous_branch_and_file\n+'\n+\n test_expect_success 'checkout of branch from multiple remotes fails with advice' '\n \tgit checkout -B master &&\n \ttest_might_fail git branch -D foo &&\n-- \ngitgitgadget\n"},{"id":"387179","messageId":"1e40cc485b5a25189310053cedfd3a335b225552.1574872991.git.gitgitgadget@gmail.com","threadId":"52356","inReplyTo":"pull.477.v2.git.1574872991.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] parse_branchname_arg(): extract part as new function","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-27T16:43:09Z","receivedAt":"2019-11-27T16:43:18Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>\n\nThis is done for the next commit to avoid crazy 7x tab code padding.\n\nSigned-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>\n---\n builtin/checkout.c | 25 +++++++++++++++++++------\n 1 file changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3634a3dac1..e1b9df1543 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1113,6 +1113,22 @@ static void setup_new_branch_info_and_source_tree(\n \t}\n }\n \n+static const char *parse_remote_branch(const char *arg,\n+\t\t\t\t       struct object_id *rev,\n+\t\t\t\t       int could_be_checkout_paths,\n+\t\t\t\t       int *dwim_remotes_matched)\n+{\n+\tconst char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);\n+\t\n+\tif (remote && could_be_checkout_paths) {\n+\t\tdie(_(\"'%s' could be both a local file and a tracking branch.\\n\"\n+\t\t\t\"Please use -- (and optionally --no-guess) to disambiguate\"),\n+\t\t    arg);\n+\t}\n+\n+\treturn remote;\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_branch_info,\n@@ -1223,13 +1239,10 @@ static int parse_branchname_arg(int argc, const char **argv,\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\t\t\t\t\t\t  dwim_remotes_matched);\n+\t\t\tconst char *remote = parse_remote_branch(arg, rev,\n+\t\t\t\t\t\t\t\t could_be_checkout_paths,\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-- \ngitgitgadget\n\n"},{"id":"387376","messageId":"xmqqwobgyrnf.fsf@gitster-ct.c.googlers.com","threadId":"52356","inReplyTo":"pull.477.v2.git.1574872991.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] checkout: die() on ambiguous tracking branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-01T15:47:16Z","receivedAt":"2019-12-01T15:47:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alexandr Miloslavskiy via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> Please refer to commit messages for details.\n\nWhen sending an updated series after getting review comments from\nothers to your earlier round of the same series, please make sure\nthese reviewers are CC'ed, especially those who thought the earlier\none(s) were not quite right, so that they can say \"oh, I changed my\nmind.  This round is good\", or \"I think it is still wrong in this\nround, because of ...\".\n\nYou do not necessarily have to include me on CC: line before the\nlist seems to have reached concensus that the patches are good.\n\nThanks.\n"},{"id":"387379","messageId":"10e7d07d-9432-1c41-289e-2be86539aa56@syntevo.com","threadId":"52356","inReplyTo":"xmqqwobgyrnf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/2] checkout: die() on ambiguous tracking branches","fromName":"Alexandr Miloslavskiy","fromEmail":"alexandr.miloslavskiy@syntevo.com","sentAt":"2019-12-01T16:52:34Z","receivedAt":"2019-12-01T16:52:48Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"On 01.12.2019 16:47, Junio C Hamano wrote:\n> When sending an updated series after getting review comments from\n> others to your earlier round of the same series, please make sure\n> these reviewers are CC'ed, especially those who thought the earlier\n> one(s) were not quite right, so that they can say \"oh, I changed my\n> mind.  This round is good\", or \"I think it is still wrong in this\n> round, because of ...\".\n> \n> You do not necessarily have to include me on CC: line before the\n> list seems to have reached concensus that the patches are good.\n\nOK, thanks for explaining! I'm new and didn't know about that.\n\nFor this patch, I already informed reviewers via explicit replies. From \nthe next one, I'll try to remember to also CC them.\n"},{"id":"387560","messageId":"065c02a7-8ef9-192b-097e-57913db05520@syntevo.com","threadId":"52356","inReplyTo":"08b8acc1-6925-ad40-faa2-88a16b8d135f@syntevo.com","subject":"Re: [PATCH 2/2] checkout: die() on ambiguous tracking branches","fromName":"Alexandr Miloslavskiy","fromEmail":"alexandr.miloslavskiy@syntevo.com","sentAt":"2019-12-05T15:34:17Z","receivedAt":"2019-12-05T15:34:25Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"Ævar, please let me know if your concern is resolved?\n\nOn 27.11.2019 17:09, Alexandr Miloslavskiy wrote:\n> \n> \n> On 27.11.2019 16:43, Ævar Arnfjörð Bjarmason wrote:\n>> I'll reserve judgement on whether we really should do this for now, my\n>> current opinion on the matter is undefined as I haven't re-paged this\n>> behavior of checkout into my brain.\n>>\n>> But a giant red flag here for me is that you say \"I understand that this\n>> was never intended\".\n>>\n>> Just from a cursory look at this that's not true, for better or worse it\n>> *is* intended behavior. Most of the code you're moving around here is\n>> what I added in ad8d5104b4 (\"checkout: add advice for ambiguous\n>> \"checkout <branch>\"\", 2018-06-05), and the very start of that commit\n>> message refers to the checkout documentation we have that explicitly\n>> documents this edge case.\n>>\n>> Digging a bit further reveals that we've had this behavior (again,\n>> intended, not emergent) since 70c9ac2f19 (\"DWIM \"git checkout frotz\" to\n>> \"git checkout -b frotz origin/frotz\"\", 2009-10-18), and had it\n>> documented since 00bb4378c7 (\"Documentation/git-checkout.txt: document\n>> 70c9ac2 behavior\", 2012-12-17).\n>>\n>> So at the very least I'd say you need a v2 where you amend the relevant\n>> docs & commit message to make a case to the effect of \"we've had this\n>> since 2009, but it was never really all that important etc.\".\n>>\n> \n> I'm sorry, can you please clarify?\n> \n> My patch addresses the situation where there are *multiple* remote \n> candidates *and* a file with the same name.\n> \n> My feeling is that in this case, reverting a file is an unintended \n> surprise.\n> \n> I understand previous patches this way:\n> ad8d5104 - patch series is mostly for \"if there are *multiple* remotes,\n>             disambiguate via checkout.defaultRemote\". This essentially\n>             converts the case of multiple remotes into a single remote.\n>             However, this also semi-documents what I'm now preventing.\n>             Was that really intended?\n> 70c9ac2f - if there is *one* remote, DWIM it.\n>             This isn't what I'm changing.\n> be4908f1 - if there is *one* remote *and* file, die().\n>             This is what I'm extending further.\n> \n> If the prevented behavior is documented, could you please quote it \n> explicitly?\n> \n>> Such a change should also be changing the docs etc. added in 8d7b558bae\n>> (\"checkout & worktree: introduce checkout.defaultRemote\",\n>> 2018-06-05). With this series our docs don't make a lot of sense anymore\n>> & don't describe the behavior with the patches applied.\n> \n> To my understanding, my patch doesn't affect those pieces of docs? \n> Please correct me if I'm wrong.\n\n"}]}