patchcheckout: disambiguate dwim tracking branches and local files

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

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

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?

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

On Mon, Nov 12, 2018 at 7:44 AM Junio C Hamano <gitster@pobox.com> wrote:

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.

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.

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 Nguyen <pclouds@gmail.com> writes:

: 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?

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

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
    + '

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