git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] checkout: disambiguate dwim tracking branches and local files
@ 2018-11-10 12:07 Nguyễn Thái Ngọc Duy
  2018-11-12  6:44 ` Junio C Hamano
  2018-11-13 17:52 ` [PATCH v2 0/1] " Nguyễn Thái Ngọc Duy
  0 siblings, 2 replies; 6+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-11-10 12:07 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy

When checkout dwim is added in [1], it is restricted to only dwim when
certain conditions are met and fall back to default checkout behavior
otherwise. It turns out falling back could be confusing. One of the
conditions to turn

    git checkout frotz

to

    git checkout -b frotz origin/frotz

is that frotz must not exist as a file. But when the user comes to
expect "git checkout frotz" to create the branch "frotz" and there
happens to be a file named "frotz", git's silently reverting "frotz"
file content is not helping. This is reported in Git mailing list [2]
and even used as an example of "Git is bad" elsewhere [3].

We normally try to do the right thing, but when there are multiple
"right things" to do, it's best to leave it to the user to decide.
Check this case, ask the user to use "--" to disambiguate.

[1] 70c9ac2f19 (DWIM "git checkout frotz" to "git checkout -b frotz
    origin/frotz" - 2009-10-18)
[2] https://public-inbox.org/git/CACsJy8B2TVr1g+k+eSQ=pBEO3WN4_LtgLo9gpur8X7Z9GOFL_A@mail.gmail.com/
[3] https://news.ycombinator.com/item?id=18230655

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c       | 15 ++++++++++++---
 t/t2024-checkout-dwim.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index acdafc6e4c..17d48166d1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1079,9 +1079,15 @@ static int parse_branchname_arg(int argc, const char **argv,
 		 */
 		int recover_with_dwim = dwim_new_local_branch_ok;
 
-		if (!has_dash_dash &&
-		    (check_filename(opts->prefix, arg) || !no_wildcard(arg)))
-			recover_with_dwim = 0;
+		/*
+		 * If both refs/remotes/origin/master and the file
+		 * 'master'. Don't blindly go for 'master' file
+		 * because it's ambiguous. Leave it for the user to
+		 * decide.
+		 */
+		int disambi_local_file = !has_dash_dash &&
+			(check_filename(opts->prefix, arg) || !no_wildcard(arg));
+
 		/*
 		 * Accept "git checkout foo" and "git checkout foo --"
 		 * as candidates for dwim.
@@ -1094,6 +1100,9 @@ static int parse_branchname_arg(int argc, const char **argv,
 			const char *remote = unique_tracking_name(arg, rev,
 								  dwim_remotes_matched);
 			if (remote) {
+				if (disambi_local_file)
+					die(_("'%s' could be both a local file and a tracking branch.\n"
+					      "Please use -- to disambiguate"), arg);
 				*new_branch = arg;
 				arg = remote;
 				/* DWIMmed to create local branch, case (3).(b) */
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 69b6774d10..fa0718c730 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -278,4 +278,35 @@ test_expect_success 'loosely defined local base branch is reported correctly' '
 	test_cmp expect actual
 '
 
+test_expect_success 'reject when arg could be part of dwim branch' '
+	git remote add foo file://non-existent-place &&
+	git update-ref refs/remotes/foo/dwim-arg HEAD &&
+	echo foo >dwim-arg &&
+	git add dwim-arg &&
+	echo bar >dwim-arg &&
+	test_must_fail git checkout dwim-arg &&
+	test_must_fail git rev-parse refs/heads/dwim-arg -- &&
+	grep bar dwim-arg
+'
+
+test_expect_success 'disambiguate dwim branch and checkout path (1)' '
+	git update-ref refs/remotes/foo/dwim-arg1 HEAD &&
+	echo foo >dwim-arg1 &&
+	git add dwim-arg1 &&
+	echo bar >dwim-arg1 &&
+	git checkout -- dwim-arg1 &&
+	test_must_fail git rev-parse refs/heads/dwim-arg1 -- &&
+	grep foo dwim-arg1
+'
+
+test_expect_success 'disambiguate dwim branch and checkout path (2)' '
+	git update-ref refs/remotes/foo/dwim-arg2 HEAD &&
+	echo foo >dwim-arg2 &&
+	git add dwim-arg2 &&
+	echo bar >dwim-arg2 &&
+	git checkout dwim-arg2 -- &&
+	git rev-parse refs/heads/dwim-arg2 -- &&
+	grep bar dwim-arg2
+'
+
 test_done
-- 
2.19.1.1231.g84aef82467


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] checkout: disambiguate dwim tracking branches and local files
  2018-11-10 12:07 [PATCH] checkout: disambiguate dwim tracking branches and local files Nguyễn Thái Ngọc Duy
@ 2018-11-12  6:44 ` Junio C Hamano
  2018-11-12 17:26   ` Duy Nguyen
  2018-11-13 17:52 ` [PATCH v2 0/1] " Nguyễn Thái Ngọc Duy
  1 sibling, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2018-11-12  6:44 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> @@ -1079,9 +1079,15 @@ static int parse_branchname_arg(int argc, const char **argv,
>  		 */
>  		int recover_with_dwim = dwim_new_local_branch_ok;
>  
> -		if (!has_dash_dash &&
> -		    (check_filename(opts->prefix, arg) || !no_wildcard(arg)))
> -			recover_with_dwim = 0;
> +		/*
> +		 * If both refs/remotes/origin/master and the file
> +		 * 'master'. Don't blindly go for 'master' file
> +		 * because it's ambiguous. Leave it for the user to
> +		 * decide.
> +		 */
> +		int disambi_local_file = !has_dash_dash &&
> +			(check_filename(opts->prefix, arg) || !no_wildcard(arg));

What you are computing on the right hand side is if the arg is
ambiguous.  And the code that looks at this variable does not
disambiguate, but it just punts and makes it responsibility to the
user (which is by the way the correct thing to do).

When a file with exact name is in the working tree, we do not
declare it is a pathspec and not a rev, as we may be allowed to dwim
and create a rev with that name and at that point we'd be in an
ambigous situation.  If the arg _has_ wildcard, however, it is a
strong sign that it *is* a pathspec, isn't it?  It is iffy that a
single variable that cannot be used to distinguish these two cases
is introduced---one of these cases will be mishandled.

Also how does the patch ensures that this new logic does not kick in
for those who refuse to let the command dwim to create a new branch?

>  		/*
>  		 * Accept "git checkout foo" and "git checkout foo --"
>  		 * as candidates for dwim.
> @@ -1094,6 +1100,9 @@ static int parse_branchname_arg(int argc, const char **argv,
>  			const char *remote = unique_tracking_name(arg, rev,
>  								  dwim_remotes_matched);
>  			if (remote) {
> +				if (disambi_local_file)
> +					die(_("'%s' could be both a local file and a tracking branch.\n"
> +					      "Please use -- to disambiguate"), arg);

Ah, the only user of this variable triggers when recover_with_dwim
is true, so that is how dwim-less case is handled, OK.

That still leaves the question if it is OK to handle these two cases
the same way in a repository without 'next' branch whose origin has
one:

    $ >next; git checkout --guess next
    $ >next; git checkout --guess 'n??t'

Perhaps the variable should be named "local_file_crashes_with_rev"
and its the scope narrowed to the same block as "remote" is
computed.  Or just remove the variable and check the condition right
there where you need to.  I.e.

	if (remote) {
		if (!has_dash_dash &&
		    check_filename(opts->prefix, arg))
			die("did you mean a branch or path?: '%s'", arg);
		...


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] checkout: disambiguate dwim tracking branches and local files
  2018-11-12  6:44 ` Junio C Hamano
@ 2018-11-12 17:26   ` Duy Nguyen
  2018-11-12 19:44     ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Duy Nguyen @ 2018-11-12 17:26 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

On Mon, Nov 12, 2018 at 7:44 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
> > @@ -1079,9 +1079,15 @@ static int parse_branchname_arg(int argc, const char **argv,
> >                */
> >               int recover_with_dwim = dwim_new_local_branch_ok;
> >
> > -             if (!has_dash_dash &&
> > -                 (check_filename(opts->prefix, arg) || !no_wildcard(arg)))
> > -                     recover_with_dwim = 0;
> > +             /*
> > +              * If both refs/remotes/origin/master and the file
> > +              * 'master'. Don't blindly go for 'master' file
> > +              * because it's ambiguous. Leave it for the user to
> > +              * decide.
> > +              */
> > +             int disambi_local_file = !has_dash_dash &&
> > +                     (check_filename(opts->prefix, arg) || !no_wildcard(arg));
>
> What you are computing on the right hand side is if the arg is
> ambiguous.  And the code that looks at this variable does not
> disambiguate, but it just punts and makes it responsibility to the
> user (which is by the way the correct thing to do).
>
> When a file with exact name is in the working tree, we do not
> declare it is a pathspec and not a rev, as we may be allowed to dwim
> and create a rev with that name and at that point we'd be in an
> ambigous situation.  If the arg _has_ wildcard, however, it is a
> strong sign that it *is* a pathspec, isn't it?  It is iffy that a
> single variable that cannot be used to distinguish these two cases
> is introduced---one of these cases will be mishandled.

Is it that different between an exact path name and a pathspec?
Suppose it is a pathspec (with wildcards) that matches some paths, and
we happen to have the remote branch origin/<that-pathspec>, then it is
still ambiguous whether we should go create branch
"<that-pathspec>" or go check out the paths matched by the pathspec.

> Also how does the patch ensures that this new logic does not kick in
> for those who refuse to let the command dwim to create a new branch?

I would hope that it would be "--" to settle all disputes. But it
looks like "git checkout foo --" is explicitly a candidate for dwim.
We have a hidden option --no-guess to disable dwim. Maybe it's a good
idea to make that one visible. It's at least clearer than using "--"
even if "--" worked on this case.

>
> >               /*
> >                * Accept "git checkout foo" and "git checkout foo --"
> >                * as candidates for dwim.
> > @@ -1094,6 +1100,9 @@ static int parse_branchname_arg(int argc, const char **argv,
> >                       const char *remote = unique_tracking_name(arg, rev,
> >                                                                 dwim_remotes_matched);
> >                       if (remote) {
> > +                             if (disambi_local_file)
> > +                                     die(_("'%s' could be both a local file and a tracking branch.\n"
> > +                                           "Please use -- to disambiguate"), arg);
>
> Ah, the only user of this variable triggers when recover_with_dwim
> is true, so that is how dwim-less case is handled, OK.
>
> That still leaves the question if it is OK to handle these two cases
> the same way in a repository without 'next' branch whose origin has
> one:
>
>     $ >next; git checkout --guess next
>     $ >next; git checkout --guess 'n??t'
>
> Perhaps the variable should be named "local_file_crashes_with_rev"
> and its the scope narrowed to the same block as "remote" is
> computed.  Or just remove the variable and check the condition right
> there where you need to.  I.e.
>
>         if (remote) {
>                 if (!has_dash_dash &&
>                     check_filename(opts->prefix, arg))
>                         die("did you mean a branch or path?: '%s'", arg);
>                 ...
>

I still think handing both cases the same way is right. In the second
case we would not find 'origin/n??t' so we fall back to checking out
pathspec 'n??t' anyway (expected from you, I think). But just in case
the remote branch 'origin/n??t' exists, we kick and scream.

I think you see 'n??t' as a pathspec, but I'm thinking about a user
who sees 'n??t' as a branch name, not pathspec and he would have a
different expectation.
-- 
Duy

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] checkout: disambiguate dwim tracking branches and local files
  2018-11-12 17:26   ` Duy Nguyen
@ 2018-11-12 19:44     ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2018-11-12 19:44 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List

Duy Nguyen <pclouds@gmail.com> writes:

> Is it that different between an exact path name and a pathspec?
> Suppose it is a pathspec (with wildcards) that matches some paths, and
> we happen to have the remote branch origin/<that-pathspec>, then it is
> still ambiguous whether we should go create branch
> "<that-pathspec>" or go check out the paths matched by the pathspec.
:
A huge difference between these two

	$ echo Hello >'n??t' && git add 'n??t'
	$ git branch 'n??t'

is that the former is taken and the latter is rejected, even though
neither of them is particularly a sane or a likely thing.

Isn't that a good enough reason why

	$ git checkout 'n??t'

cannot mean checking 'n??t' branch out, be it either an existing
local one or auto-vivified one out of a remote-tracking branch?

> I think you see 'n??t' as a pathspec, but I'm thinking about a user
> who sees 'n??t' as a branch name, not pathspec and he would have a
> different expectation.

See above for the reason why I think there is no room for different
expectations to come in the picture in this case.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 0/1] disambiguate dwim tracking branches and local files
  2018-11-10 12:07 [PATCH] checkout: disambiguate dwim tracking branches and local files Nguyễn Thái Ngọc Duy
  2018-11-12  6:44 ` Junio C Hamano
@ 2018-11-13 17:52 ` Nguyễn Thái Ngọc Duy
  2018-11-13 17:52   ` [PATCH v2 1/1] checkout: " Nguyễn Thái Ngọc Duy
  1 sibling, 1 reply; 6+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-11-13 17:52 UTC (permalink / raw)
  To: pclouds; +Cc: git, Junio C Hamano

v2 leaves "pathspec with wildcard" case alone. The behavior in this
case remains as before.

--no-guess is now made visible in "git checkout -h" and the man page.


PS. Based on git-checkout.txt I don't think any user can work out that
"git checkout branch --" can be used to disambiguate. And updating the
doc to show that makes it a lot uglier.

Perhaps it's time we add two new semi-aliases, switch-branch and
restore-path. They will have the same syntax as checkout in their
respective use case, without ambiguation. Semi-aliases because like
builtin commands, they cannot be overriden by user aliases.

Nguyễn Thái Ngọc Duy (1):
  checkout: disambiguate dwim tracking branches and local files

 Documentation/git-checkout.txt |  4 ++++
 builtin/checkout.c             | 18 +++++++++++++-----
 t/t2024-checkout-dwim.sh       | 31 +++++++++++++++++++++++++++++++
 t/t9902-completion.sh          |  3 ++-
 4 files changed, 50 insertions(+), 6 deletions(-)

Range-diff against v1:
1:  530f5d8f03 ! 1:  0408fdde4d checkout: disambiguate dwim tracking branches and local files
    @@ -21,12 +21,36 @@
     
         We normally try to do the right thing, but when there are multiple
         "right things" to do, it's best to leave it to the user to decide.
    -    Check this case, ask the user to use "--" to disambiguate.
    +    Check this case, ask the user to to disambiguate:
    +
    +    - "git checkout -- foo" will check out path "foo"
    +    - "git checkout foo --" will dwim and create branch "foo" [4]
    +
    +    For users who do not want dwim, use --no-guess. It's useless in this
    +    particular case because "git checkout --no-guess foo --" will just
    +    fail. But it could be used by scripts.
     
         [1] 70c9ac2f19 (DWIM "git checkout frotz" to "git checkout -b frotz
             origin/frotz" - 2009-10-18)
         [2] https://public-inbox.org/git/CACsJy8B2TVr1g+k+eSQ=pBEO3WN4_LtgLo9gpur8X7Z9GOFL_A@mail.gmail.com/
         [3] https://news.ycombinator.com/item?id=18230655
    +    [4] a047fafc78 (checkout: allow dwim for branch creation for "git
    +        checkout $branch --" - 2013-10-18)
    +
    + diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
    + --- a/Documentation/git-checkout.txt
    + +++ b/Documentation/git-checkout.txt
    +@@
    + 	Just like linkgit:git-submodule[1], this will detach the
    + 	submodules HEAD.
    + 
    ++--no-guess::
    ++	Do not attempt to create a branch if a remote tracking branch
    ++	of the same name exists.
    ++
    + <branch>::
    + 	Branch to checkout; if it refers to a branch (i.e., a name that,
    + 	when prepended with "refs/heads/", is a valid ref), then that
     
      diff --git a/builtin/checkout.c b/builtin/checkout.c
      --- a/builtin/checkout.c
    @@ -37,9 +61,11 @@
      
     -		if (!has_dash_dash &&
     -		    (check_filename(opts->prefix, arg) || !no_wildcard(arg)))
    --			recover_with_dwim = 0;
     +		int could_be_checkout_paths = !has_dash_dash &&
    -+			(check_filename(opts->prefix, arg) || !no_wildcard(arg));
    ++			check_filename(opts->prefix, arg);
    ++
    ++		if (!has_dash_dash && !no_wildcard(arg))
    + 			recover_with_dwim = 0;
     +
      		/*
      		 * Accept "git checkout foo" and "git checkout foo --"
    @@ -50,10 +76,39 @@
      			if (remote) {
     +				if (could_be_checkout_paths)
     +					die(_("'%s' could be both a local file and a tracking branch.\n"
    -+					      "Please use -- to disambiguate"), arg);
    ++					      "Please use -- (and optionally --no-guess) to disambiguate"),
    ++					    arg);
      				*new_branch = arg;
      				arg = remote;
      				/* DWIMmed to create local branch, case (3).(b) */
    +@@
    + 	struct checkout_opts opts;
    + 	struct branch_info new_branch_info;
    + 	char *conflict_style = NULL;
    +-	int dwim_new_local_branch = 1;
    ++	int dwim_new_local_branch, no_dwim_new_local_branch = 0;
    + 	int dwim_remotes_matched = 0;
    + 	struct option options[] = {
    + 		OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
    +@@
    + 		OPT_BOOL('p', "patch", &opts.patch_mode, N_("select hunks interactively")),
    + 		OPT_BOOL(0, "ignore-skip-worktree-bits", &opts.ignore_skipworktree,
    + 			 N_("do not limit pathspecs to sparse entries only")),
    +-		OPT_HIDDEN_BOOL(0, "guess", &dwim_new_local_branch,
    +-				N_("second guess 'git checkout <no-such-branch>'")),
    ++		OPT_BOOL(0, "no-guess", &no_dwim_new_local_branch,
    ++			 N_("do not second guess 'git checkout <no-such-branch>'")),
    + 		OPT_BOOL(0, "ignore-other-worktrees", &opts.ignore_other_worktrees,
    + 			 N_("do not check if another worktree is holding the given ref")),
    + 		{ OPTION_CALLBACK, 0, "recurse-submodules", NULL,
    +@@
    + 	argc = parse_options(argc, argv, prefix, options, checkout_usage,
    + 			     PARSE_OPT_KEEP_DASHDASH);
    + 
    ++	dwim_new_local_branch = !no_dwim_new_local_branch;
    + 	if (opts.show_progress < 0) {
    + 		if (opts.quiet)
    + 			opts.show_progress = 0;
     
      diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
      --- a/t/t2024-checkout-dwim.sh
    @@ -94,3 +149,17 @@
     +'
     +
      test_done
    +
    + diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
    + --- a/t/t9902-completion.sh
    + +++ b/t/t9902-completion.sh
    +@@
    + 	--ignore-other-worktrees Z
    + 	--recurse-submodules Z
    + 	--progress Z
    +-	--no-quiet Z
    ++	--guess Z
    ++	--no-guess Z
    + 	--no-... Z
    + 	EOF
    + '
-- 
2.19.1.1318.g5295c6727d


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/1] checkout: disambiguate dwim tracking branches and local files
  2018-11-13 17:52 ` [PATCH v2 0/1] " Nguyễn Thái Ngọc Duy
