From: "Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: "Phillip Wood <phillip.wood123@gmail.com>,
Junio C Hamano" <gitster@pobox.com>,
"Emily Shaffer" <emilyshaffer@google.com>,
"Derrick Stolee" <stolee@gmail.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Alexandr Miloslavskiy" <alexandr.miloslavskiy@syntevo.com>,
"Junio C Hamano" <gitster@pobox.com>,
"Alexandr Miloslavskiy" <alexandr.miloslavskiy@syntevo.com>
Subject: [PATCH 10/16] checkout: die() on ambiguous tracking branches
Date: Thu, 12 Dec 2019 14:36:19 +0000 [thread overview]
Message-ID: <271e8ed3ab1a67dbd190ebeb6fe8c675b79b47e3.1576161385.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.490.git.1576161385.gitgitgadget@gmail.com>
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Before this patch, when there were multiple DWIM candidates for remote
branch, git decided to try the argument as pathspec instead. I believe
that such behavior is a surprise: adding another remote suddenly causes
git to discard file contents, because it was unsure which branch to
pick. There was an incomplete attempt to prevent that in [3].
I understand that this was never intended:
[1] introduces the unexpected behavior. Before, there was fallback
from not-a-ref to pathspec. This is reasonable DWIM. After, there is
another fallback from ambiguous-remote to pathspec. I understand that
it was kind of 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.
Change to complain about ambiguity instead of doing unexpected things.
[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>
---
builtin/checkout.c | 56 ++++++++++++++++++----------------------
t/t2024-checkout-dwim.sh | 28 ++++++++++++++++++--
2 files changed, 51 insertions(+), 33 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index e1b9df1543..b847695d2b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1115,10 +1115,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"
@@ -1126,6 +1126,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;
}
@@ -1133,8 +1149,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;
@@ -1240,8 +1255,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;
@@ -1505,7 +1519,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));
@@ -1613,8 +1626,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) {
@@ -1672,28 +1684,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)
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index fa0718c730..c35d67b697 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -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 &&
--
gitgitgadget
next prev parent reply other threads:[~2019-12-12 14:36 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-12 14:36 [PATCH 00/16] Extend --pathspec-from-file to git add, checkout Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` [PATCH 01/16] t7107, t7526: directly test parse_pathspec_file() Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` [PATCH 02/16] commit: forbid --pathspec-from-file --all Alexandr Miloslavskiy via GitGitGadget
2019-12-16 12:02 ` Phillip Wood
2019-12-16 15:53 ` Alexandr Miloslavskiy
2019-12-12 14:36 ` [PATCH 03/16] cmd_add: prepare for next patch Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` [PATCH 04/16] add: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` [PATCH 05/16] doc: checkout: remove duplicate synopsis Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` [PATCH 06/16] doc: checkout: fix broken text reference Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` [PATCH 07/16] doc: checkout: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` [PATCH 08/16] doc: restore: " Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` [PATCH 09/16] parse_branchname_arg(): extract part as new function Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` Alexandr Miloslavskiy via GitGitGadget [this message]
2019-12-12 14:36 ` [PATCH 11/16] parse_branchname_arg(): easier to understand variables Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` [PATCH 12/16] parse_branchname_arg(): introduce expect_commit_only Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` [PATCH 13/16] parse_branchname_arg(): update code comments Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` [PATCH 14/16] parse_branchname_arg(): refactor the decision making Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` [PATCH 15/16] t2024: cover more cases Alexandr Miloslavskiy via GitGitGadget
2019-12-12 14:36 ` [PATCH 16/16] checkout, restore: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:47 ` [PATCH v2 00/18] Extend --pathspec-from-file to git add, checkout Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:47 ` [PATCH v2 01/18] t7107, t7526: directly test parse_pathspec_file() Alexandr Miloslavskiy via GitGitGadget
2019-12-18 21:57 ` Junio C Hamano
2019-12-19 6:25 ` Junio C Hamano
2019-12-19 17:19 ` Alexandr Miloslavskiy
2019-12-16 15:47 ` [PATCH v2 02/18] t7526: add tests for error conditions Alexandr Miloslavskiy via GitGitGadget
2019-12-18 22:02 ` Junio C Hamano
2019-12-19 18:03 ` Alexandr Miloslavskiy
2019-12-16 15:47 ` [PATCH v2 03/18] t7107: " Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:47 ` [PATCH v2 04/18] commit: forbid --pathspec-from-file --all Alexandr Miloslavskiy via GitGitGadget
2019-12-17 20:00 ` Phillip Wood
2019-12-18 22:04 ` Junio C Hamano
2019-12-18 22:06 ` Junio C Hamano
2019-12-18 22:16 ` Junio C Hamano
2019-12-19 17:38 ` Alexandr Miloslavskiy
2019-12-16 15:47 ` [PATCH v2 05/18] cmd_add: prepare for next patch Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:47 ` [PATCH v2 06/18] add: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:47 ` [PATCH v2 07/18] doc: checkout: remove duplicate synopsis Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:47 ` [PATCH v2 08/18] doc: checkout: fix broken text reference Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:47 ` [PATCH v2 09/18] doc: checkout: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:47 ` [PATCH v2 10/18] doc: restore: " Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:47 ` [PATCH v2 11/18] parse_branchname_arg(): extract part as new function Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:48 ` [PATCH v2 12/18] checkout: die() on ambiguous tracking branches Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:48 ` [PATCH v2 13/18] parse_branchname_arg(): easier to understand variables Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:48 ` [PATCH v2 14/18] parse_branchname_arg(): introduce expect_commit_only Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:48 ` [PATCH v2 15/18] parse_branchname_arg(): update code comments Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:48 ` [PATCH v2 16/18] parse_branchname_arg(): refactor the decision making Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:48 ` [PATCH v2 17/18] t2024: cover more cases Alexandr Miloslavskiy via GitGitGadget
2019-12-16 15:48 ` [PATCH v2 18/18] checkout, restore: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 00/18] Extend --pathspec-from-file to git add, checkout Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 01/18] t7107, t7526: directly test parse_pathspec_file() Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 02/18] t7526: add tests for error conditions Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 03/18] t7107: " Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 04/18] commit: forbid --pathspec-from-file --all Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 05/18] cmd_add: prepare for next patch Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 06/18] add: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 07/18] doc: checkout: remove duplicate synopsis Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 08/18] doc: checkout: fix broken text reference Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 09/18] doc: checkout: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 10/18] doc: restore: " Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 11/18] parse_branchname_arg(): extract part as new function Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 12/18] checkout: die() on ambiguous tracking branches Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 13/18] parse_branchname_arg(): easier to understand variables Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 14/18] parse_branchname_arg(): simplify argument eating Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 15/18] parse_branchname_arg(): update code comments Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 16/18] parse_branchname_arg(): refactor the decision making Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 17/18] t2024: cover more cases Alexandr Miloslavskiy via GitGitGadget
2019-12-19 18:01 ` [PATCH v3 18/18] checkout, restore: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=271e8ed3ab1a67dbd190ebeb6fe8c675b79b47e3.1576161385.git.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=alexandr.miloslavskiy@syntevo.com \
--cc=avarab@gmail.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).