@matheusbsbs / matheus-bagliano / commits / fa74180d08

Fork 0

checkout: don't revert file on ambiguous tracking branches

For easier understanding, here are the existing good scenarios: 1) Have *no* file 'foo', *no* local branch 'foo' and a *single* remote branch 'foo' 2) `git checkout foo` will create local branch foo, see [1] and 1) Have *a* file 'foo', *no* local branch 'foo' and a *single* remote branch 'foo' 2) `git checkout foo` will complain, see [3] This patch prevents the following scenario: 1) Have *a* file 'foo', *no* local branch 'foo' and *multiple* remote branches 'foo' 2) `git checkout foo` will successfully... revert contents of file `foo`! That is, adding another remote suddenly changes behavior significantly, which is a surprise at best and could go unnoticed by user at worst. Please see [3] which gives some real world complaints. To my understanding, fix in [3] overlooked the case of multiple remotes, and the whole behavior of falling back to reverting file was never intended: [1] introduces the unexpected behavior. Before, there was fallback from not-a-ref to pathspec. This is reasonable fallback. After, there is another fallback from ambiguous-remote to pathspec. I understand that it was a copy&paste oversight. [2] noticed the unexpected behavior but chose to semi-document it instead of forbidding, because the goal of the patch series was focused on something else. [3] adds `die()` when there is ambiguity between branch and file. The case of multiple tracking branches is seemingly overlooked. The new behavior: if there is no local branch and multiple remote candidates, just die() and don't try reverting file whether it exists (prevents surprise) or not (improves error message). [1] Commit 70c9ac2f ("DWIM "git checkout frotz" to "git checkout -b frotz origin/frotz"" 2009-10-18) https://public-inbox.org/git/7vaazpxha4.fsf_-_@alter.siamese.dyndns.org/ [2] Commit ad8d5104 ("checkout: add advice for ambiguous "checkout <branch>"", 2018-06-05) https://public-inbox.org/git/20180502105452.17583-1-avarab@gmail.com/ [3] Commit be4908f1 ("checkout: disambiguate dwim tracking branches and local files", 2018-11-13) https://public-inbox.org/git/20181110120707.25846-1-pclouds@gmail.com/ Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Alexandr Miloslavskiy committed Dec 30, 2019 at 18:38 UTC fa74180d08e0a82bdae0c127a009b055c603b11a

2 files changed +51 -33

builtin/checkout.c

+25 -31

@@ -1115,10 +1115,10 @@ static void setup_new_branch_info_and_source_tree(

1115

1116 static const char *parse_remote_branch(const char *arg,

1117 struct object_id *rev,

1118 - int could_be_checkout_paths,

1119 - int *dwim_remotes_matched)

1118 + int could_be_checkout_paths)

1119 {

1121 - const char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);

1120 + int num_matches = 0;

1121 + const char *remote = unique_tracking_name(arg, rev, &num_matches);

1122

1123 if (remote && could_be_checkout_paths) {

1124 die(_("'%s' could be both a local file and a tracking branch.\n"

@@ -1126,6 +1126,22 @@ static const char *parse_remote_branch(const char *arg,

1126 arg);

1127 }

1128

1129 + if (!remote && num_matches > 1) {

1130 + if (advice_checkout_ambiguous_remote_branch_name) {

1131 + advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"

1132 + "you can do so by fully qualifying the name with the --track option:\n"

1133 + "\n"

1134 + " git checkout --track origin/<name>\n"

1135 + "\n"

1136 + "If you'd like to always have checkouts of an ambiguous <name> prefer\n"

1137 + "one remote, e.g. the 'origin' remote, consider setting\n"

1138 + "checkout.defaultRemote=origin in your config."));

1139 + }

1140 +

1141 + die(_("'%s' matched multiple (%d) remote tracking branches"),

1142 + arg, num_matches);

1143 + }

1144 +

1145 return remote;

1146 }

1147

@@ -1133,8 +1149,7 @@ static int parse_branchname_arg(int argc, const char **argv,

1149 int dwim_new_local_branch_ok,

1150 struct branch_info *new_branch_info,

1151 struct checkout_opts *opts,

1136 - struct object_id *rev,

1137 - int *dwim_remotes_matched)

1152 + struct object_id *rev)