@ 2018-11-13 17:52   ` Nguyễn Thái Ngọc Duy
  0 siblings, 0 replies; 6+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-11-13 17:52 UTC (permalink / raw)
  To: pclouds; +Cc: git, Junio C Hamano

When checkout dwim is added in [1], it is restricted to only dwim when
certain conditions are met and fall back to default checkout behavior
otherwise. It turns out falling back could be confusing. One of the
conditions to turn

    git checkout frotz

to

    git checkout -b frotz origin/frotz

is that frotz must not exist as a file. But when the user comes to
expect "git checkout frotz" to create the branch "frotz" and there
happens to be a file named "frotz", git's silently reverting "frotz"
file content is not helping. This is reported in Git mailing list [2]
and even used as an example of "Git is bad" elsewhere [3].

We normally try to do the right thing, but when there are multiple
"right things" to do, it's best to leave it to the user to decide.
Check this case, ask the user to to disambiguate:

- "git checkout -- foo" will check out path "foo"
- "git checkout foo --" will dwim and create branch "foo" [4]

For users who do not want dwim, use --no-guess. It's useless in this
particular case because "git checkout --no-guess foo --" will just
fail. But it could be used by scripts.

[1] 70c9ac2f19 (DWIM "git checkout frotz" to "git checkout -b frotz
    origin/frotz" - 2009-10-18)
[2] https://public-inbox.org/git/CACsJy8B2TVr1g+k+eSQ=pBEO3WN4_LtgLo9gpur8X7Z9GOFL_A@mail.gmail.com/
[3] https://news.ycombinator.com/item?id=18230655
[4] a047fafc78 (checkout: allow dwim for branch creation for "git
    checkout $branch --" - 2013-10-18)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-checkout.txt |  4 ++++
 builtin/checkout.c             | 18 +++++++++++++-----
 t/t2024-checkout-dwim.sh       | 31 +++++++++++++++++++++++++++++++
 t/t9902-completion.sh          |  3 ++-
 4 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 801de2f764..6acc3d98e7 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -276,6 +276,10 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
 	Just like linkgit:git-submodule[1], this will detach the
 	submodules HEAD.
 
+--no-guess::
+	Do not attempt to create a branch if a remote tracking branch
+	of the same name exists.
+
 <branch>::
 	Branch to checkout; if it refers to a branch (i.e., a name that,
 	when prepended with "refs/heads/", is a valid ref), then that
diff --git a/builtin/checkout.c b/builtin/checkout.c
index acdafc6e4c..4744e8c0a7 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1079,9 +1079,12 @@ static int parse_branchname_arg(int argc, const char **argv,
 		 */
 		int recover_with_dwim = dwim_new_local_branch_ok;
 
-		if (!has_dash_dash &&
-		    (check_filename(opts->prefix, arg) || !no_wildcard(arg)))
+		int could_be_checkout_paths = !has_dash_dash &&
+			check_filename(opts->prefix, arg);
+
+		if (!has_dash_dash && !no_wildcard(arg))
 			recover_with_dwim = 0;
+
 		/*
 		 * Accept "git checkout foo" and "git checkout foo --"
 		 * as candidates for dwim.
@@ -1094,6 +1097,10 @@ static int parse_branchname_arg(int argc, const char **argv,
 			const char *remote = unique_tracking_name(arg, rev,
 								  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) */
@@ -1228,7 +1235,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	struct checkout_opts opts;
 	struct branch_info new_branch_info;
 	char *conflict_style = NULL;
-	int dwim_new_local_branch = 1;
+	int dwim_new_local_branch, no_dwim_new_local_branch = 0;
 	int dwim_remotes_matched = 0;
 	struct option options[] = {
 		OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
@@ -1258,8 +1265,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('p', "patch", &opts.patch_mode, N_("select hunks interactively")),
 		OPT_BOOL(0, "ignore-skip-worktree-bits", &opts.ignore_skipworktree,
 			 N_("do not limit pathspecs to sparse entries only")),
-		OPT_HIDDEN_BOOL(0, "guess", &dwim_new_local_branch,
-				N_("second guess 'git checkout <no-such-branch>'")),
+		OPT_BOOL(0, "no-guess", &no_dwim_new_local_branch,
+			 N_("do not second guess 'git checkout <no-such-branch>'")),
 		OPT_BOOL(0, "ignore-other-worktrees", &opts.ignore_other_worktrees,
 			 N_("do not check if another worktree is holding the given ref")),
 		{ OPTION_CALLBACK, 0, "recurse-submodules", NULL,
@@ -1282,6 +1289,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, checkout_usage,
 			     PARSE_OPT_KEEP_DASHDASH);
 
+	dwim_new_local_branch = !no_dwim_new_local_branch;
 	if (opts.show_progress < 0) {
 		if (opts.quiet)
 			opts.show_progress = 0;
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 69b6774d10..fa0718c730 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -278,4 +278,35 @@ test_expect_success 'loosely defined local base branch is reported correctly' '
 	test_cmp expect actual
 '
 
+test_expect_success 'reject when arg could be part of dwim branch' '
+	git remote add foo file://non-existent-place &&
+	git update-ref refs/remotes/foo/dwim-arg HEAD &&
+	echo foo >dwim-arg &&
+	git add dwim-arg &&
+	echo bar >dwim-arg &&
+	test_must_fail git checkout dwim-arg &&
+	test_must_fail git rev-parse refs/heads/dwim-arg -- &&
+	grep bar dwim-arg
+'
+
+test_expect_success 'disambiguate dwim branch and checkout path (1)' '
+	git update-ref refs/remotes/foo/dwim-arg1 HEAD &&
+	echo foo >dwim-arg1 &&
+	git add dwim-arg1 &&
+	echo bar >dwim-arg1 &&
+	git checkout -- dwim-arg1 &&
+	test_must_fail git rev-parse refs/heads/dwim-arg1 -- &&
+	grep foo dwim-arg1
+'
+
+test_expect_success 'disambiguate dwim branch and checkout path (2)' '
+	git update-ref refs/remotes/foo/dwim-arg2 HEAD &&
+	echo foo >dwim-arg2 &&
+	git add dwim-arg2 &&
+	echo bar >dwim-arg2 &&
+	git checkout dwim-arg2 -- &&
+	git rev-parse refs/heads/dwim-arg2 -- &&
+	grep bar dwim-arg2
+'
+
 test_done
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 175f83d704..aa92f85230 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1434,7 +1434,8 @@ test_expect_success 'double dash "git checkout"' '
 	--ignore-other-worktrees Z
 	--recurse-submodules Z
 	--progress Z
-	--no-quiet Z
+	--guess Z
+	--no-guess Z
 	--no-... Z
 	EOF
 '
-- 
2.19.1.1318.g5295c6727d


^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-11-13 17:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-10 12:07 [PATCH] checkout: disambiguate dwim tracking branches and local files Nguyễn Thái Ngọc Duy
2018-11-12  6:44 ` Junio C Hamano
2018-11-12 17:26   ` Duy Nguyen
2018-11-12 19:44     ` Junio C Hamano
2018-11-13 17:52 ` [PATCH v2 0/1] " Nguyễn Thái Ngọc Duy
2018-11-13 17:52   ` [PATCH v2 1/1] checkout: " Nguyễn Thái Ngọc Duy

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