[PATCH] checkout: disambiguate dwim tracking branches and local files

Subsystems: the rest

STALE2874d

3 messages, 1 author, 2018-11-13 · open the first message on its own page

[PATCH] checkout: disambiguate dwim tracking branches and local files

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2018-11-10 12:07:16

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

[PATCH v2 0/1] disambiguate dwim tracking branches and local files

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2018-11-13 17:52:36

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

[PATCH v2 1/1] checkout: disambiguate dwim tracking branches and local files

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2018-11-13 17:52:37

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 <redacted>
---
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help