Skip to content

checkout: die() on ambiguous tracking branches #477

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

checkout: die() on ambiguous tracking branches #477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e40cc4
parse_branchname_arg(): extract part as new function
SyntevoAlex Nov 26, 2019
575eeb9
checkout: die() on ambiguous tracking branches
SyntevoAlex Nov 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 39 additions & 32 deletions builtin/checkout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1113,12 +1113,43 @@ static void setup_new_branch_info_and_source_tree(
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


On Wed, Nov 27 2019, Alexandr Miloslavskiy via GitGitGadget wrote:

> From: Alexandr Miloslavskiy <[email protected]>
>
> 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/[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]/

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 <[email protected]>
> ---
>  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 &&

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):



On 27.11.2019 16:43, Ævar Arnfjörð Bjarmason wrote:
> 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.".
> 

I'm sorry, can you please clarify?

My patch addresses the situation where there are *multiple* remote 
candidates *and* a file with the same name.

My feeling is that in this case, reverting a file is an unintended surprise.

I understand previous patches this way:
ad8d5104 - patch series is mostly for "if there are *multiple* remotes,
            disambiguate via checkout.defaultRemote". This essentially
            converts the case of multiple remotes into a single remote.
            However, this also semi-documents what I'm now preventing.
            Was that really intended?
70c9ac2f - if there is *one* remote, DWIM it.
            This isn't what I'm changing.
be4908f1 - if there is *one* remote *and* file, die().
            This is what I'm extending further.

If the prevented behavior is documented, could you please quote it 
explicitly?

> 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.

To my understanding, my patch doesn't affect those pieces of docs? 
Please correct me if I'm wrong.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):

Ævar, please let me know if your concern is resolved?

On 27.11.2019 17:09, Alexandr Miloslavskiy wrote:
> 
> 
> On 27.11.2019 16:43, Ævar Arnfjörð Bjarmason wrote:
>> 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.".
>>
> 
> I'm sorry, can you please clarify?
> 
> My patch addresses the situation where there are *multiple* remote 
> candidates *and* a file with the same name.
> 
> My feeling is that in this case, reverting a file is an unintended 
> surprise.
> 
> I understand previous patches this way:
> ad8d5104 - patch series is mostly for "if there are *multiple* remotes,
>             disambiguate via checkout.defaultRemote". This essentially
>             converts the case of multiple remotes into a single remote.
>             However, this also semi-documents what I'm now preventing.
>             Was that really intended?
> 70c9ac2f - if there is *one* remote, DWIM it.
>             This isn't what I'm changing.
> be4908f1 - if there is *one* remote *and* file, die().
>             This is what I'm extending further.
> 
> If the prevented behavior is documented, could you please quote it 
> explicitly?
> 
>> 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.
> 
> To my understanding, my patch doesn't affect those pieces of docs? 
> Please correct me if I'm wrong.

}

static const char *parse_remote_branch(const char *arg,
struct object_id *rev,
int could_be_checkout_paths)
{
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"
"Please use -- (and optionally --no-guess) to disambiguate"),
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;
}

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;
Expand Down Expand Up @@ -1223,13 +1254,9 @@ 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);
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) */
Expand Down Expand Up @@ -1492,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));
Expand Down Expand Up @@ -1600,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) {
Expand Down Expand Up @@ -1659,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)
Expand Down
28 changes: 26 additions & 2 deletions t/t2024-checkout-dwim.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
(
Expand All @@ -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 &&
Expand Down Expand Up @@ -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 &&
Expand Down