{"thread":{"id":"52541","subject":"[PATCH 0/2] checkout: don't revert file on ambiguous tracking branches","startedAt":"2019-12-30T18:38:17Z","lastAt":"2019-12-30T18:38:20Z","messageCount":3,"participants":["Alexandr Miloslavskiy via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"389085","messageId":"pull.504.git.1577731093.gitgitgadget@gmail.com","threadId":"52541","inReplyTo":null,"subject":"[PATCH 0/2] checkout: don't revert file on ambiguous tracking branches","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-30T18:38:11Z","receivedAt":"2019-12-30T18:38:17Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"This is an improved version of [1]; I tried to clarify the commit message.\nCC'ing authors of previous commits mentioned in my commit message.\n\n[1] https://public-inbox.org/git/pull.477.git.1574848137.gitgitgadget@gmail.com/T/#u\n\nAlexandr Miloslavskiy (2):\n  parse_branchname_arg(): extract part as new function\n  checkout: don't revert file 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: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-504%2FSyntevoAlex%2F%230207(git)_2c_prevent_ambiguous_checkout-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-504/SyntevoAlex/#0207(git)_2c_prevent_ambiguous_checkout-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/504\n-- \ngitgitgadget\n"},{"id":"389086","messageId":"8ab243c2ccf81ded7a32155dde676a8d368ea567.1577731093.git.gitgitgadget@gmail.com","threadId":"52541","inReplyTo":"pull.504.git.1577731093.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-12-30T18:38:12Z","receivedAt":"2019-12-30T18:38:19Z","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 b52c490c8f..f832040e94 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1115,6 +1115,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+\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@@ -1225,13 +1241,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":"389087","messageId":"12229e45fdcdce4b3e67f92b3945162eb6ff11cc.1577731093.git.gitgitgadget@gmail.com","threadId":"52541","inReplyTo":"pull.504.git.1577731093.gitgitgadget@gmail.com","subject":"[PATCH 2/2] checkout: don't revert file on ambiguous tracking branches","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-30T18:38:13Z","receivedAt":"2019-12-30T18:38:20Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>\n\nFor easier understanding, here are the existing good scenarios:\n\n  1) Have *no* file 'foo', *no* local branch 'foo' and a *single*\n     remote branch 'foo'\n  2) `git checkout foo` will create local branch foo, see [1]\n\n  and\n\n  1) Have *a* file 'foo', *no* local branch 'foo' and a *single*\n     remote branch 'foo'\n  2) `git checkout foo` will complain, see [3]\n\nThis patch prevents the following scenario:\n\n  1) Have *a* file 'foo', *no* local branch 'foo' and *multiple*\n     remote branches 'foo'\n  2) `git checkout foo` will successfully... revert contents of\n     file `foo`!\n\nThat is, adding another remote suddenly changes behavior significantly,\nwhich is a surprise at best and could go unnoticed by user at worst.\nPlease see [3] which gives some real world complaints.\n\nTo my understanding, fix in [3] overlooked the case of multiple remotes,\nand the whole behavior of falling back to reverting file was never\nintended:\n\n  [1] introduces the unexpected behavior. Before, there was fallback\n  from not-a-ref to pathspec. This is reasonable fallback. After, there\n  is another fallback from ambiguous-remote to pathspec. I understand\n  that it was a 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\nThe new behavior: if there is no local branch and multiple remote\ncandidates, just die() and don't try reverting file whether it\nexists (prevents surprise) or not (improves error message).\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 f832040e94..5c41645c7d 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1117,10 +1117,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@@ -1128,6 +1128,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@@ -1135,8 +1151,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@@ -1242,8 +1257,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@@ -1509,7 +1523,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@@ -1617,8 +1630,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@@ -1695,28 +1707,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..accfa9aa4b 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+\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"}]}