checkout: don't revert file on ambiguous tracking branches
cover checkout: don't revert file on ambiguous tracking branches
This is an improved version of [1]; I tried to clarify the commit message. CC'ing authors of previous commits mentioned in my commit message.
[1] https://public-inbox.org/git/[email protected]/T/#u
Alexandr Miloslavskiy (2):
parse_branchname_arg(): extract part as new function
checkout: don't revert file on ambiguous tracking branches
builtin/checkout.c | 71 ++++++++++++++++++++++------------------ t/t2024-checkout-dwim.sh | 28 ++++++++++++++-- 2 files changed, 65 insertions(+), 34 deletions(-)
base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-504%2FSyntevoAlex%2F%230207(git)_2c_prevent_ambiguous_checkout-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-504/SyntevoAlex/#0207(git)_2c_prevent_ambiguous_checkout-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/504
Signature · 1 line
gitgitgadget
PATCH 1/2 parse_branchname_arg(): extract part as new function
From: Alexandr Miloslavskiy <[email protected]>
This is done for the next commit to avoid crazy 7x tab code padding.
--- builtin/checkout.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)
builtin/checkout.c +19 -6
@@ -1115,6 +1115,22 @@ static void setup_new_branch_info_and_source_tree( } } +static const char *parse_remote_branch (const char *arg, +struct object_id *rev, +int could_be_checkout_paths, +int *dwim_remotes_matched) +{ +const char *remote =unique_tracking_name (arg, rev, dwim_remotes_matched); + +if (remote && could_be_checkout_paths) { +die (_ ("'%s' could be both a local file and a tracking branch.\n" +"Please use -- (and optionally --no-guess) to disambiguate" ), + arg); + } + +return remote; +} + static int parse_branchname_arg(int argc, const char **argv, int dwim_new_local_branch_ok, struct branch_info *new_branch_info,
@@ -1225,13 +1241,10 @@ static int parse_branchname_arg(int argc, const char **argv, recover_with_dwim =0 ;if (recover_with_dwim) { -const char *remote =unique_tracking_name (arg, rev, - dwim_remotes_matched); +const char *remote =parse_remote_branch (arg, rev, + could_be_checkout_paths, + dwim_remotes_matched);if (remote) { -if (could_be_checkout_paths) -die (_ ("'%s' could be both a local file and a tracking branch.\n" -"Please use -- (and optionally --no-guess) to disambiguate" ), - arg); *new_branch = arg; arg = remote;/* DWIMmed to create local branch, case (3).(b) */
Signature · 1 line
gitgitgadget
PATCH 2/2 checkout: don't revert file on ambiguous tracking branches
In reply to Alexandr Miloslavskiy via GitGitGadgetFrom: Alexandr Miloslavskiy <[email protected]>
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/[email protected]/
[2] Commit ad8d5104 ("checkout: add advice for ambiguous "checkout <branch>"", 2018-06-05)
https://public-inbox.org/git/[email protected]/ [3] Commit be4908f1 ("checkout: disambiguate dwim tracking branches and local files", 2018-11-13) https://public-inbox.org/git/[email protected]/
--- builtin/checkout.c | 56 ++++++++++++++++++---------------------- t/t2024-checkout-dwim.sh | 28 ++++++++++++++++++-- 2 files changed, 51 insertions(+), 33 deletions(-)
builtin/checkout.c +25 -31
@@ -1117,10 +1117,10 @@ static void setup_new_branch_info_and_source_tree(static const char *parse_remote_branch (const char *arg,struct object_id *rev, -int could_be_checkout_paths, -int *dwim_remotes_matched) +int could_be_checkout_paths) { -const char *remote =unique_tracking_name (arg, rev, dwim_remotes_matched); +int num_matches =0 ; +const char *remote =unique_tracking_name (arg, rev, &num_matches);if (remote && could_be_checkout_paths) { die(_("'%s' could be both a local file and a tracking branch.\n"
@@ -1128,6 +1128,22 @@ static const char *parse_remote_branch(const char *arg, arg); } +if (!remote && num_matches >1 ) { +if (advice_checkout_ambiguous_remote_branch_name) { +advise (_ ("If you meant to check out a remote tracking branch on, e.g. 'origin',\n" +"you can do so by fully qualifying the name with the --track option:\n" +"\n" +" git checkout --track origin/<name>\n" +"\n" +"If you'd like to always have checkouts of an ambiguous <name> prefer\n" +"one remote, e.g. the 'origin' remote, consider setting\n" +"checkout.defaultRemote=origin in your config." )); + } + +die (_ ("'%s' matched multiple (%d) remote tracking branches" ), + arg, num_matches); + } +return remote; }
@@ -1135,8 +1151,7 @@ static int parse_branchname_arg(int argc, const char **argv,int dwim_new_local_branch_ok, struct branch_info *new_branch_info, struct checkout_opts *opts, - struct object_id *rev, - int *dwim_remotes_matched) + struct object_id *rev) { const char **new_branch = &opts->new_branch;int argcount =0 ;
@@ -1242,8 +1257,7 @@ static int parse_branchname_arg(int argc, const char **argv,if (recover_with_dwim) {const char *remote =parse_remote_branch (arg, rev, - could_be_checkout_paths, - dwim_remotes_matched); + could_be_checkout_paths);if (remote) { *new_branch = arg; arg = remote;
@@ -1509,7 +1523,6 @@ static int checkout_main(int argc, const char **argv, const char *prefix, const char * const usagestr[]) {struct branch_info new_branch_info; -int dwim_remotes_matched =0 ;int parseopt_flags =0 ;memset (&new_branch_info,0 ,sizeof (new_branch_info));
@@ -1617,8 +1630,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, opts->track ==BRANCH_TRACK_UNSPECIFIED && !opts->new_branch;int n =parse_branchname_arg (argc, argv, dwim_ok, - &new_branch_info, opts, &rev, - &dwim_remotes_matched); + &new_branch_info, opts, &rev); argv += n; argc -= n; }else if (!opts->accept_ref && opts->from_treeish) {
@@ -1695,28 +1707,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix, }UNLEAK (opts); -if (opts->patch_mode || opts->pathspec.nr) { -int ret =checkout_paths (opts, new_branch_info.name); -if (ret && dwim_remotes_matched >1 && - advice_checkout_ambiguous_remote_branch_name) -advise (_ ("'%s' matched more than one remote tracking branch.\n" -"We found %d remotes with a reference that matched. So we fell back\n" -"on trying to resolve the argument as a path, but failed there too!\n" -"\n" -"If you meant to check out a remote tracking branch on, e.g. 'origin',\n" -"you can do so by fully qualifying the name with the --track option:\n" -"\n" -" git checkout --track origin/<name>\n" -"\n" -"If you'd like to always have checkouts of an ambiguous <name> prefer\n" -"one remote, e.g. the 'origin' remote, consider setting\n" -"checkout.defaultRemote=origin in your config." ), - argv[0 ], - dwim_remotes_matched); -return ret; - }else { +if (opts->patch_mode || opts->pathspec.nr) +return checkout_paths (opts, new_branch_info.name); +else return checkout_branch (opts, &new_branch_info); - } }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' ' git checkout -b foo && test_commit a_foo && git checkout -b bar && - test_commit a_bar + test_commit a_bar && + git checkout -b ambiguous_branch_and_file && + test_commit a_ambiguous_branch_and_file ) && git init repo_b && (
@@ -46,7 +48,9 @@ test_expect_success 'setup' ' git checkout -b foo && test_commit b_foo && git checkout -b baz && - test_commit b_baz + test_commit b_baz && + git checkout -b ambiguous_branch_and_file && + test_commit b_ambiguous_branch_and_file ) && git remote add repo_a repo_a && git remote add repo_b repo_b &&
@@ -75,6 +79,26 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' ' test_branch master ' +test_expect_success 'when arg matches multiple remotes, do not fallback to interpreting as pathspec' ' + # create a file with name matching remote branch name + git checkout -b t_ambiguous_branch_and_file && + >ambiguous_branch_and_file && + git add ambiguous_branch_and_file && + git commit -m "ambiguous_branch_and_file" && + + # modify file to verify that it will not be touched by checkout + test_when_finished "git checkout -- ambiguous_branch_and_file" && + echo "file contents" >ambiguous_branch_and_file && + cp ambiguous_branch_and_file expect && + + test_must_fail git checkout ambiguous_branch_and_file 2>err && + + test_i18ngrep "matched multiple (2) remote tracking branches" err && + + # file must not be altered + test_cmp expect ambiguous_branch_and_file +' + test_expect_success 'checkout of branch from multiple remotes fails with advice' ' git checkout -B master && test_might_fail git branch -D foo &&
Signature · 1 line
gitgitgadget