[PATCH 1/3] checkout.c: add strict usage of -- before file_path

[PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Dannier Castro L]

Currently, <checkout> is a complex command able to handle both
branches and files without any distintion other than their names,
taking into account that depending on the type (branch or file),
the functionality is completely different, the easier example:

$ git checkout <branch>  # Switch from current branch to <branch>.
$ git checkout <file>    # Restore <file> from HEAD, discarding
                         # changes if it's necessary.
$ git checkout -- <file> # The same as the last one, only with an
                         # useless '--'.

For GIT new users, this complicated versatility of <checkout> could
be very confused, also considering that actually the flag '--' is
completely useless (added or not, there is not any difference for
this command), when the same program messages promote the use of
this flag.

Regarding the <checkout>'s power to overwrite any file, discarding
changes if they exist without some way of recovering them, the
solution propuses that the usage of '--' is strict before to
specify the file(s) path(s) for any <checkout> command (including
all types of flags), as a "defense barrier" to make sure about
user's knowledge and intension running <checkout>.

The solution consists in detect '--' into command args, allowing
the discard of changes and considering the following names as
file paths, otherwise, they are branch names.

Signed-off-by: Dannier Castro L <danniercl@gmail.com>
---
 builtin/checkout.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index d76e13c..ec577b3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -40,6 +40,7 @@ struct checkout_opts {
 	int ignore_skipworktree;
 	int ignore_other_worktrees;
 	int show_progress;
+	int discard_changes;
 
 	const char *new_branch;
 	const char *new_branch_force;
@@ -885,8 +886,8 @@ static int parse_branchname_arg(int argc, const char **argv,
 	/*
 	 * case 1: git checkout <ref> -- [<paths>]
 	 *
-	 *   <ref> must be a valid tree, everything after the '--' must be
-	 *   a path.
+	 *   <ref> must be a valid tree. '--' must always be before any path,
+	 *   it means, everything after the '--' must be a path.
 	 *
 	 * case 2: git checkout -- [<paths>]
 	 *
@@ -900,26 +901,28 @@ static int parse_branchname_arg(int argc, const char **argv,
 	 *       omit at most one side), and if there is a unique merge base
 	 *       between A and B, A...B names that merge base.
 	 *
-	 *   (b) If <something> is _not_ a commit, either "--" is present
-	 *       or <something> is not a path, no -t or -b was given, and
-	 *       and there is a tracking branch whose name is <something>
-	 *       in one and only one remote, then this is a short-hand to
-	 *       fork local <something> from that remote-tracking branch.
+	 *   (b) If <something> is _not_ a commit, either "--" is present,
+	 *       no -t or -b was given, and there is a tracking branch
+	 *       whose name is <something> in one and only one remote,
+	 *       then this is a short-hand to fork local <something> from
+	 *       that remote-tracking branch.
 	 *
 	 *   (c) Otherwise, if "--" is present, treat it like case (1).
 	 *
 	 *   (d) Otherwise :
 	 *       - if it's a reference, treat it like case (1)
-	 *       - else if it's a path, treat it like case (2)
 	 *       - else: fail.
 	 *
-	 * case 4: git checkout <something> <paths>
+	 *   <something> can never be a path (at least without '--' before).
+	 *
+	 * case 4: git checkout <something> -- <paths>
 	 *
 	 *   The first argument must not be ambiguous.
 	 *   - If it's *only* a reference, treat it like case (1).
-	 *   - If it's only a path, treat it like case (2).
 	 *   - else: fail.
 	 *
+	 *   <something> can never be a path (at least without '--' before).
+	 *
 	 */
 	if (!argc)
 		return 0;
@@ -928,6 +931,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 	dash_dash_pos = -1;
 	for (i = 0; i < argc; i++) {
 		if (!strcmp(argv[i], "--")) {
+			opts->discard_changes = 1;
 			dash_dash_pos = i;
 			break;
 		}
@@ -957,8 +961,8 @@ static int parse_branchname_arg(int argc, const char **argv,
 		    (check_filename(opts->prefix, arg) || !no_wildcard(arg)))
 			recover_with_dwim = 0;
 		/*
-		 * Accept "git checkout foo" and "git checkout foo --"
-		 * as candidates for dwim.
+		 * Accept "git checkout foo_branch" and
+		 * "git checkout foo_branch --" as candidates for dwim.
 		 */
 		if (!(argc == 1 && !has_dash_dash) &&
 		    !(argc == 2 && has_dash_dash))
@@ -1254,7 +1258,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	}
 
 	UNLEAK(opts);
-	if (opts.patch_mode || opts.pathspec.nr)
+	if (opts.patch_mode || (opts.pathspec.nr && opts.discard_changes))
 		return checkout_paths(&opts, new_branch_info.name);
 	else
 		return checkout_branch(&opts, &new_branch_info);
-- 
2.7.4

[PATCH 2/3] test: update tests for strict usage of -- checkout

[Dannier Castro L]

The flag '--' must always be before any file path when <checkout>
command is used.

The list of 34 test files updated is shown:

  t0021-conversion.sh
  t0027-auto-crlf.sh
  t1011-read-tree-sparse-checkout.sh
  t1050-large.sh
  t1051-large-conversion.sh
  t2009-checkout-statinfo.sh
  t2010-checkout-ambiguous.sh
  t2013-checkout-submodule.sh
  t2016-checkout-patch.sh
  t2022-checkout-paths.sh
  t2028-worktree-move.sh
  t2030-unresolve-info.sh
  t3001-ls-files-others-exclude.sh
  t3509-cherry-pick-merge-df.sh
  t3510-cherry-pick-sequence.sh
  t4015-diff-whitespace.sh
  t4124-apply-ws-rule.sh
  t5403-post-checkout-hook.sh
  t6007-rev-list-cherry-pick-file.sh
  t6026-merge-attr.sh
  t6030-bisect-porcelain.sh
  t6111-rev-list-treesame.sh
  t7001-mv.sh
  t7008-grep-binary.sh
  t7201-co.sh
  t7300-clean.sh
  t7501-commit.sh
  t7502-commit.sh
  t7607-merge-overwrite.sh
  t7810-grep.sh
  t7811-grep-open.sh
  t8006-blame-textconv.sh
  t9010-svn-fe.sh
  t8003-blame-corner-cases.sh

Signed-off-by: Dannier Castro L <danniercl@gmail.com>
---
 t/t0021-conversion.sh                | 12 ++++++------
 t/t0027-auto-crlf.sh                 |  4 ++--
 t/t1011-read-tree-sparse-checkout.sh |  4 ++--
 t/t1050-large.sh                     |  2 +-
 t/t1051-large-conversion.sh          |  4 ++--
 t/t2009-checkout-statinfo.sh         |  6 +++---
 t/t2010-checkout-ambiguous.sh        |  4 ++--
 t/t2013-checkout-submodule.sh        |  4 ++--
 t/t2016-checkout-patch.sh            |  9 +--------
 t/t2022-checkout-paths.sh            |  4 ++--
 t/t2028-worktree-move.sh             |  2 +-
 t/t2030-unresolve-info.sh            |  6 +++---
 t/t3001-ls-files-others-exclude.sh   |  2 +-
 t/t3420-rebase-autostash.sh          |  2 +-
 t/t3510-cherry-pick-sequence.sh      |  4 ++--
 t/t3910-mac-os-precompose.sh         |  4 ++--
 t/t4015-diff-whitespace.sh           |  4 ++--
 t/t4117-apply-reject.sh              |  2 +-
 t/t4124-apply-ws-rule.sh             | 16 ++++++++--------
 t/t5403-post-checkout-hook.sh        |  2 +-
 t/t6007-rev-list-cherry-pick-file.sh |  2 +-
 t/t6026-merge-attr.sh                |  2 +-
 t/t6030-bisect-porcelain.sh          |  2 +-
 t/t6038-merge-text-auto.sh           |  2 +-
 t/t6050-replace.sh                   |  2 +-
 t/t6111-rev-list-treesame.sh         |  2 +-
 t/t7001-mv.sh                        |  2 +-
 t/t7008-grep-binary.sh               |  2 +-
 t/t7201-co.sh                        |  8 ++++----
 t/t7300-clean.sh                     |  2 +-
 t/t7501-commit.sh                    | 10 +++++-----
 t/t7502-commit.sh                    |  2 +-
 t/t7607-merge-overwrite.sh           |  4 ++--
 t/t7810-grep.sh                      |  2 +-
 t/t7811-grep-open.sh                 |  2 +-
 t/t8003-blame-corner-cases.sh        |  2 +-
 t/t8006-blame-textconv.sh            |  2 +-
 t/t9010-svn-fe.sh                    |  2 +-
 38 files changed, 71 insertions(+), 78 deletions(-)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 46f8e58..9f4955a 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -380,7 +380,7 @@ test_expect_success PERL 'required process filter should filter data' '
 		git commit -m "test commit 2" &&
 		rm -f test2.r "testsubdir/test3 '\''sq'\'',\$x=.r" &&
 
-		filter_git checkout --quiet --no-progress . &&
+		filter_git checkout --quiet --no-progress -- . &&
 		cat >expected.log <<-EOF &&
 			START
 			init handshake complete
@@ -466,7 +466,7 @@ test_expect_success PERL 'required process filter should be used only for "clean
 
 		rm test.r &&
 
-		filter_git checkout --quiet --no-progress . &&
+		filter_git checkout --quiet --no-progress -- . &&
 		# If the filter would be used for "smudge", too, we would see
 		# "IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]" here
 		cat >expected.log <<-EOF &&
@@ -580,7 +580,7 @@ test_expect_success PERL 'process filter should restart after unexpected write f
 		rm -f *.r &&
 
 		rm -f debug.log &&
-		git checkout --quiet --no-progress . 2>git-stderr.log &&
+		git checkout --quiet --no-progress -- . 2>git-stderr.log &&
 
 		grep "smudge write error at" git-stderr.log &&
 		grep "error: external filter" git-stderr.log &&
@@ -630,7 +630,7 @@ test_expect_success PERL 'process filter should not be restarted if it signals a
 		git add . &&
 		rm -f *.r &&
 
-		filter_git checkout --quiet --no-progress . &&
+		filter_git checkout --quiet --no-progress -- . &&
 		cat >expected.log <<-EOF &&
 			START
 			init handshake complete
@@ -669,7 +669,7 @@ test_expect_success PERL 'process filter abort stops processing of all further f
 
 		# Note: This test assumes that Git filters files in alphabetical
 		# order ("abort.r" before "test.r").
-		filter_git checkout --quiet --no-progress . &&
+		filter_git checkout --quiet --no-progress -- . &&
 		cat >expected.log <<-EOF &&
 			START
 			init handshake complete
@@ -763,7 +763,7 @@ test_expect_success PERL 'delayed checkout in process filter' '
 		test_cmp_committed_rot13 "$TEST_ROOT/test.o" test-delay10.b &&
 
 		rm *.a *.b &&
-		filter_git checkout . &&
+		filter_git checkout -- . &&
 		test_cmp_count ../a.exp a.log &&
 		test_cmp_count ../b.exp b.log &&
 
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index beb5927..3587e45 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -293,9 +293,9 @@ checkout_files () {
 	do
 		rm crlf_false_attr__$f.txt &&
 		if test -z "$ceol"; then
-			git checkout crlf_false_attr__$f.txt
+			git checkout -- crlf_false_attr__$f.txt
 		else
-			git -c core.eol=$ceol checkout crlf_false_attr__$f.txt
+			git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt
 		fi
 	done
 
diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index c167f60..82e0013 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -257,7 +257,7 @@ test_expect_success 'checkout without --ignore-skip-worktree-bits' '
 	echo sub >.git/info/sparse-checkout &&
 	git checkout &&
 	echo modified >> sub/added &&
-	git checkout . &&
+	git checkout -- . &&
 	test_path_is_missing init.t &&
 	git diff --exit-code HEAD
 '
@@ -269,7 +269,7 @@ test_expect_success 'checkout with --ignore-skip-worktree-bits' '
 	echo sub >.git/info/sparse-checkout &&
 	git checkout &&
 	echo modified >> sub/added &&
-	git checkout --ignore-skip-worktree-bits . &&
+	git checkout --ignore-skip-worktree-bits -- . &&
 	test_path_is_file init.t &&
 	git diff --exit-code HEAD
 '
diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index 6fd264c..270bb40 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -89,7 +89,7 @@ test_expect_success 'add a large file or two' '
 test_expect_success 'checkout a large file' '
 	large1=$(git rev-parse :large1) &&
 	git update-index --add --cacheinfo 100644 $large1 another &&
-	git checkout another &&
+	git checkout -- another &&
 	test_cmp large1 another
 '
 
diff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh
index 8b7640b..7326528 100755
--- a/t/t1051-large-conversion.sh
+++ b/t/t1051-large-conversion.sh
@@ -18,7 +18,7 @@ check_input() {
 
 check_output() {
 	rm -f small large &&
-	git checkout small large &&
+	git checkout -- small large &&
 	head -n 1 large >large.head &&
 	test_cmp small large.head
 }
@@ -76,7 +76,7 @@ test_expect_success 'user-defined filters convert on output' '
 test_expect_success 'ident converts on output' '
 	set_attr ident &&
 	rm -f small large &&
-	git checkout small large &&
+	git checkout -- small large &&
 	sed -n "s/Id: .*/Id: SHA/p" <small >small.clean &&
 	head -n 1 large >large.head &&
 	sed -n "s/Id: .*/Id: SHA/p" <large.head >large.clean &&
diff --git a/t/t2009-checkout-statinfo.sh b/t/t2009-checkout-statinfo.sh
index f3c2152..0a51b14 100755
--- a/t/t2009-checkout-statinfo.sh
+++ b/t/t2009-checkout-statinfo.sh
@@ -37,13 +37,13 @@ test_expect_success 'path checkout' '
 	git reset --hard &&
 	test "$(git diff-files --raw)" = "" &&
 
-	git checkout master world &&
+	git checkout master -- world &&
 	test "$(git diff-files --raw)" = "" &&
 
-	git checkout side world &&
+	git checkout side -- world &&
 	test "$(git diff-files --raw)" = "" &&
 
-	git checkout master world &&
+	git checkout master -- world &&
 	test "$(git diff-files --raw)" = ""
 
 '
diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
index 2e47fe0..87c8ad2 100755
--- a/t/t2010-checkout-ambiguous.sh
+++ b/t/t2010-checkout-ambiguous.sh
@@ -29,7 +29,7 @@ test_expect_success 'checkout world from the index' '
 '
 
 test_expect_success 'non ambiguous call' '
-	git checkout all
+	git checkout -- all
 '
 
 test_expect_success 'allow the most common case' '
@@ -44,7 +44,7 @@ test_expect_success 'check ambiguity' '
 test_expect_success 'check ambiguity in subdir' '
 	mkdir sub &&
 	# not ambiguous because sub/world does not exist
-	git -C sub checkout world ../all &&
+	git -C sub checkout world -- ../all &&
 	echo hello >sub/world &&
 	# ambiguous because sub/world does exist
 	test_must_fail git -C sub checkout world ../all
diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index 6ef1573..2581382 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh
@@ -34,9 +34,9 @@ test_expect_success '"checkout <submodule>" updates the index only' '
 	git update-index --refresh &&
 	git diff-files --quiet &&
 	git diff-index --quiet --cached HEAD &&
-	git checkout HEAD^ submodule &&
+	git checkout HEAD^ -- submodule &&
 	test_must_fail git diff-files --quiet &&
-	git checkout HEAD submodule &&
+	git checkout HEAD -- submodule &&
 	git diff-files --quiet
 '
 
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index 9cd0ac4..c39d2c4 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -79,13 +79,6 @@ test_expect_success PERL 'git checkout -p handles deletion' '
 # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
 # the failure case (and thus get out of the loop).
 
-test_expect_success PERL 'path limiting works: dir' '
-	set_state dir/foo work head &&
-	(echo y; echo n) | git checkout -p dir &&
-	verify_saved_state bar &&
-	verify_state dir/foo head head
-'
-
 test_expect_success PERL 'path limiting works: -- dir' '
 	set_state dir/foo work head &&
 	(echo y; echo n) | git checkout -p -- dir &&
@@ -103,7 +96,7 @@ test_expect_success PERL 'path limiting works: HEAD^ -- dir' '
 test_expect_success PERL 'path limiting works: foo inside dir' '
 	set_state dir/foo work head &&
 	# the third n is to get out in case it mistakenly does not apply
-	(echo y; echo n; echo n) | (cd dir && git checkout -p foo) &&
+	(echo y; echo n; echo n) | (cd dir && git checkout -p -- foo) &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
diff --git a/t/t2022-checkout-paths.sh b/t/t2022-checkout-paths.sh
index f46d049..6e254fa 100755
--- a/t/t2022-checkout-paths.sh
+++ b/t/t2022-checkout-paths.sh
@@ -26,7 +26,7 @@ test_expect_success 'checking out paths out of a tree does not clobber unrelated
 	echo untracked >expect.next2 &&
 	cat expect.next2 >dir/next2 &&
 
-	git checkout master dir &&
+	git checkout master -- dir &&
 
 	test_cmp expect.common dir/common &&
 	test_path_is_file dir/master &&
@@ -52,7 +52,7 @@ test_expect_success 'do not touch unmerged entries matching $path but not in $tr
 	EOF
 	git update-index --index-info <expect.next0 &&
 
-	git checkout master dir &&
+	git checkout master -- dir &&
 
 	test_cmp expect.common dir/common &&
 	test_path_is_file dir/master &&
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 5d5b363..3e869e8 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -111,7 +111,7 @@ test_expect_success 'remove locked worktree' '
 
 test_expect_success 'remove worktree with dirty tracked file' '
 	echo dirty >>destination/init.t &&
-	test_when_finished "git -C destination checkout init.t" &&
+	test_when_finished "git -C destination checkout -- init.t" &&
 	test_must_fail git worktree remove destination
 '
 
diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh
index 309199b..b599c37 100755
--- a/t/t2030-unresolve-info.sh
+++ b/t/t2030-unresolve-info.sh
@@ -108,7 +108,7 @@ test_expect_success 'plumbing clears' '
 test_expect_success 'add records checkout -m undoes' '
 	prime_resolve_undo &&
 	git diff HEAD &&
-	git checkout --conflict=merge fi/le &&
+	git checkout --conflict=merge -- fi/le &&
 	echo checkout used the record and removed it &&
 	check_resolve_undo removed &&
 	echo the index and the work tree is unmerged again &&
@@ -131,7 +131,7 @@ test_expect_success 'rerere and rerere forget' '
 	rerere_id=$(cd .git/rr-cache && echo */postimage) &&
 	rerere_id=${rerere_id%/postimage} &&
 	test -f .git/rr-cache/$rerere_id/postimage &&
-	git checkout -m fi/le &&
+	git checkout -m -- fi/le &&
 	echo resurrect the conflict &&
 	grep "^=======" fi/le &&
 	echo reresolve the conflict &&
@@ -157,7 +157,7 @@ test_expect_success 'rerere and rerere forget (subdirectory)' '
 	rerere_id=$(cd .git/rr-cache && echo */postimage) &&
 	rerere_id=${rerere_id%/postimage} &&
 	test -f .git/rr-cache/$rerere_id/postimage &&
-	(cd fi && git checkout -m le) &&
+	(cd fi && git checkout -m -- le) &&
 	echo resurrect the conflict &&
 	grep "^=======" fi/le &&
 	echo reresolve the conflict &&
diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
index 3fc484e..f376590 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-exclude.sh
@@ -103,7 +103,7 @@ test_expect_success \
      test_cmp expect output'
 
 test_expect_success 'restore gitignore' '
-	git checkout --ignore-skip-worktree-bits $allignores &&
+	git checkout --ignore-skip-worktree-bits -- $allignores &&
 	rm .git/index
 '
 
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index e243700..7503b0f 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -345,7 +345,7 @@ test_expect_success 'autostash is saved on editor failure with conflict' '
 	) &&
 	echo conflicting-content >expected &&
 	test_cmp expected file0 &&
-	git checkout file0 &&
+	git checkout -- file0 &&
 	git stash pop &&
 	echo uncommitted-content >expected &&
 	test_cmp expected file0
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 0acf4b1..742e467 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -52,7 +52,7 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '
 	test_must_fail git cherry-pick base..anotherpick &&
 	test_cmp_rev picked CHERRY_PICK_HEAD &&
 	# "oops, I forgot that these patches rely on the change from base"
-	git checkout HEAD foo &&
+	git checkout HEAD -- foo &&
 	git cherry-pick base &&
 	git cherry-pick picked &&
 	git cherry-pick --continue &&
@@ -205,7 +205,7 @@ test_expect_success '--abort refuses to clobber unrelated change, harder case' '
 	test_line_count = 2 log &&
 	test_must_fail git update-index --refresh &&
 
-	git checkout unrelated &&
+	git checkout -- unrelated &&
 	git cherry-pick --abort &&
 	test_cmp_rev initial HEAD
 '
diff --git a/t/t3910-mac-os-precompose.sh b/t/t3910-mac-os-precompose.sh
index 26dd5b7..6932f20 100755
--- a/t/t3910-mac-os-precompose.sh
+++ b/t/t3910-mac-os-precompose.sh
@@ -141,9 +141,9 @@ test_expect_success "git checkout nfc" '
 	git checkout f.$Odiarnfc
 '
 # Make it possible to checkout files with their NFD names
-test_expect_success "git checkout file nfd" '
+test_expect_success "git checkout -- file nfd" '
 	rm -f f.* &&
-	git checkout f.$Odiarnfd
+	git checkout -- f.$Odiarnfd
 '
 # Make it possible to checkout links with their NFD names
 test_expect_success "git checkout link nfd" '
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 17df491..90da499 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -552,7 +552,7 @@ test_expect_success 'check with space before tab in indent' '
 '
 
 test_expect_success '--check and --exit-code are not exclusive' '
-	git checkout x &&
+	git checkout -- x &&
 	git diff --check --exit-code
 '
 
@@ -769,7 +769,7 @@ test_expect_success 'checkdiff detects new trailing blank lines (2)' '
 '
 
 test_expect_success 'checkdiff allows new blank lines' '
-	git checkout x &&
+	git checkout -- x &&
 	mv x y &&
 	(
 		echo "/* This is new */" &&
diff --git a/t/t4117-apply-reject.sh b/t/t4117-apply-reject.sh
index d80187d..e97dfa5 100755
--- a/t/t4117-apply-reject.sh
+++ b/t/t4117-apply-reject.sh
@@ -49,7 +49,7 @@ test_expect_success setup '
 test_expect_success 'apply --reject is incompatible with --3way' '
 	test_when_finished "cat saved.file1 >file1" &&
 	git diff >patch.0 &&
-	git checkout file1 &&
+	git checkout -- file1 &&
 	test_must_fail git apply --reject --3way patch.0 &&
 	git diff --exit-code
 '
diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index 4fc27c5..ae5ace7 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -223,7 +223,7 @@ test_expect_success 'blank at EOF with --whitespace=fix (1)' '
 	{ cat expect; echo; } >one &&
 	git diff -- one >patch &&
 
-	git checkout one &&
+	git checkout -- one &&
 	git apply --whitespace=fix patch &&
 	test_cmp expect one
 '
@@ -235,7 +235,7 @@ test_expect_success 'blank at EOF with --whitespace=fix (2)' '
 	{ cat expect; echo; echo; } >one &&
 	git diff -- one >patch &&
 
-	git checkout one &&
+	git checkout -- one &&
 	git apply --whitespace=fix patch &&
 	test_cmp expect one
 '
@@ -247,7 +247,7 @@ test_expect_success 'blank at EOF with --whitespace=fix (3)' '
 	{ cat expect; echo; echo; } >one &&
 	git diff -- one >patch &&
 
-	git checkout one &&
+	git checkout -- one &&
 	git apply --whitespace=fix patch &&
 	test_cmp expect one
 '
@@ -259,7 +259,7 @@ test_expect_success 'blank at end of hunk, not at EOF with --whitespace=fix' '
 	cp expect one &&
 	git diff -- one >patch &&
 
-	git checkout one &&
+	git checkout -- one &&
 	git apply --whitespace=fix patch &&
 	test_cmp expect one
 '
@@ -271,7 +271,7 @@ test_expect_success 'blank at EOF with --whitespace=warn' '
 	cat one >expect &&
 	git diff -- one >patch &&
 
-	git checkout one &&
+	git checkout -- one &&
 	git apply --whitespace=warn patch 2>error &&
 	test_cmp expect one &&
 	grep "new blank line at EOF" error
@@ -284,7 +284,7 @@ test_expect_success 'blank at EOF with --whitespace=error' '
 	echo >>one &&
 	git diff -- one >patch &&
 
-	git checkout one &&
+	git checkout -- one &&
 	test_must_fail git apply --whitespace=error patch 2>error &&
 	test_cmp expect one &&
 	grep "new blank line at EOF" error
@@ -297,7 +297,7 @@ test_expect_success 'blank but not empty at EOF' '
 	cat one >expect &&
 	git diff -- one >patch &&
 
-	git checkout one &&
+	git checkout -- one &&
 	git apply --whitespace=warn patch 2>error &&
 	test_cmp expect one &&
 	grep "new blank line at EOF" error
@@ -309,7 +309,7 @@ test_expect_success 'applying beyond EOF requires one non-blank context line' '
 	{ echo b; } >>one &&
 	git diff -- one >patch &&
 
-	git checkout one &&
+	git checkout -- one &&
 	{ echo a; echo; } >one &&
 	cp one expect &&
 	test_must_fail git apply --whitespace=fix patch &&
diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
index fc898c9..f72ee38 100755
--- a/t/t5403-post-checkout-hook.sh
+++ b/t/t5403-post-checkout-hook.sh
@@ -64,7 +64,7 @@ test_expect_success 'post-checkout receives the right args with HEAD changed ' '
 '
 
 test_expect_success 'post-checkout receives the right args when not switching branches ' '
-	GIT_DIR=clone2/.git git checkout master b &&
+	GIT_DIR=clone2/.git git checkout master -- b &&
 	old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
 	new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
 	flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index f026837..e5b0daa 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -32,7 +32,7 @@ test_expect_success setup '
 	git commit -m "E" &&
 	git tag E &&
 	git checkout master &&
-	git checkout branch foo &&
+	git checkout branch -- foo &&
 	test_tick &&
 	git commit -m "B" &&
 	git tag B &&
diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
index 8f9b48a..4aae0e9 100755
--- a/t/t6026-merge-attr.sh
+++ b/t/t6026-merge-attr.sh
@@ -72,7 +72,7 @@ test_expect_success 'check merge result in working tree' '
 
 test_expect_success 'retry the merge with longer context' '
 	echo text conflict-marker-size=32 >>.gitattributes &&
-	git checkout -m text &&
+	git checkout -m -- text &&
 	sed -ne "/^\([<=>]\)\1\1\1*/{
 		s/ .*$//
 		p
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index f84ff94..7104f08 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -177,7 +177,7 @@ test_expect_success 'bisect start: no ".git/BISECT_START" if checkout error' '
 	grep "* other" branch.output > /dev/null &&
 	test_must_fail test -e .git/BISECT_START &&
 	test -z "$(git for-each-ref "refs/bisect/*")" &&
-	git checkout HEAD hello
+	git checkout HEAD -- hello
 '
 
 # $HASH1 is good, $HASH4 is bad, we skip $HASH3
diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh
index 5e8d5fa..2e2979b 100755
--- a/t/t6038-merge-text-auto.sh
+++ b/t/t6038-merge-text-auto.sh
@@ -51,7 +51,7 @@ test_expect_success setup '
 
 	git rm .gitattributes &&
 	rm file &&
-	git checkout file &&
+	git checkout -- file &&
 	test_tick &&
 	git commit -m "remove .gitattributes" &&
 	git tag c &&
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index c630aba..0bea83c 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -292,7 +292,7 @@ test_expect_success 'not just commits' '
 	git update-ref refs/replace/$ORIGINAL $REPLACED &&
 	mv file file.original &&
 
-	git checkout file &&
+	git checkout -- file &&
 	test_cmp file.replaced file
 '
 
diff --git a/t/t6111-rev-list-treesame.sh b/t/t6111-rev-list-treesame.sh
index 32474c2..bc46a77 100755
--- a/t/t6111-rev-list-treesame.sh
+++ b/t/t6111-rev-list-treesame.sh
@@ -43,7 +43,7 @@ test_expect_success setup '
 	test_commit "file=Blah" file "Blah" F &&
 
 	test_tick && git merge --no-commit third-branch &&
-	git checkout third-branch file &&
+	git checkout third-branch -- file &&
 	git commit &&
 	note G &&
 	git branch fiddler-branch &&
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index d4e6485..f46903f 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -481,7 +481,7 @@ test_expect_success 'mv -k does not accidentally destroy submodules' '
 	git status --porcelain >actual &&
 	grep "^R  sub -> dest/sub" actual &&
 	git reset --hard &&
-	git checkout .
+	git checkout -- .
 '
 
 test_expect_success 'moving a submodule in nested directories' '
diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index 615e7e0..de19773 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -192,7 +192,7 @@ test_expect_success 'grep --cached respects binary diff attribute (2)' '
 	rm .gitattributes &&
 	git grep --cached text t >actual &&
 	test_when_finished "git rm --cached .gitattributes" &&
-	test_when_finished "git checkout .gitattributes" &&
+	test_when_finished "git checkout -- .gitattributes" &&
 	test_cmp expect actual
 '
 
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 76c223c..65a3d24 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -498,7 +498,7 @@ test_expect_success 'checkout with an unmerged path can be ignored' '
 	cat sample >fild &&
 	cat sample >file &&
 	cat sample >filf &&
-	git checkout -f fild file filf &&
+	git checkout -f -- fild file filf &&
 	test_cmp expect fild &&
 	test_cmp expect filf &&
 	test_cmp sample file
@@ -511,11 +511,11 @@ test_expect_success 'checkout unmerged stage' '
 	cat sample >fild &&
 	cat sample >file &&
 	cat sample >filf &&
-	git checkout --ours . &&
+	git checkout --ours -- . &&
 	test_cmp expect fild &&
 	test_cmp expect filf &&
 	test_cmp expect file &&
-	git checkout --theirs file &&
+	git checkout --theirs -- file &&
 	test ztheirside = "z$(cat file)"
 '
 
@@ -677,7 +677,7 @@ test_expect_success 'custom merge driver with checkout -m' '
 	) &&
 
 	mv arm expect &&
-	git checkout -m arm &&
+	git checkout -m -- arm &&
 	test_cmp expect arm
 '
 
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 7b36954..7b22454 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -38,7 +38,7 @@ test_expect_success 'git clean with skip-worktree .gitignore' '
 	test -f obj.o &&
 	test -f build/lib.so &&
 	git update-index --no-skip-worktree .gitignore &&
-	git checkout .gitignore
+	git checkout -- .gitignore
 '
 
 test_expect_success 'git clean' '
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index fa61b1a..bae0137 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -30,12 +30,12 @@ test_expect_success 'setup: initial commit' '
 '
 
 test_expect_success '-m and -F do not mix' '
-	git checkout HEAD file && echo >>file && git add file &&
+	git checkout HEAD -- file && echo >>file && git add file &&
 	test_must_fail git commit -m foo -m bar -F file
 '
 
 test_expect_success '-m and -C do not mix' '
-	git checkout HEAD file && echo >>file && git add file &&
+	git checkout HEAD -- file && echo >>file && git add file &&
 	test_must_fail git commit -C HEAD -m illegal
 '
 
@@ -119,18 +119,18 @@ test_expect_success 'empty commit message' '
 '
 
 test_expect_success 'template "emptyness" check does not kick in with -F' '
-	git checkout HEAD file && echo >>file && git add file &&
+	git checkout HEAD -- file && echo >>file && git add file &&
 	git commit -t file -F file
 '
 
 test_expect_success 'template "emptyness" check' '
-	git checkout HEAD file && echo >>file && git add file &&
+	git checkout HEAD -- file && echo >>file && git add file &&
 	test_must_fail git commit -t file 2>err &&
 	test_i18ngrep "did not edit" err
 '
 
 test_expect_success 'setup: commit message from file' '
-	git checkout HEAD file && echo >>file && git add file &&
+	git checkout HEAD -- file && echo >>file && git add file &&
 	echo this is the commit message, coming from a file >msg &&
 	git commit -F msg -a
 '
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index d33a3cb..ec8df56 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -470,7 +470,7 @@ test_expect_success 'Hand committing of a redundant merge removes dups' '
 
 	git rev-parse second master >expect &&
 	test_must_fail git merge second master &&
-	git checkout master g &&
+	git checkout master -- g &&
 	EDITOR=: git commit -a &&
 	git cat-file commit HEAD | sed -n -e "s/^parent //p" -e "/^$/q" >actual &&
 	test_cmp expect actual
diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index 9c422bc..5e2ea53 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -55,7 +55,7 @@ test_expect_success 'will not overwrite staged changes' '
 	rm c2.c &&
 	test_must_fail git merge c2 &&
 	test_path_is_missing .git/MERGE_HEAD &&
-	git checkout c2.c &&
+	git checkout -- c2.c &&
 	test_cmp important c2.c
 '
 
@@ -88,7 +88,7 @@ test_expect_success 'will not overwrite removed file with staged changes' '
 	rm c1.c &&
 	test_must_fail git merge c1a &&
 	test_path_is_missing .git/MERGE_HEAD &&
-	git checkout c1.c &&
+	git checkout -- c1.c &&
 	test_cmp important c1.c
 '
 
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1797f63..8d00cc6 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -714,7 +714,7 @@ test_expect_success 'grep with CE_VALID file' '
 	rm t/t &&
 	test "$(git grep test)" = "t/t:test" &&
 	git update-index --no-assume-unchanged t/t &&
-	git checkout t/t
+	git checkout -- t/t
 '
 
 cat >expected <<EOF
diff --git a/t/t7811-grep-open.sh b/t/t7811-grep-open.sh
index e1951a5..e0e0fbf 100755
--- a/t/t7811-grep-open.sh
+++ b/t/t7811-grep-open.sh
@@ -119,7 +119,7 @@ test_expect_success 'modified file' '
 	>empty &&
 
 	echo "enum grep_pat_token" >unrelated &&
-	test_when_finished "git checkout HEAD unrelated" &&
+	test_when_finished "git checkout HEAD -- unrelated" &&
 	GIT_PAGER=./less git grep -F -O "enum grep_pat_token" >out &&
 	test_cmp expect actual &&
 	test_cmp empty out
diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index 661f9d4..da3f98f 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -265,7 +265,7 @@ test_expect_success 'blame file with CRLF core.autocrlf=true' '
 	git commit -m "add crlfinrepo" &&
 	git config core.autocrlf true &&
 	mv crlfinrepo tmp &&
-	git checkout crlfinrepo &&
+	git checkout -- crlfinrepo &&
 	rm tmp &&
 	git blame crlfinrepo >actual &&
 	grep "A U Thor" actual
diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
index 7683515..a81d677 100755
--- a/t/t8006-blame-textconv.sh
+++ b/t/t8006-blame-textconv.sh
@@ -73,7 +73,7 @@ test_expect_success 'blame --textconv going through revisions' '
 '
 
 test_expect_success 'blame --textconv with local changes' '
-	test_when_finished "git checkout zero.bin" &&
+	test_when_finished "git checkout -- zero.bin" &&
 	printf "bin: updated number 0\015" >zero.bin &&
 	git blame --textconv zero.bin >blame &&
 	expect="(Not Committed Yet ....-..-.. ..:..:.. +0000 1)" &&
diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh
index 8eaaca6..ba54fb2 100755
--- a/t/t9010-svn-fe.sh
+++ b/t/t9010-svn-fe.sh
@@ -258,7 +258,7 @@ test_expect_success 'directory with files' '
 	try_dump directory.dump &&
 
 	git ls-tree -r --name-only HEAD >actual.files &&
-	git checkout HEAD directory &&
+	git checkout HEAD -- directory &&
 	test_cmp expect.files actual.files &&
 	test_cmp hello directory/file1 &&
 	test_cmp hi directory/file2
-- 
2.7.4

[PATCH 3/3] doc: update doc for strict usage of -- checkout

[Dannier Castro L]

The flag '--' must always be before any file path when <checkout>
command is used.

The main documentation about the usage of <checkout> command is
updated to include the strict usage of the flag '--' so that the
user can specify file names over branch names.

Signed-off-by: Dannier Castro L <danniercl@gmail.com>:
---
 Documentation/git-checkout.txt | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index ca5fc9c..8360a98 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -12,9 +12,9 @@ SYNOPSIS
 'git checkout' [-q] [-f] [-m] --detach [<branch>]
 'git checkout' [-q] [-f] [-m] [--detach] <commit>
 'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] <new_branch>] [<start_point>]
-'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...
-'git checkout' [<tree-ish>] [--] <pathspec>...
-'git checkout' (-p|--patch) [<tree-ish>] [--] [<paths>...]
+'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] -- <paths>...
+'git checkout' [<tree-ish>] -- <pathspec>...
+'git checkout' (-p|--patch) [<tree-ish>] [-- <paths>...]
 
 DESCRIPTION
 -----------
@@ -79,7 +79,7 @@ be used to detach HEAD at the tip of the branch (`git checkout
 +
 Omitting <branch> detaches HEAD at the tip of the current branch.
 
-'git checkout' [<tree-ish>] [--] <pathspec>...::
+'git checkout' [<tree-ish>] -- <pathspec>...::
 
 	Overwrite paths in the working tree by replacing with the
 	contents in the index or in the <tree-ish> (most often a
@@ -95,7 +95,7 @@ specific side of the merge can be checked out of the index by
 using `--ours` or `--theirs`.  With `-m`, changes made to the working tree
 file can be discarded to re-create the original conflicted merge result.
 
-'git checkout' (-p|--patch) [<tree-ish>] [--] [<pathspec>...]::
+'git checkout' (-p|--patch) [<tree-ish>] [-- <pathspec>...]::
 	This is similar to the "check out paths to the working tree
 	from either the index or from a tree-ish" mode described
 	above, but lets you use the interactive interface to show
@@ -453,7 +453,7 @@ mistake, and gets it back from the index.
 $ git checkout master             <1>
 $ git checkout master~2 Makefile  <2>
 $ rm -f hello.c
-$ git checkout hello.c            <3>
+$ git checkout -- hello.c         <3>
 ------------
 +
 <1> switch branch
@@ -471,14 +471,6 @@ Note the quotes around `*.c`.  The file `hello.c` will also be
 checked out, even though it is no longer in the working tree,
 because the file globbing is used to match entries in the index
 (not in the working tree by the shell).
-+
-If you have an unfortunate branch that is named `hello.c`, this
-step would be confused as an instruction to switch to that branch.
-You should instead write:
-+
-------------
-$ git checkout -- hello.c
-------------
 
 . After working in the wrong branch, switching to the correct
 branch would be done using:
-- 
2.7.4

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Duy Nguyen]

On Sun, May 13, 2018 at 4:23 AM, Dannier Castro L <danniercl@gmail.com> wrote:
> Currently, <checkout> is a complex command able to handle both
> branches and files without any distintion other than their names,
> taking into account that depending on the type (branch or file),
> the functionality is completely different, the easier example:
>
> $ git checkout <branch>  # Switch from current branch to <branch>.
> $ git checkout <file>    # Restore <file> from HEAD, discarding
>                          # changes if it's necessary.
> $ git checkout -- <file> # The same as the last one, only with an
>                          # useless '--'.
>
> For GIT new users, this complicated versatility of <checkout> could
> be very confused, also considering that actually the flag '--' is
> completely useless (added or not, there is not any difference for
> this command), when the same program messages promote the use of
> this flag.

I would like an option to revert back to current behavior. I'm not a
new user. I know what I'm doing. Please don't make me type more.

And '--" is not completely useless. If you have <file> and <branch>
with the same name, you have to give "--" to to tell git what the
first argument means.

> Regarding the <checkout>'s power to overwrite any file, discarding
> changes if they exist without some way of recovering them, the
> solution propuses that the usage of '--' is strict before to
> specify the file(s) path(s) for any <checkout> command (including
> all types of flags), as a "defense barrier" to make sure about
> user's knowledge and intension running <checkout>.
>
> The solution consists in detect '--' into command args, allowing
> the discard of changes and considering the following names as
> file paths, otherwise, they are branch names.
-- 
Duy

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Matthieu Moy]

"Dannier Castro L" <danniercl@gmail.com> wrote:

> Currently, <checkout> is a complex command able to handle both
> branches and files without any distintion other than their names,
> taking into account that depending on the type (branch or file),
> the functionality is completely different, the easier example:
> 
> $ git checkout <branch>  # Switch from current branch to <branch>.
> $ git checkout <file>    # Restore <file> from HEAD, discarding
>                         # changes if it's necessary.
> $ git checkout -- <file> # The same as the last one, only with an
>                         # useless '--'.

It's not really "useless".

In the first two commands you give, git guesses whether the first
argument is a branch or a file. In the 3rd, the -- indicates that it
must be a file.

> For GIT new users,

Nit: we write Git (for the system) or git (for the command-line tool),
but usually avoid the all-caps GIT.

> The solution consists in detect '--' into command args, allowing
> the discard of changes and considering the following names as
> file paths, otherwise, they are branch names.

This answers the "what?" question, but does not explain why this is a
good change. A good commit message should focus more on the "why?"
question.

While I agree that "git checkout" is a complex command, and would love
to see a simpler syntax at least for the most common operations, I do
not think that this is a good change for several reasons:

* If one considers that this "--" syntax is an issue, then this patch
  is a very partial solution only. Many other commands use the same
  convention (for example "git log <commit>" Vs "git log -- <file>"),
  so changing only one makes git less consistent. Also, note that this
  "--" convention is not git-specific. Try "touch --help" and "touch
  -- --help" for example.

* This breaks backward compatibility. People used to "git checkout
  <file>" won't be able to use it anymore. Scripts using it will
  break. Git avoids breaking backward compatibility, and when there's
  a good reason to do so we need a good transition plan. In this case,
  one possible plan would be to 1) issue a warning whenever "git
  checkout <file>" is used for a while, and then 2) actually forbid
  it. But following this path, I don't think step 2) would actually be
  useful.

> @@ -928,6 +931,7 @@ static int parse_branchname_arg(int argc, const char **argv,
> 	dash_dash_pos = -1;
> 	for (i = 0; i < argc; i++) {
> 		if (!strcmp(argv[i], "--")) {
> +			opts->discard_changes = 1;
> 			dash_dash_pos = i;

Wouldn't "dash_dash_pos != -1" be enough to know whether there's a --?

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Dannier Castro L]

On 13/05/2018 00:03, Duy Nguyen wrote:

> On Sun, May 13, 2018 at 4:23 AM, Dannier Castro L <danniercl@gmail.com> wrote:
>> For GIT new users, this complicated versatility of <checkout> could
>> be very confused, also considering that actually the flag '--' is
>> completely useless (added or not, there is not any difference for
>> this command), when the same program messages promote the use of
>> this flag.
> I would like an option to revert back to current behavior. I'm not a
> new user. I know what I'm doing. Please don't make me type more.
>
> And '--" is not completely useless. If you have <file> and <branch>
> with the same name, you have to give "--" to to tell git what the
> first argument means.

Sure Duy, you're right, probably "completely useless" is not the correct
definition, even according with the code I didn't find another useful
case that is not file and branch with the same name. The program is able
to know the type using only the name, turning "--" into an extra flag in
most of cases.

I think this solution could please you more: By default the configuration
is the current, but the user has the chance to set this, for example:

git config --global flag.strictdashdash true

Thank you so much for the spent time reviewing the patch, this is my
first one in this repository.

-Dannier CL

Re: Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[SZEDER Gábor]

> On 13/05/2018 00:03, Duy Nguyen wrote:
> 
> > On Sun, May 13, 2018 at 4:23 AM, Dannier Castro L <danniercl@gmail.com> wrote:
> >> For GIT new users, this complicated versatility of <checkout> could
> >> be very confused, also considering that actually the flag '--' is
> >> completely useless (added or not, there is not any difference for
> >> this command), when the same program messages promote the use of
> >> this flag.
> > I would like an option to revert back to current behavior. I'm not a
> > new user. I know what I'm doing. Please don't make me type more.
> >
> > And '--" is not completely useless. If you have <file> and <branch>
> > with the same name, you have to give "--" to to tell git what the
> > first argument means.
> 
> Sure Duy, you're right, probably "completely useless" is not the correct
> definition,

No, "completely useless" is just plain wrong.

> even according with the code I didn't find another useful
> case that is not file and branch with the same name.

The optional disambiguating doubledash is the standard way to erm, well,
to disambiguate whatever there is to disambiguate.  Not only refs and
filenames, but also e.g. options and filenames:

  $ git checkout --option-looking-file
  error: unknown option `option-looking-file'
  usage: git checkout [<options>] <branch>
  <...>
  $ git checkout -- --option-looking-file
  error: pathspec '--option-looking-file' did not match any file(s) known to git.

Note that this is not at all specific to Git, many other programs
support the optional disambiguating doubledash as well:

  $ touch --option-looking-file
  touch: unrecognized option '--option-looking-file'
  Try 'touch --help' for more information.
  $ touch -- --option-looking-file
  $ ls --option-looking-file
  ls: unrecognized option '--option-looking-file'
  Try 'ls --help' for more information.
  $ ls -- --option-looking-file
  --option-looking-file

Please do not make it mandatory.

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Kevin Daudt]

On Sat, May 12, 2018 at 08:23:32PM -0600, Dannier Castro L wrote:
> Currently, <checkout> is a complex command able to handle both
> branches and files without any distintion other than their names,
> taking into account that depending on the type (branch or file),
> the functionality is completely different, the easier example:
> 
> $ git checkout <branch>  # Switch from current branch to <branch>.
> $ git checkout <file>    # Restore <file> from HEAD, discarding
>                          # changes if it's necessary.
> $ git checkout -- <file> # The same as the last one, only with an
>                          # useless '--'.
> 
> For GIT new users, this complicated versatility of <checkout> could
> be very confused, also considering that actually the flag '--' is
> completely useless (added or not, there is not any difference for
> this command), when the same program messages promote the use of
> this flag.
> 
> Regarding the <checkout>'s power to overwrite any file, discarding
> changes if they exist without some way of recovering them, the
> solution propuses that the usage of '--' is strict before to
> specify the file(s) path(s) for any <checkout> command (including
> all types of flags), as a "defense barrier" to make sure about
> user's knowledge and intension running <checkout>.
> 
> The solution consists in detect '--' into command args, allowing
> the discard of changes and considering the following names as
> file paths, otherwise, they are branch names.
> 
> Signed-off-by: Dannier Castro L <danniercl@gmail.com>

One data point indicating this is giving issues is that today on IRC a
user was confused why `git checkout pt` did not show any message and did
not checkout a remote branch called 'pt' as they expected. It turned out
they also had a local file/dir called 'pt', which caused git to checkout
that file/dir rather than creating a local branch based on the remote
branch.

Kevin

Re: Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Philip Oakley]

From: "Dannier Castro L" <danniercl@gmail.com>
> On 13/05/2018 00:03, Duy Nguyen wrote:
>
>> On Sun, May 13, 2018 at 4:23 AM, Dannier Castro L <danniercl@gmail.com>
>> wrote:
>>> For GIT new users, this complicated versatility of <checkout> could
>>> be very confused, also considering that actually the flag '--' is
>>> completely useless (added or not, there is not any difference for
>>> this command), when the same program messages promote the use of
>>> this flag.
>> I would like an option to revert back to current behavior. I'm not a
>> new user. I know what I'm doing. Please don't make me type more.
>>
>> And '--" is not completely useless. If you have <file> and <branch>
>> with the same name, you have to give "--" to to tell git what the
>> first argument means.
>
> Sure Duy, you're right, probably "completely useless" is not the correct
> definition, even according with the code I didn't find another useful
> case that is not file and branch with the same name. The program is able
> to know the type using only the name, turning "--" into an extra flag in
> most of cases.
>
> I think this solution could please you more: By default the configuration
> is the current, but the user has the chance to set this, for example:
>
> git config --global flag.strictdashdash true
>
> Thank you so much for the spent time reviewing the patch, this is my
> first one in this repository.

It maybe that after review you could suggest an appropriate rewording or
re-arrangement of the man page to better highlight the proper use of the
'--' disambiguation.

Perhaps frame the man page as if it is normal for the '--' to be included
within command lines (which should be the case for scripts anyway!).

Then indicate that it isn't mandatory if the file/branch/dwim distinction is
obvious. i.e. make sure that the man page is educational as well as being a
reference that may be misunderstood.

Those well versed in the Git cli will normally omit the '--', only using it
where necessary, however for a new users/readers of the man page, it may be
better to be more explicit and avoid future misunderstandings.

--
Philip

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>
> I would like an option to revert back to current behavior. I'm not a
> new user. I know what I'm doing. Please don't make me type more.

I can guarantee that nobody will stay a newbie.  They either become
more proficient and aware of what they are doing without having to
think, at which point they start feeling the same way as you are.
Or they leave the Git ecosystem and move to better things ;-)

> And '--" is not completely useless. If you have <file> and <branch>
> with the same name, you have to give "--" to to tell git what the
> first argument means.

Correct.

We _could_ do better than the corrent code, though, when we happen
to have a file called 'master'.  "git checkout master master" cannot
mean anything other than "I want to make the index and the working
tree copies of 'master' file to match what is recorded on the master
branch", but I think we do require "git checkout master -- master"
disambiguation.

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Duy Nguyen]

On Sun, May 13, 2018 at 11:02 PM, Kevin Daudt <me@ikke.info> wrote:
> One data point indicating this is giving issues is that today on IRC a
> user was confused why `git checkout pt` did not show any message and did
> not checkout a remote branch called 'pt' as they expected. It turned out
> they also had a local file/dir called 'pt', which caused git to checkout
> that file/dir rather than creating a local branch based on the remote
> branch.

Now this is something we should fix. When an argument can be
interpreted in more than one way Git should reject it, but I think
this ambiguation logic does not take dwim (i.e. create a new branch
beased on remote) into account.
-- 
Duy

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Duy Nguyen]

On Mon, May 14, 2018 at 04:52:53PM +0200, Duy Nguyen wrote:
> On Sun, May 13, 2018 at 11:02 PM, Kevin Daudt <me@ikke.info> wrote:
> > One data point indicating this is giving issues is that today on IRC a
> > user was confused why `git checkout pt` did not show any message and did
> > not checkout a remote branch called 'pt' as they expected. It turned out
> > they also had a local file/dir called 'pt', which caused git to checkout
> > that file/dir rather than creating a local branch based on the remote
> > branch.
> 
> Now this is something we should fix. When an argument can be
> interpreted in more than one way Git should reject it, but I think
> this ambiguation logic does not take dwim (i.e. create a new branch
> beased on remote) into account.

This is a quick draft to make this happen

-- 8< --
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b49b582071..f4f6951f05 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -953,8 +953,19 @@ 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)))
+		if (!has_dash_dash && check_filename(opts->prefix, arg) &&
+		    recover_with_dwim) {
+			const char *remote = unique_tracking_name(arg, rev);
+			if (remote)
+				die(_("don't know whether to create a tracking "
+				      "branch from remote %s or to checkout path %s, "
+				      "use -- to disambiguate"),
+				    remote, arg);
+			printf("here?\n");
+			recover_with_dwim = 0;
+		}
+
+		if (!has_dash_dash && !no_wildcard(arg))
 			recover_with_dwim = 0;
 		/*
 		 * Accept "git checkout foo" and "git checkout foo --"
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 3e5ac81bd2..ea95fb8668 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -215,4 +215,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
-- 8< --