From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Alexandr Miloslavskiy via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Subject: Re: [PATCH 2/2] checkout: die() on ambiguous tracking branches
Date: Wed, 27 Nov 2019 16:43:31 +0100 [thread overview]
Message-ID: <87v9r5nx3w.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <7dde1a3b4e4e76cd1a820b5277f694fdfad3a922.1574848137.git.gitgitgadget@gmail.com>
On Wed, Nov 27 2019, Alexandr Miloslavskiy via GitGitGadget wrote:
> 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/
I'll reserve judgement on whether we really should do this for now, my
current opinion on the matter is undefined as I haven't re-paged this
behavior of checkout into my brain.
But a giant red flag here for me is that you say "I understand that this
was never intended".
Just from a cursory look at this that's not true, for better or worse it
*is* intended behavior. Most of the code you're moving around here is
what I added in ad8d5104b4 ("checkout: add advice for ambiguous
"checkout <branch>"", 2018-06-05), and the very start of that commit
message refers to the checkout documentation we have that explicitly
documents this edge case.
Digging a bit further reveals that we've had this behavior (again,
intended, not emergent) since 70c9ac2f19 ("DWIM "git checkout frotz" to
"git checkout -b frotz origin/frotz"", 2009-10-18), and had it
documented since 00bb4378c7 ("Documentation/git-checkout.txt: document
70c9ac2 behavior", 2012-12-17).
So at the very least I'd say you need a v2 where you amend the relevant
docs & commit message to make a case to the effect of "we've had this
since 2009, but it was never really all that important etc.".
Such a change should also be changing the docs etc. added in 8d7b558bae
("checkout & worktree: introduce checkout.defaultRemote",
2018-06-05). With this series our docs don't make a lot of sense anymore
& don't describe the behavior with the patches applied.
> Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> ---
> builtin/checkout.c | 56 ++++++++++++++++++----------------------
> t/t2024-checkout-dwim.sh | 23 +++++++++++++++--
> 2 files changed, 46 insertions(+), 33 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index e1b9df1543..6fb427990f 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..707c88ceba 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,21 @@ 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' '
> + 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" &&
> +
> + 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 &&
> +
> + 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 &&
next prev parent reply other threads:[~2019-11-27 15:43 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-27 9:48 [PATCH 0/2] checkout: die() on ambiguous tracking branches Alexandr Miloslavskiy via GitGitGadget
2019-11-27 9:48 ` [PATCH 1/2] parse_branchname_arg(): extract part as new function Alexandr Miloslavskiy via GitGitGadget
2019-11-27 9:48 ` [PATCH 2/2] checkout: die() on ambiguous tracking branches Alexandr Miloslavskiy via GitGitGadget
2019-11-27 14:46 ` Derrick Stolee
2019-11-27 16:42 ` Alexandr Miloslavskiy
2019-11-27 15:43 ` Ævar Arnfjörð Bjarmason [this message]
2019-11-27 16:09 ` Alexandr Miloslavskiy
2019-12-05 15:34 ` Alexandr Miloslavskiy
2019-11-27 16:43 ` [PATCH v2 0/2] " Alexandr Miloslavskiy via GitGitGadget
2019-11-27 16:43 ` [PATCH v2 1/2] parse_branchname_arg(): extract part as new function Alexandr Miloslavskiy via GitGitGadget
2019-11-27 16:43 ` [PATCH v2 2/2] checkout: die() on ambiguous tracking branches Alexandr Miloslavskiy via GitGitGadget
2019-12-01 15:47 ` [PATCH v2 0/2] " Junio C Hamano
2019-12-01 16:52 ` Alexandr Miloslavskiy
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87v9r5nx3w.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=alexandr.miloslavskiy@syntevo.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).