1153 {

1154 const char **new_branch = &opts->new_branch;

1155 int argcount = 0;

@@ -1240,8 +1255,7 @@ static int parse_branchname_arg(int argc, const char **argv,

1255

1256 if (recover_with_dwim) {

1257 const char *remote = parse_remote_branch(arg, rev,

1243 - could_be_checkout_paths,

1244 - dwim_remotes_matched);

1258 + could_be_checkout_paths);

1259 if (remote) {

1260 *new_branch = arg;

1261 arg = remote;

@@ -1505,7 +1519,6 @@ static int checkout_main(int argc, const char **argv, const char *prefix,

1519 const char * const usagestr[])

1520 {

1521 struct branch_info new_branch_info;

1508 - int dwim_remotes_matched = 0;

1522 int parseopt_flags = 0;

1523

1524 memset(&new_branch_info, 0, sizeof(new_branch_info));

@@ -1613,8 +1626,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,

1626 opts->track == BRANCH_TRACK_UNSPECIFIED &&

1627 !opts->new_branch;

1628 int n = parse_branchname_arg(argc, argv, dwim_ok,

1616 - &new_branch_info, opts, &rev,

1617 - &dwim_remotes_matched);

1629 + &new_branch_info, opts, &rev);

1630 argv += n;

1631 argc -= n;

1632 } else if (!opts->accept_ref && opts->from_treeish) {

@@ -1672,28 +1684,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,

1684 }

1685

1686 UNLEAK(opts);

1675 - if (opts->patch_mode || opts->pathspec.nr) {

1676 - int ret = checkout_paths(opts, new_branch_info.name);

1677 - if (ret && dwim_remotes_matched > 1 &&

1678 - advice_checkout_ambiguous_remote_branch_name)

1679 - advise(_("'%s' matched more than one remote tracking branch.\n"

1680 - "We found %d remotes with a reference that matched. So we fell back\n"

1681 - "on trying to resolve the argument as a path, but failed there too!\n"

1682 - "\n"

1683 - "If you meant to check out a remote tracking branch on, e.g. 'origin',\n"

1684 - "you can do so by fully qualifying the name with the --track option:\n"

1685 - "\n"

1686 - " git checkout --track origin/<name>\n"

1687 - "\n"

1688 - "If you'd like to always have checkouts of an ambiguous <name> prefer\n"

1689 - "one remote, e.g. the 'origin' remote, consider setting\n"

1690 - "checkout.defaultRemote=origin in your config."),

1691 - argv[0],

1692 - dwim_remotes_matched);

1693 - return ret;

1694 - } else {

1687 + if (opts->patch_mode || opts->pathspec.nr)

1688 + return checkout_paths(opts, new_branch_info.name);

1689 + else

1690 return checkout_branch(opts, &new_branch_info);

1696 - }

1691 }

1692

1693 int cmd_checkout(int argc, const char **argv, const char *prefix)

t/t2024-checkout-dwim.sh

+26 -2

@@ -37,7 +37,9 @@ test_expect_success 'setup' '

37 git checkout -b foo &&

38 test_commit a_foo &&

39 git checkout -b bar &&

40 - test_commit a_bar

40 + test_commit a_bar &&

41 + git checkout -b ambiguous_branch_and_file &&

42 + test_commit a_ambiguous_branch_and_file

43 ) &&

44 git init repo_b &&

45 (

@@ -46,7 +48,9 @@ test_expect_success 'setup' '

48 git checkout -b foo &&

49 test_commit b_foo &&

50 git checkout -b baz &&

49 - test_commit b_baz

51 + test_commit b_baz &&

52 + git checkout -b ambiguous_branch_and_file &&

53 + test_commit b_ambiguous_branch_and_file

54 ) &&

55 git remote add repo_a repo_a &&

56 git remote add repo_b repo_b &&

@@ -75,6 +79,26 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '

79 test_branch master

80 '

82 +test_expect_success 'when arg matches multiple remotes, do not fallback to interpreting as pathspec' '

83 + # create a file with name matching remote branch name

84 + git checkout -b t_ambiguous_branch_and_file &&

85 + >ambiguous_branch_and_file &&

86 + git add ambiguous_branch_and_file &&

87 + git commit -m "ambiguous_branch_and_file" &&

88 +

89 + # modify file to verify that it will not be touched by checkout

90 + test_when_finished "git checkout -- ambiguous_branch_and_file" &&

91 + echo "file contents" >ambiguous_branch_and_file &&

92 + cp ambiguous_branch_and_file expect &&

93 +

94 + test_must_fail git checkout ambiguous_branch_and_file 2>err &&

95 +

96 + test_i18ngrep "matched multiple (2) remote tracking branches" err &&

97 +

98 + # file must not be altered

99 + test_cmp expect ambiguous_branch_and_file

100 +'

101 +

102 test_expect_success 'checkout of branch from multiple remotes fails with advice' '

103 git checkout -B master &&

104 test_might_fail git branch -D foo &&