git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
To: git@vger.kernel.org
Cc: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Subject: [PATCH] checkout: disambiguate dwim tracking branches and local files
Date: Sat, 10 Nov 2018 13:07:07 +0100	[thread overview]
Message-ID: <20181110120707.25846-1-pclouds@gmail.com> (raw)

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


             reply	other threads:[~2018-11-10 12:07 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-10 12:07 Nguyễn Thái Ngọc Duy [this message]
2018-11-12  6:44 ` [PATCH] checkout: disambiguate dwim tracking branches and local files 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

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=20181110120707.25846-1-pclouds@gmail.com \
    --to=pclouds@gmail.com \
    --cc=git@vger.kernel.org \
    /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).