Extend --pathspec-from-file to git add, checkout

This topic continues the effort to support --pathspec-from-file in various commands [1][2]. It also includes some refactorings that I developed while working on it - previously submitted separately [3][4] which was probably a mistake.

Anatomy of the branch:
checkout, restore: support the --pathspec-from-file option
Extends --pathspec-from-file to git checkout/restore.

t2024: cover more cases
Some new tests for cases that I deemed worthy while working on parse_branchname_arg() refactoring.

parse_branchname_arg(): refactor the decision making parse_branchname_arg(): update code comments parse_branchname_arg(): introduce expect_commit_only parse_branchname_arg(): easier to understand variables
These patches prepare for || opts->pathspec_from_file addition in git checkout/restore patch. Without this refactoring, I found it pretty hard to modify the old code.

checkout: die() on ambiguous tracking branches parse_branchname_arg(): extract part as new function
Initially I was trying to remove some inconsistency standing in the way of git checkout/restore patch. Later I figured that this change is worthy on its own: it prevents some pretty surprising behavior of git.

doc: restore: synchronize <pathspec> description
doc: checkout: synchronize <pathspec> description
doc: checkout: fix broken text reference
doc: checkout: remove duplicate synopsis
Some polishing of docs in preparation for git checkout/restore patch.

add: support the --pathspec-from-file option
cmd_add: prepare for next patch
Extends --pathspec-from-file to git add.

commit: forbid --pathspec-from-file --all t7107, t7526: directly test parse_pathspec_file()
Some polishing of merged topic [1].

CC'ing people who shown interest in any of the previous topics, thanks for your reviews!

[1] https://public-inbox.org/git/[email protected]/
[2] https://public-inbox.org/git/[email protected]/
[3] https://public-inbox.org/git/[email protected]/
[4] https://public-inbox.org/git/[email protected]/

Alexandr Miloslavskiy (16):
t7107, t7526: directly test parse_pathspec_file()
commit: forbid --pathspec-from-file --all
cmd_add: prepare for next patch
add: support the --pathspec-from-file option
doc: checkout: remove duplicate synopsis
doc: checkout: fix broken text reference
doc: checkout: synchronize <pathspec> description
doc: restore: synchronize <pathspec> description parse_branchname_arg(): extract part as new function
checkout: die() on ambiguous tracking branches parse_branchname_arg(): easier to understand variables parse_branchname_arg(): introduce expect_commit_only parse_branchname_arg(): update code comments parse_branchname_arg(): refactor the decision making
t2024: cover more cases
checkout, restore: support the --pathspec-from-file option

 Documentation/git-add.txt           |  16 +-
 Documentation/git-checkout.txt      |  50 +++--
 Documentation/git-restore.txt       |  26 ++-
 Makefile                            |   1 +
 builtin/add.c                       |  60 ++++--
 builtin/checkout.c                  | 279 ++++++++++++++--------------
 builtin/commit.c                    |   3 +
 t/helper/test-parse-pathspec-file.c |  34 ++++
 t/helper/test-tool.c                |   1 +
 t/helper/test-tool.h                |   1 +
 t/t0067-parse_pathspec_file.sh      |  89 +++++++++
 t/t2024-checkout-dwim.sh            |  55 +++++-
 t/t2026-checkout-pathspec-file.sh   |  61 ++++++
 t/t2072-restore-pathspec-file.sh    | 139 ++++++++++++++
 t/t3704-add-pathspec-file.sh        |  49 +++++
 t/t7107-reset-pathspec-file.sh      | 105 +----------
 t/t7526-commit-pathspec-file.sh     |  80 +-------
 t/t9902-completion.sh               |   2 +
 18 files changed, 694 insertions(+), 357 deletions(-)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh
 create mode 100755 t/t2026-checkout-pathspec-file.sh
 create mode 100755 t/t2072-restore-pathspec-file.sh
 create mode 100755 t/t3704-add-pathspec-file.sh

base-commit: ad05a3d8e5a6a06443836b5e40434262d992889a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-490%2FSyntevoAlex%2F%230207_pathspec_from_file_2-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-490/SyntevoAlex/#0207_pathspec_from_file_2-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/490

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

I forgot this in my previous patch --pathspec-from-file for git commit [1]. When both --pathspec-from-file and --all were specified, --all took precedence and --pathspec-from-file was ignored. Before --pathspec-from-file was implemented, this case was prevented by this check in parse_and_validate_options() :

    die(_("paths '%s ...' with -a does not make sense"), argv[0]);

It is unfortunate that these two cases are disconnected. This came as result of how the code was laid out before my patches, where pathspec is parsed outside of parse_and_validate_options(). This branch is already full of refactoring patches and I did not dare to go for another one.

Fix by mirroring die() for --pathspec-from-file as well.

[1] Commit e440fc58 ("commit: support the --pathspec-from-file option" 2019-11-19)

---
 builtin/commit.c | 3 +++
 1 file changed, 3 insertions(+)

index 2db2ad0de4..893a9f29b2 100644
@@ -347,6 +347,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 		if (interactive)
 			die(_("--pathspec-from-file is incompatible with --interactive/--patch"));
 
+		if (all)
+			die(_("--pathspec-from-file with -a does not make sense"));
+
 		if (pathspec.nr)
 			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

In my previous patches, parse_pathspec_file() was tested indirectly by invoking git reset and git commit with properly crafted inputs. This has some disadvantages:
1) A number of tests are copy&pasted for every command where --pathspec-from-file is supported. With just two commands, it wasn't too bad, but I'm going to extend support to many more commands, which would make a handful of low-value tests.
2) Tests are located in suboptimal test packages
3) Tests are indirect

Fix this by testing parse_pathspec_file() directly via a new test helper.

While working on it, I also noticed that quotes testing via "\"file\\101.t\"" was somewhat incorrect: I escaped \ one time while I had to escape it two times! Tests still worked due to " which prevented pathspec from matching files.

Fix this by properly escaping one more time.

---
 Makefile                            |   1 +
 t/helper/test-parse-pathspec-file.c |  34 +++++++++
 t/helper/test-tool.c                |   1 +
 t/helper/test-tool.h                |   1 +
 t/t0067-parse_pathspec_file.sh      |  89 +++++++++++++++++++++++
 t/t7107-reset-pathspec-file.sh      | 105 ++--------------------------
 t/t7526-commit-pathspec-file.sh     |  80 +--------------------
 7 files changed, 134 insertions(+), 177 deletions(-)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh

index b7d7374dac..fd7bcaac9c 100644
@@ -721,6 +721,7 @@ TEST_BUILTINS_OBJS += test-mktemp.o
 TEST_BUILTINS_OBJS += test-oidmap.o
 TEST_BUILTINS_OBJS += test-online-cpus.o
 TEST_BUILTINS_OBJS += test-parse-options.o
+TEST_BUILTINS_OBJS += test-parse-pathspec-file.o
 TEST_BUILTINS_OBJS += test-path-utils.o
 TEST_BUILTINS_OBJS += test-pkt-line.o
 TEST_BUILTINS_OBJS += test-prio-queue.o

new file mode 100644
index 0000000000..e7f525feb9
@@ -0,0 +1,34 @@
+#include "test-tool.h"
+#include "parse-options.h"
+#include "pathspec.h"
+#include "gettext.h"
+
+int cmd__parse_pathspec_file(int argc, const char **argv)
+{
+	struct pathspec pathspec;
+	const char *pathspec_from_file = 0;
+	int pathspec_file_nul = 0, i;
+
+	static const char *const usage[] = {
+		"test-tool parse-pathspec-file --pathspec-from-file [--pathspec-file-nul]",
+		NULL
+	};
+
+	struct option options[] = {
+		OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
+		OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
+		OPT_END()
+	};
+
+	parse_options(argc, argv, 0, options, usage, 0);
+
+	parse_pathspec_file(&pathspec, 0, 0, 0, pathspec_from_file,
+			    pathspec_file_nul);
+
+	for (i = 0; i < pathspec.nr; i++) {
+		printf("%s\n", pathspec.items[i].original);
+	}
+
+	clear_pathspec(&pathspec);
+	return 0;
+}

index f20989d449..c9a232d238 100644
@@ -39,6 +39,7 @@ static struct test_cmd cmds[] = {
 	{ "oidmap", cmd__oidmap },
 	{ "online-cpus", cmd__online_cpus },
 	{ "parse-options", cmd__parse_options },
+	{ "parse-pathspec-file", cmd__parse_pathspec_file },
 	{ "path-utils", cmd__path_utils },
 	{ "pkt-line", cmd__pkt_line },
 	{ "prio-queue", cmd__prio_queue },

index 8ed2af71d1..c8549fd87f 100644
@@ -29,6 +29,7 @@ int cmd__mktemp(int argc, const char **argv);
 int cmd__oidmap(int argc, const char **argv);
 int cmd__online_cpus(int argc, const char **argv);
 int cmd__parse_options(int argc, const char **argv);
+int cmd__parse_pathspec_file(int argc, const char** argv);
 int cmd__path_utils(int argc, const char **argv);
 int cmd__pkt_line(int argc, const char **argv);
 int cmd__prio_queue(int argc, const char **argv);

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

---
 Documentation/git-checkout.txt | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

index 2011fdbb1d..d47046e050 100644
@@ -95,12 +95,10 @@ 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>...]::
-	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
-	the "diff" output and choose which hunks to use in the
-	result.  See below for the description of `--patch` option.
-
+	This is similar to the previous mode, but lets you use the
+	interactive interface to show the "diff" output and choose which
+	hunks to use in the result.  See below for the description of
+	`--patch` option.
 
 OPTIONS
 -------

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Some code blocks were moved down to be able to test for pathspec.nr in the next patch. Blocks are moved as is without any changes. This is done as separate patch to reduce the amount of diffs in next patch.

---
 builtin/add.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

index d4686d5218..3d1791dd82 100644
@@ -430,10 +430,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (addremove && take_worktree_changes)
 		die(_("-A and -u are mutually incompatible"));
 
-	if (!take_worktree_changes && addremove_explicit < 0 && argc)
-		/* Turn "git add pathspec..." to "git add -A pathspec..." */
-		addremove = 1;
-
 	if (!show_only && ignore_missing)
 		die(_("Option --ignore-missing can only be used together with --dry-run"));
 

@@ -446,19 +442,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
 
-	flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
-		 (show_only ? ADD_CACHE_PRETEND : 0) |
-		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
-		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
-		 (!(addremove || take_worktree_changes)
-		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
-
-	if (require_pathspec && argc == 0) {
-		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
-		return 0;
-	}
-
 	/*
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.

@@ -468,6 +451,23 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
 
+	if (require_pathspec && argc == 0) {
+		fprintf(stderr, _("Nothing specified, nothing added.\n"));
+		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
+		return 0;
+	}
+
+	if (!take_worktree_changes && addremove_explicit < 0 && argc)
+		/* Turn "git add pathspec..." to "git add -A pathspec..." */
+		addremove = 1;
+
+	flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
+		 (show_only ? ADD_CACHE_PRETEND : 0) |
+		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
+		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
+		 (!(addremove || take_worktree_changes)
+		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
+
 	if (read_cache_preload(&pathspec) < 0)
 		die(_("index file corrupt"));
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

dash_dash_pos was only calculated under opts->accept_pathspec. This is unexpected to readers and made it harder to reason about the code. Fix this by restoring the expected meaning.

Simplify the code by dropping argcount and useless argc / argv manipulations.

This should not change behavior in any way.

---
 builtin/checkout.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

index b847695d2b..f35fe2cc26 100644
@@ -1152,7 +1152,6 @@ static int parse_branchname_arg(int argc, const char **argv,
 				struct object_id *rev)
 {
 	const char **new_branch = &opts->new_branch;
-	int argcount = 0;
 	const char *arg;
 	int dash_dash_pos;
 	int has_dash_dash = 0;

@@ -1211,17 +1210,21 @@ static int parse_branchname_arg(int argc, const char **argv,
 	arg = argv[0];
 	dash_dash_pos = -1;
 	for (i = 0; i < argc; i++) {
-		if (opts->accept_pathspec && !strcmp(argv[i], "--")) {
+		if (!strcmp(argv[i], "--")) {
 			dash_dash_pos = i;
 			break;
 		}
 	}
-	if (dash_dash_pos == 0)
-		return 1; /* case (2) */
-	else if (dash_dash_pos == 1)
-		has_dash_dash = 1; /* case (3) or (1) */
-	else if (dash_dash_pos >= 2)
-		die(_("only one reference expected, %d given."), dash_dash_pos);
+
+	if (opts->accept_pathspec) {
+	    if (dash_dash_pos == 0)
+		    return 1; /* case (2) */
+	    else if (dash_dash_pos == 1)
+		    has_dash_dash = 1; /* case (3) or (1) */
+	    else if (dash_dash_pos >= 2)
+		    die(_("only one reference expected, %d given."), dash_dash_pos);
+	}
+
 	opts->count_checkout_paths = !opts->quiet && !has_dash_dash;
 
 	if (!strcmp(arg, "-"))

@@ -1268,15 +1271,10 @@ static int parse_branchname_arg(int argc, const char **argv,
 		if (!recover_with_dwim) {
 			if (has_dash_dash)
 				die(_("invalid reference: %s"), arg);
-			return argcount;
+			return 0;
 		}
 	}
 
-	/* we can't end up being in (2) anymore, eat the argument */
-	argcount++;
-	argv++;
-	argc--;
-
 	setup_new_branch_info_and_source_tree(new_branch_info, opts, rev, arg);
 
 	if (!opts->source_tree)                   /* case (1): want a tree */

@@ -1289,15 +1287,11 @@ static int parse_branchname_arg(int argc, const char **argv,
 		 * even if there happen to be a file called 'branch';
 		 * it would be extremely annoying.
 		 */
-		if (argc)
+		if (argc > 1)
 			verify_non_filename(opts->prefix, arg);
-	} else if (opts->accept_pathspec) {
-		argcount++;
-		argv++;
-		argc--;
 	}
 
-	return argcount;
+	return (dash_dash_pos == 1) ? 2 : 1;
 }
 
 static int switch_unborn_to_new_branch(const struct checkout_opts *opts)

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

git add shows an example of good writing, follow it.

---
 Documentation/git-restore.txt | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

index 1ab2e40ea9..d7bf016bba 100644
@@ -8,8 +8,8 @@ git-restore - Restore working tree files
 SYNOPSIS
 --------
 [verse]
-'git restore' [<options>] [--source=<tree>] [--staged] [--worktree] <pathspec>...
-'git restore' (-p|--patch) [<options>] [--source=<tree>] [--staged] [--worktree] [<pathspec>...]
+'git restore' [<options>] [--source=<tree>] [--staged] [--worktree] [--] <pathspec>...
+'git restore' (-p|--patch) [<options>] [--source=<tree>] [--staged] [--worktree] [--] [<pathspec>...]
 
 DESCRIPTION
 -----------

@@ -113,6 +113,14 @@ in linkgit:git-checkout[1] for details.
 	appear in the `--source` tree are removed, to make them match
 	`<tree>` exactly. The default is no-overlay mode.
 
+\--::
+	Do not interpret any more arguments as options.
+
+<pathspec>...::
+	Limits the paths affected by the operation.
++
+For more details, see the 'pathspec' entry in linkgit:gitglossary[7].
+
 EXAMPLES
 --------
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Before this patch, when there were multiple DWIM candidates for remote branch, git decided to try the argument as pathspec instead. I believe that such behavior is a surprise: adding another remote suddenly causes git to discard file contents, because it was unsure which branch to pick. There was an incomplete attempt to prevent that in [3].

I understand that this was never intended:

[1] introduces the unexpected behavior. Before, there was fallback from not-a-ref to pathspec. This is reasonable DWIM. After, there is another fallback from ambiguous-remote to pathspec. I understand that it was kind of copy&paste oversight.

[2] noticed the unexpected behavior but chose to semi-document it instead of forbidding, because the goal of the patch series was focused on something else.

[3] adds die() when there is ambiguity between branch and file. The case of multiple tracking branches is seemingly overlooked.

Change to complain about ambiguity instead of doing unexpected things.

[1] Commit 70c9ac2f ("DWIM "git checkout frotz" to "git checkout -b frotz origin/frotz"" 2009-10-18)
https://public-inbox.org/git/[email protected]/
[2] Commit ad8d5104 ("checkout: add advice for ambiguous "checkout <branch>"", 2018-06-05)

    https://public-inbox.org/git/[email protected]/
[3] Commit be4908f1 ("checkout: disambiguate dwim tracking branches and local files", 2018-11-13)
    https://public-inbox.org/git/[email protected]/

---
 builtin/checkout.c       | 56 ++++++++++++++++++----------------------
 t/t2024-checkout-dwim.sh | 28 ++++++++++++++++++--
 2 files changed, 51 insertions(+), 33 deletions(-)

index e1b9df1543..b847695d2b 100644
@@ -1115,10 +1115,10 @@ static void setup_new_branch_info_and_source_tree(
 
 static const char *parse_remote_branch(const char *arg,
 				       struct object_id *rev,
-				       int could_be_checkout_paths,
-				       int *dwim_remotes_matched)
+				       int could_be_checkout_paths)
 {
-	const char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);
+	int num_matches = 0;
+	const char *remote = unique_tracking_name(arg, rev, &num_matches);
 	
 	if (remote && could_be_checkout_paths) {
 		die(_("'%s' could be both a local file and a tracking branch.\n"

@@ -1126,6 +1126,22 @@ static const char *parse_remote_branch(const char *arg,
 		    arg);
 	}
 
+	if (!remote && num_matches > 1) {
+	    if (advice_checkout_ambiguous_remote_branch_name) {
+		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+			     "you can do so by fully qualifying the name with the --track option:\n"
+			     "\n"
+			     "    git checkout --track origin/<name>\n"
+			     "\n"
+			     "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+			     "one remote, e.g. the 'origin' remote, consider setting\n"
+			     "checkout.defaultRemote=origin in your config."));
+	    }
+
+	    die(_("'%s' matched multiple (%d) remote tracking branches"),
+		arg, num_matches);
+	}
+
 	return remote;
 }
 

@@ -1133,8 +1149,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 				int dwim_new_local_branch_ok,
 				struct branch_info *new_branch_info,
 				struct checkout_opts *opts,
-				struct object_id *rev,
-				int *dwim_remotes_matched)
+				struct object_id *rev)
 {
 	const char **new_branch = &opts->new_branch;
 	int argcount = 0;

@@ -1240,8 +1255,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 
 		if (recover_with_dwim) {
 			const char *remote = parse_remote_branch(arg, rev,
-								 could_be_checkout_paths,
-								 dwim_remotes_matched);
+								 could_be_checkout_paths);
 			if (remote) {
 				*new_branch = arg;
 				arg = remote;

@@ -1505,7 +1519,6 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			 const char * const usagestr[])
 {
 	struct branch_info new_branch_info;
-	int dwim_remotes_matched = 0;
 	int parseopt_flags = 0;
 
 	memset(&new_branch_info, 0, sizeof(new_branch_info));

@@ -1613,8 +1626,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			opts->track == BRANCH_TRACK_UNSPECIFIED &&
 			!opts->new_branch;
 		int n = parse_branchname_arg(argc, argv, dwim_ok,
-					     &new_branch_info, opts, &rev,
-					     &dwim_remotes_matched);
+					     &new_branch_info, opts, &rev);
 		argv += n;
 		argc -= n;
 	} else if (!opts->accept_ref && opts->from_treeish) {

@@ -1672,28 +1684,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 	}
 
 	UNLEAK(opts);
-	if (opts->patch_mode || opts->pathspec.nr) {
-		int ret = checkout_paths(opts, new_branch_info.name);
-		if (ret && dwim_remotes_matched > 1 &&
-		    advice_checkout_ambiguous_remote_branch_name)
-			advise(_("'%s' matched more than one remote tracking branch.\n"
-				 "We found %d remotes with a reference that matched. So we fell back\n"
-				 "on trying to resolve the argument as a path, but failed there too!\n"
-				 "\n"
-				 "If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
-				 "you can do so by fully qualifying the name with the --track option:\n"
-				 "\n"
-				 "    git checkout --track origin/<name>\n"
-				 "\n"
-				 "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
-				 "one remote, e.g. the 'origin' remote, consider setting\n"
-				 "checkout.defaultRemote=origin in your config."),
-			       argv[0],
-			       dwim_remotes_matched);
-		return ret;
-	} else {
+	if (opts->patch_mode || opts->pathspec.nr)
+		return checkout_paths(opts, new_branch_info.name);
+	else
 		return checkout_branch(opts, &new_branch_info);
-	}
 }
 
 int cmd_checkout(int argc, const char **argv, const char *prefix)

index fa0718c730..c35d67b697 100755
@@ -37,7 +37,9 @@ test_expect_success 'setup' '
 		git checkout -b foo &&
 		test_commit a_foo &&
 		git checkout -b bar &&
-		test_commit a_bar
+		test_commit a_bar &&
+		git checkout -b ambiguous_branch_and_file &&
+		test_commit a_ambiguous_branch_and_file
 	) &&
 	git init repo_b &&
 	(

@@ -46,7 +48,9 @@ test_expect_success 'setup' '
 		git checkout -b foo &&
 		test_commit b_foo &&
 		git checkout -b baz &&
-		test_commit b_baz
+		test_commit b_baz &&
+		git checkout -b ambiguous_branch_and_file &&
+		test_commit b_ambiguous_branch_and_file
 	) &&
 	git remote add repo_a repo_a &&
 	git remote add repo_b repo_b &&

@@ -75,6 +79,26 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '
 	test_branch master
 '
 
+test_expect_success 'when arg matches multiple remotes, do not fallback to interpreting as pathspec' '
+	# create a file with name matching remote branch name
+	git checkout -b t_ambiguous_branch_and_file &&
+	>ambiguous_branch_and_file &&
+	git add ambiguous_branch_and_file &&
+	git commit -m "ambiguous_branch_and_file" &&
+
+	# modify file to verify that it will not be touched by checkout
+	test_when_finished "git checkout -- ambiguous_branch_and_file" &&
+	echo "file contents" >ambiguous_branch_and_file &&
+	cp ambiguous_branch_and_file expect &&
+
+	test_must_fail git checkout ambiguous_branch_and_file 2>err &&
+
+	test_i18ngrep "matched multiple (2) remote tracking branches" err &&
+	
+	# file must not be altered
+	test_cmp expect ambiguous_branch_and_file
+'
+
 test_expect_success 'checkout of branch from multiple remotes fails with advice' '
 	git checkout -B master &&
 	test_might_fail git branch -D foo &&

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Make it easier to understand which branches handle which cases.

Drop obfuscated variable has_dash_dash which also took opts->accept_pathspec into account, making it pretty hard to reason about code, especially when used together with argc and opts->accept_pathspec here:

	if (!(argc == 1 && !has_dash_dash) &&
		!(argc == 2 && has_dash_dash) &&
		opts->accept_pathspec)
		recover_with_dwim = 0;

Avoid double-negation in the code mentioned above ("it is not OK to proceed if it's not one of those cases").

Avoid hard-to-understand condition opts->accept_pathspec in the code mentioned above.

There are some minor die() message changes for:
git switch <commit> <unexpected>
git switch <commit> -- <unexpected>

---
 builtin/checkout.c | 74 ++++++++++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 29 deletions(-)

index 6072f7cef7..aa4ff14ec2 100644
@@ -1153,9 +1153,8 @@ static int parse_branchname_arg(int argc, const char **argv,
 {
 	const char **new_branch = &opts->new_branch;
 	const char *arg;
-	int dash_dash_pos;
-	int has_dash_dash = 0, expect_commit_only = 0;
-	int i;
+	int dash_dash_pos, i;
+	int recover_with_dwim, expect_commit_only;
 
 	/*
 	 * Resolve ambiguity where argv[0] may be <pathspec> or <commit>.

@@ -1174,15 +1173,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 	 *    do that and proceed with (2)(3).
 	 * 5) Otherwise, let caller proceed with <pathspec> interpretation.
 	 */
-	if (!argc)
-		return 0;
-
-	if (!opts->accept_pathspec) {
-		if (argc > 1)
-			die(_("only one reference expected"));
-		has_dash_dash = 1; /* helps disambiguate */
-	}
-
+	 
 	arg = argv[0];
 	dash_dash_pos = -1;
 	for (i = 0; i < argc; i++) {

@@ -1192,17 +1183,49 @@ static int parse_branchname_arg(int argc, const char **argv,
 		}
 	}
 
-	if (opts->accept_pathspec) {
-	    if (dash_dash_pos == 0)
-		    return 1;
-	    else if (dash_dash_pos == 1)
-		    has_dash_dash = 1;
-	    else if (dash_dash_pos >= 2)
-		    die(_("only one reference expected, %d given."), dash_dash_pos);
-	}
+	if (dash_dash_pos == -1) {
+		if (argc == 0) {
+			/* 'git checkout/switch/restore' */
+			return 0;
+		} else if (argc == 1) {
+			/* 'git checkout/switch/restore <something>' */
+			recover_with_dwim = dwim_new_local_branch_ok;
+		} else if (!opts->accept_pathspec) {
+			/* 'git switch <commit> <unexpected> [...]' */
+			die(_("only one reference expected, %d given."), argc);
+		} else {
+			/* 'git checkout/restore <something> <pathspec> [...]' */
+			recover_with_dwim = 0;
+		}
+
+		/*
+		 * Absence of '--' leaves <pathspec>/<commit> ambiguity.
+		 * Try to resolve it with additional knowledge about pathspec args.
+		 */
+		expect_commit_only = !opts->accept_pathspec;
+	} else if (dash_dash_pos == 0) {
+		/* 'git checkout/switch/restore -- [...]' */
+		return 1;  /* Eat '--' */
+	} else if (dash_dash_pos == 1) {
+		if (!opts->accept_pathspec) {
+			/* 'git switch <commit> -- [...]' */
+			die(_("incompatible with pathspec arguments"));
+		}
+
+		if (argc == 2) {
+			/* 'git checkout/restore <commit> --' */
+			recover_with_dwim = dwim_new_local_branch_ok;
+		} else {
+			/* 'git checkout/restore <commit> -- <pathspec> [...]' */
+			recover_with_dwim = 0;
+		}
 
-	if (has_dash_dash)
-	    expect_commit_only = 1;
+		/* Presence of '--' makes it certain that arg is <commit> */
+		expect_commit_only = 1;
+	} else {
+		/* 'git checkout/switch/restore <commit> <unxpected> [...] -- [...]' */
+		die(_("only one reference expected, %d given."), dash_dash_pos);
+	}
 
 	opts->count_checkout_paths = !opts->quiet && !expect_commit_only;
 

@@ -1210,19 +1233,12 @@ static int parse_branchname_arg(int argc, const char **argv,
 		arg = "@{-1}";
 
 	if (get_oid_mb(arg, rev)) {
-		int recover_with_dwim = dwim_new_local_branch_ok;
-
 		int could_be_checkout_paths = !expect_commit_only &&
 			check_filename(opts->prefix, arg);
 
 		if (!expect_commit_only && !no_wildcard(arg))
 			recover_with_dwim = 0;
 
-		if (!(argc == 1 && !has_dash_dash) &&
-		    !(argc == 2 && has_dash_dash) &&
-		    opts->accept_pathspec)
-			recover_with_dwim = 0;
-
 		if (recover_with_dwim) {
 			const char *remote = parse_remote_branch(arg, rev,
 								 could_be_checkout_paths);

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

After working on parse_branchname_arg() I think that these cases are worth testing.

---
 t/t2024-checkout-dwim.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

index c35d67b697..fd993bf45d 100755
@@ -156,6 +156,33 @@ test_expect_success 'checkout of branch from a single remote succeeds #2' '
 	test_branch_upstream baz repo_b baz
 '
 
+test_expect_success 'checkout of branch from a single remote succeeds with --' '
+	git checkout -B master &&
+	test_might_fail git branch -D baz &&
+
+	git checkout baz -- &&
+	status_uno_is_clean &&
+	test_branch baz &&
+	test_cmp_rev remotes/other_b/baz HEAD &&
+	test_branch_upstream baz repo_b baz
+'
+
+test_expect_success 'dont DWIM with pathspec #1' '
+	git checkout -B master &&
+	test_might_fail git branch -D baz &&
+
+	test_must_fail git checkout baz nonExistingFile 2>err &&
+	test_i18ngrep "did not match any file(s) known to git" err
+'
+
+test_expect_success 'dont DWIM with pathspec #2' '
+	git checkout -B master &&
+	test_might_fail git branch -D baz &&
+
+	test_must_fail git checkout baz -- nonExistingFile 2>err &&
+	test_i18ngrep "fatal: invalid reference: baz" err
+'
+
 test_expect_success '--no-guess suppresses branch auto-vivification' '
 	git checkout -B master &&
 	status_uno_is_clean &&

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

These parts repeat git documentation:
... if <something> is A...B <...>
... remote named in checkout.defaultRemote ...

Some parts repeat the code below. With next patch, code will be easier to understand, so this is no longer needed.

This is a separate patch to reduce the amount of diffs in next patch.

---
 builtin/checkout.c | 86 +++++++++++-----------------------------------
 1 file changed, 21 insertions(+), 65 deletions(-)

index bd0efa9140..6072f7cef7 100644
@@ -1158,45 +1158,21 @@ static int parse_branchname_arg(int argc, const char **argv,
 	int i;
 
 	/*
-	 * case 1: git checkout <ref> -- [<paths>]
-	 *
-	 *   <ref> must be a valid tree, everything after the '--' must be
-	 *   a path.
-	 *
-	 * case 2: git checkout -- [<paths>]
-	 *
-	 *   everything after the '--' must be paths.
-	 *
-	 * case 3: git checkout <something> [--]
-	 *
-	 *   (a) If <something> is a commit, that is to
-	 *       switch to the branch or detach HEAD at it.  As a special case,
-	 *       if <something> is A...B (missing A or B means HEAD but you can
-	 *       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 (or if the branch exists on the
-	 *       remote named in checkout.defaultRemote), 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>
-	 *
-	 *   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.
-	 *
+	 * Resolve ambiguity where argv[0] may be <pathspec> or <commit>.
+	 * High-level approach is:
+	 * 1) Use various things to reduce ambiguity, examples:
+	 *    * '--' is present
+	 *    * command doesn't accept <pathspec>
+	 *    * additional options like '-b' were given
+	 * 2) If ambiguous and matches both existing <commit> and existing
+	 *    file, complain. However, in 1-argument 'git checkout <arg>'
+	 *    treat as <commit> to avoid annoying users.
+	 * 3) Otherwise, if it matches some existing <commit>, treat as
+	 *    <commit>.
+	 * 4) Otherwise, if it matches a remote branch, and it's considered
+	 *    reasonable to DWIM to create a local branch from remote branch,
+	 *    do that and proceed with (2)(3).
+	 * 5) Otherwise, let caller proceed with <pathspec> interpretation.
 	 */
 	if (!argc)
 		return 0;

@@ -1218,9 +1194,9 @@ static int parse_branchname_arg(int argc, const char **argv,
 
 	if (opts->accept_pathspec) {
 	    if (dash_dash_pos == 0)
-		    return 1; /* case (2) */
+		    return 1;
 	    else if (dash_dash_pos == 1)
-		    has_dash_dash = 1; /* case (3) or (1) */
+		    has_dash_dash = 1;
 	    else if (dash_dash_pos >= 2)
 		    die(_("only one reference expected, %d given."), dash_dash_pos);
 	}

@@ -1234,14 +1210,6 @@ static int parse_branchname_arg(int argc, const char **argv,
 		arg = "@{-1}";
 
 	if (get_oid_mb(arg, rev)) {
-		/*
-		 * Either case (3) or (4), with <something> not being
-		 * a commit, or an attempt to use case (1) with an
-		 * invalid ref.
-		 *
-		 * It's likely an error, but we need to find out if
-		 * we should auto-create the branch, case (3).(b).
-		 */
 		int recover_with_dwim = dwim_new_local_branch_ok;
 
 		int could_be_checkout_paths = !expect_commit_only &&

@@ -1250,10 +1218,6 @@ static int parse_branchname_arg(int argc, const char **argv,
 		if (!expect_commit_only && !no_wildcard(arg))
 			recover_with_dwim = 0;
 
-		/*
-		 * Accept "git checkout foo", "git checkout foo --"
-		 * and "git switch foo" as candidates for dwim.
-		 */
 		if (!(argc == 1 && !has_dash_dash) &&
 		    !(argc == 2 && has_dash_dash) &&
 		    opts->accept_pathspec)

@@ -1265,7 +1229,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 			if (remote) {
 				*new_branch = arg;
 				arg = remote;
-				/* DWIMmed to create local branch, case (3).(b) */
+				/* DWIMmed to create local branch */
 			} else {
 				recover_with_dwim = 0;
 			}

@@ -1280,19 +1244,11 @@ static int parse_branchname_arg(int argc, const char **argv,
 
 	setup_new_branch_info_and_source_tree(new_branch_info, opts, rev, arg);
 
-	if (!opts->source_tree)                   /* case (1): want a tree */
+	if (!opts->source_tree)
 		die(_("reference is not a tree: %s"), arg);
 
-	if (!expect_commit_only) {	/* case (3).(d) -> (1) */
-		/*
-		 * Do not complain the most common case
-		 *	git checkout branch
-		 * even if there happen to be a file called 'branch';
-		 * it would be extremely annoying.
-		 */
-		if (argc > 1)
-			verify_non_filename(opts->prefix, arg);
-	}
+	if (!expect_commit_only && argc > 1)
+		verify_non_filename(opts->prefix, arg);
 
 	return (dash_dash_pos == 1) ? 2 : 1;
 }

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

This is done for the next commit to avoid crazy 7x tab code padding.

---
 builtin/checkout.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

index 3634a3dac1..e1b9df1543 100644
@@ -1113,6 +1113,22 @@ static void setup_new_branch_info_and_source_tree(
 	}
 }
 
+static const char *parse_remote_branch(const char *arg,
+				       struct object_id *rev,
+				       int could_be_checkout_paths,
+				       int *dwim_remotes_matched)
+{
+	const char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);
+	
+	if (remote && 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);
+	}
+
+	return remote;
+}
+
 static int parse_branchname_arg(int argc, const char **argv,
 				int dwim_new_local_branch_ok,
 				struct branch_info *new_branch_info,

@@ -1223,13 +1239,10 @@ static int parse_branchname_arg(int argc, const char **argv,
 			recover_with_dwim = 0;
 
 		if (recover_with_dwim) {
-			const char *remote = unique_tracking_name(arg, rev,
-								  dwim_remotes_matched);
+			const char *remote = parse_remote_branch(arg, rev,
+								 could_be_checkout_paths,
+								 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) */

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Decisions taken for simplicity:
1) For now, --pathspec-from-file is declared incompatible with --patch, even when <file> is not stdin. Such use case it not really expected.
2) It is not allowed to pass pathspec in both args and file.

you must specify path(s) to restore block was moved down to be able to test for pathspec.nr instead, because testing for argc is no longer correct.

git switch does not support the new options because it doesn't expect <pathspec> arguments.

---
 Documentation/git-checkout.txt    |  15 ++++
 Documentation/git-restore.txt     |  14 +++
 builtin/checkout.c                |  33 +++++--
 t/t2026-checkout-pathspec-file.sh |  61 +++++++++++++
 t/t2072-restore-pathspec-file.sh  | 139 ++++++++++++++++++++++++++++++
 t/t9902-completion.sh             |   2 +
 6 files changed, 259 insertions(+), 5 deletions(-)
 create mode 100755 t/t2026-checkout-pathspec-file.sh
 create mode 100755 t/t2072-restore-pathspec-file.sh

index 93124f3ad9..ffe3c1bff2 100644
@@ -13,6 +13,7 @@ SYNOPSIS
 '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>] [--] <pathspec>...
+'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] --pathspec-from-file=<file> [--pathspec-file-nul]
 'git checkout' (-p|--patch) [<tree-ish>] [--] [<pathspec>...]
 
 DESCRIPTION

@@ -79,6 +80,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' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <pathspec>...::
+'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] --pathspec-from-file=<file> [--pathspec-file-nul]::
 
 	Overwrite the contents of the files that match the pathspec.
 	When the `<tree-ish>` (most often a commit) is not given, 

@@ -306,6 +308,19 @@ Note that this option uses the no overlay mode by default (see also
 	working tree, but not in `<tree-ish>` are removed, to make them
 	match `<tree-ish>` exactly.
 
+--pathspec-from-file=<file>::
+	Pathspec is passed in `<file>` instead of commandline args. If
+	`<file>` is exactly `-` then standard input is used. Pathspec
+	elements are separated by LF or CR/LF. Pathspec elements can be
+	quoted as explained for the configuration variable `core.quotePath`
+	(see linkgit:git-config[1]). See also `--pathspec-file-nul` and
+	global `--literal-pathspecs`.
+
+--pathspec-file-nul::
+	Only meaningful with `--pathspec-from-file`. Pathspec elements are
+	separated with NUL character and all other characters are taken
+	literally (including newlines and quotes).
+
 <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

index d7bf016bba..5bf60d4943 100644
@@ -9,6 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git restore' [<options>] [--source=<tree>] [--staged] [--worktree] [--] <pathspec>...
+'git restore' [<options>] [--source=<tree>] [--staged] [--worktree] --pathspec-from-file=<file> [--pathspec-file-nul]
 'git restore' (-p|--patch) [<options>] [--source=<tree>] [--staged] [--worktree] [--] [<pathspec>...]
 
 DESCRIPTION

@@ -113,6 +114,19 @@ in linkgit:git-checkout[1] for details.
 	appear in the `--source` tree are removed, to make them match
 	`<tree>` exactly. The default is no-overlay mode.
 
+--pathspec-from-file=<file>::
+	Pathspec is passed in `<file>` instead of commandline args. If
+	`<file>` is exactly `-` then standard input is used. Pathspec
+	elements are separated by LF or CR/LF. Pathspec elements can be
+	quoted as explained for the configuration variable `core.quotePath`
+	(see linkgit:git-config[1]). See also `--pathspec-file-nul` and
+	global `--literal-pathspecs`.
+
+--pathspec-file-nul::
+	Only meaningful with `--pathspec-from-file`. Pathspec elements are
+	separated with NUL character and all other characters are taken
+	literally (including newlines and quotes).
+
 \--::
 	Do not interpret any more arguments as options.
 

index aa4ff14ec2..b02425f49b 100644
@@ -70,6 +70,8 @@ struct checkout_opts {
 	int checkout_worktree;
 	const char *ignore_unmerged_opt;
 	int ignore_unmerged;
+	int pathspec_file_nul;
+	const char *pathspec_from_file;
 
 	const char *new_branch;
 	const char *new_branch_force;

@@ -1202,7 +1204,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 		 * Absence of '--' leaves <pathspec>/<commit> ambiguity.
 		 * Try to resolve it with additional knowledge about pathspec args.
 		 */
-		expect_commit_only = !opts->accept_pathspec;
+		expect_commit_only = !opts->accept_pathspec || opts->pathspec_from_file;
 	} else if (dash_dash_pos == 0) {
 		/* 'git checkout/switch/restore -- [...]' */
 		return 1;  /* Eat '--' */

@@ -1476,6 +1478,8 @@ static struct option *add_checkout_path_options(struct checkout_opts *opts,
 		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_PATHSPEC_FROM_FILE(&opts->pathspec_from_file),
+		OPT_PATHSPEC_FILE_NUL(&opts->pathspec_file_nul),
 		OPT_END()
 	};
 	struct option *newopts = parse_options_concat(prevopts, options);

@@ -1612,10 +1616,6 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			die(_("reference is not a tree: %s"), opts->from_treeish);
 	}
 
-	if (opts->accept_pathspec && !opts->empty_pathspec_ok && !argc &&
-	    !opts->patch_mode)	/* patch mode is special */
-		die(_("you must specify path(s) to restore"));
-
 	if (argc) {
 		parse_pathspec(&opts->pathspec, 0,
 			       opts->patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0,

@@ -1635,10 +1635,33 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 		if (opts->force_detach)
 			die(_("git checkout: --detach does not take a path argument '%s'"),
 			    argv[0]);
+	}
+
+	if (opts->pathspec_from_file) {
+		if (opts->pathspec.nr)
+			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
+
+		if (opts->force_detach)
+			die(_("--pathspec-from-file is incompatible with --detach"));
 
+		if (opts->patch_mode)
+			die(_("--pathspec-from-file is incompatible with --patch"));
+
+		parse_pathspec_file(&opts->pathspec, 0,
+				    0,
+				    prefix, opts->pathspec_from_file, opts->pathspec_file_nul);
+	} else if (opts->pathspec_file_nul) {
+		die(_("--pathspec-file-nul requires --pathspec-from-file"));
+	}
+
+	if (opts->pathspec.nr) {
 		if (1 < !!opts->writeout_stage + !!opts->force + !!opts->merge)
 			die(_("git checkout: --ours/--theirs, --force and --merge are incompatible when\n"
 			      "checking out of the index."));
+	} else {
+		if (opts->accept_pathspec && !opts->empty_pathspec_ok &&
+		    !opts->patch_mode)	/* patch mode is special */
+			die(_("you must specify path(s) to restore"));
 	}
 
 	if (opts->new_branch) {

index ec3eccfd3d..93877ba9cd 100755
@@ -1438,6 +1438,8 @@ test_expect_success 'double dash "git checkout"' '
 	--no-guess Z
 	--no-... Z
 	--overlay Z
+	--pathspec-file-nul Z
+	--pathspec-from-file=Z
 	EOF
 '
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

has_dash_dash unexpectedly takes opts->accept_pathspec into account. While this may sound clever at first sight, it becomes pretty hard to reason (and not be a victim) about code, especially in combination with argc here:

	if (!(argc == 1 && !has_dash_dash) &&
	    !(argc == 2 && has_dash_dash) &&
	    opts->accept_pathspec)
		recover_with_dwim = 0;

Introduce a new non-obfuscated variable to reduce the amount of diffs in next patch.

This should not change behavior in any way.

---
 builtin/checkout.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

index f35fe2cc26..bd0efa9140 100644
@@ -1154,7 +1154,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 	const char **new_branch = &opts->new_branch;
 	const char *arg;
 	int dash_dash_pos;
-	int has_dash_dash = 0;
+	int has_dash_dash = 0, expect_commit_only = 0;
 	int i;
 
 	/*

@@ -1225,7 +1225,10 @@ static int parse_branchname_arg(int argc, const char **argv,
 		    die(_("only one reference expected, %d given."), dash_dash_pos);
 	}
 
-	opts->count_checkout_paths = !opts->quiet && !has_dash_dash;
+	if (has_dash_dash)
+	    expect_commit_only = 1;
+
+	opts->count_checkout_paths = !opts->quiet && !expect_commit_only;
 
 	if (!strcmp(arg, "-"))
 		arg = "@{-1}";

@@ -1241,10 +1244,10 @@ static int parse_branchname_arg(int argc, const char **argv,
 		 */
 		int recover_with_dwim = dwim_new_local_branch_ok;
 
-		int could_be_checkout_paths = !has_dash_dash &&
+		int could_be_checkout_paths = !expect_commit_only &&
 			check_filename(opts->prefix, arg);
 
-		if (!has_dash_dash && !no_wildcard(arg))
+		if (!expect_commit_only && !no_wildcard(arg))
 			recover_with_dwim = 0;
 
 		/*

@@ -1269,7 +1272,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 		}
 
 		if (!recover_with_dwim) {
-			if (has_dash_dash)
+			if (expect_commit_only)
 				die(_("invalid reference: %s"), arg);
 			return 0;
 		}

@@ -1280,7 +1283,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 	if (!opts->source_tree)                   /* case (1): want a tree */
 		die(_("reference is not a tree: %s"), arg);
 
-	if (!has_dash_dash) {	/* case (3).(d) -> (1) */
+	if (!expect_commit_only) {	/* case (3).(d) -> (1) */
 		/*
 		 * Do not complain the most common case
 		 *	git checkout branch

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Decisions taken for simplicity:
1) For now, --pathspec-from-file is declared incompatible with --interactive/--patch/--edit, even when <file> is not stdin. Such use case it not really expected. Also, it would require changes to interactive_add() and edit_patch().
2) It is not allowed to pass pathspec in both args and file.

---
 Documentation/git-add.txt    | 16 +++++++++++-
 builtin/add.c                | 30 +++++++++++++++++++---
 t/t3704-add-pathspec-file.sh | 49 ++++++++++++++++++++++++++++++++++++
 3 files changed, 90 insertions(+), 5 deletions(-)
 create mode 100755 t/t3704-add-pathspec-file.sh

index 8b0e4c7fa8..be5e3ac54b 100644
@@ -11,7 +11,8 @@ SYNOPSIS
 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
 	  [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
 	  [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize]
-	  [--chmod=(+|-)x] [--] [<pathspec>...]
+	  [--chmod=(+|-)x] [--pathspec-from-file=<file> [--pathspec-file-nul]]
+	  [--] [<pathspec>...]
 
 DESCRIPTION
 -----------

@@ -187,6 +188,19 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
 	bit is only changed in the index, the files on disk are left
 	unchanged.
 
+--pathspec-from-file=<file>::
+	Pathspec is passed in `<file>` instead of commandline args. If
+	`<file>` is exactly `-` then standard input is used. Pathspec
+	elements are separated by LF or CR/LF. Pathspec elements can be
+	quoted as explained for the configuration variable `core.quotePath`
+	(see linkgit:git-config[1]). See also `--pathspec-file-nul` and
+	global `--literal-pathspecs`.
+
+--pathspec-file-nul::
+	Only meaningful with `--pathspec-from-file`. Pathspec elements are
+	separated with NUL character and all other characters are taken
+	literally (including newlines and quotes).
+
 \--::
 	This option can be used to separate command-line options from
 	the list of files, (useful when filenames might be mistaken

index 3d1791dd82..7c21ad492b 100644
@@ -29,6 +29,8 @@ static const char * const builtin_add_usage[] = {
 static int patch_interactive, add_interactive, edit_interactive;
 static int take_worktree_changes;
 static int add_renormalize;
+static int pathspec_file_nul;
+static const char *pathspec_from_file;
 
 struct update_callback_data {
 	int flags;

@@ -320,6 +322,8 @@ static struct option builtin_add_options[] = {
 		   N_("override the executable bit of the listed files")),
 	OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo,
 			N_("warn when adding an embedded repository")),
+	OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
+	OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
 	OPT_END(),
 };
 

@@ -414,11 +418,17 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);
 	if (patch_interactive)
 		add_interactive = 1;
-	if (add_interactive)
+	if (add_interactive) {
+		if (pathspec_from_file)
+			die(_("--pathspec-from-file is incompatible with --interactive/--patch"));
 		exit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive));
+	}
 
-	if (edit_interactive)
+	if (edit_interactive) {
+		if (pathspec_from_file)
+			die(_("--pathspec-from-file is incompatible with --edit"));
 		return(edit_patch(argc, argv, prefix));
+	}
 	argc--;
 	argv++;
 

@@ -451,13 +461,25 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
 
-	if (require_pathspec && argc == 0) {
+	if (pathspec_from_file) {
+		if (pathspec.nr)
+			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
+
+		parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
+				    PATHSPEC_PREFER_FULL |
+				    PATHSPEC_SYMLINK_LEADING_PATH,
+				    prefix, pathspec_from_file, pathspec_file_nul);
+	} else if (pathspec_file_nul) {
+		die(_("--pathspec-file-nul requires --pathspec-from-file"));
+	}
+
+	if (require_pathspec && pathspec.nr == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
 		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
 		return 0;
 	}
 
-	if (!take_worktree_changes && addremove_explicit < 0 && argc)
+	if (!take_worktree_changes && addremove_explicit < 0 && pathspec.nr)
 		/* Turn "git add pathspec..." to "git add -A pathspec..." */
 		addremove = 1;
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

It was added in [1]. I understand that the duplicate change was not intentional and comes from an oversight.

Also, in explanation, there was only one section for two synopsis entries.

Fix both problems by removing duplicate synopsis.

<paths> vs <pathspec> is resolved in next patch.

[1] Commit b59698ae ("checkout doc: clarify command line args for "checkout paths" mode" 2017-10-11)

---
 Documentation/git-checkout.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index cf3cac0a2b..2011fdbb1d 100644
@@ -13,7 +13,6 @@ SYNOPSIS
 '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>...]
 
 DESCRIPTION

@@ -79,7 +78,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' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...::
 
 	Overwrite paths in the working tree by replacing with the
 	contents in the index or in the `<tree-ish>` (most often a

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

git add shows an example of good writing, follow it.

---
 Documentation/git-checkout.txt | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

index d47046e050..93124f3ad9 100644
@@ -12,13 +12,13 @@ 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' (-p|--patch) [<tree-ish>] [--] [<paths>...]
+'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <pathspec>...
+'git checkout' (-p|--patch) [<tree-ish>] [--] [<pathspec>...]
 
 DESCRIPTION
 -----------
 Updates files in the working tree to match the version in the index
-or the specified tree.  If no paths are given, 'git checkout' will
+or the specified tree.  If no pathspec was given, 'git checkout' will
 also update `HEAD` to set the specified branch as the current
 branch.
 

@@ -78,13 +78,13 @@ 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' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...::
+'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<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
-	commit).  When a `<tree-ish>` is given, the paths that
-	match the `<pathspec>` are updated both in the index and in
-	the working tree.
+	Overwrite the contents of the files that match the pathspec.
+	When the `<tree-ish>` (most often a commit) is not given, 
+	overwrite working tree with the contents in the index.
+	When the `<tree-ish>` is given, overwrite both the index and
+	the working tree with the contents at the `<tree-ish>`.
 +
 The index may contain unmerged entries because of a previous failed merge.
 By default, if you try to check out such an entry from the index, the

@@ -336,7 +336,13 @@ leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 	Tree to checkout from (when paths are given). If not specified,
 	the index will be used.
 
+\--::
+	Do not interpret any more arguments as options.
 
+<pathspec>...::
+	Limits the paths affected by the operation.
++
+For more details, see the 'pathspec' entry in linkgit:gitglossary[7].
 
 DETACHED HEAD
 -------------

gitgitgadget

Hi Alexandr

Thanks for fixing this. If you want to credit me then I think
Reported-by: would be more appropriate as I didn't write this patch. Also I tend to use [email protected] for footers as it's a portable email address (I should add a mailmap entry...). It would be nice to have tests for the various error conditions at some point.

Best Wishes

Phillip

From: Alexandr Miloslavskiy <[email protected]>

---
 t/t7526-commit-pathspec-file.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

index 4e592f7472..68920e8ff9 100755
@@ -61,4 +61,28 @@ test_expect_success 'only touches what was listed' '
 	verify_expect
 '
 
+test_expect_success 'error conditions' '
+	restore_checkpoint &&
+	echo fileA.t >list &&
+	>empty_list &&
+
+	test_must_fail git commit --pathspec-from-file=- --interactive -m "Commit" <list 2>err &&
+	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-interactive/\-\-patch" err &&
+
+	test_must_fail git commit --pathspec-from-file=- --patch -m "Commit" <list 2>err &&
+	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-interactive/\-\-patch" err &&
+
+	test_must_fail git commit --pathspec-from-file=- -m "Commit" -- fileA.t <list 2>err &&
+	test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&
+
+	test_must_fail git commit --pathspec-file-nul -m "Commit" 2>err &&
+	test_i18ngrep "\-\-pathspec-file-nul requires \-\-pathspec-from-file" err &&
+
+	test_must_fail git commit --pathspec-from-file=- --include -m "Commit" <empty_list 2>err &&
+	test_i18ngrep "No paths with \-\-include/\-\-only does not make sense." err &&
+
+	test_must_fail git commit --pathspec-from-file=- --only -m "Commit" <empty_list 2>err &&
+	test_i18ngrep "No paths with \-\-include/\-\-only does not make sense." err
+'
+
 test_done

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

I forgot this in my previous patch --pathspec-from-file for git commit [1]. When both --pathspec-from-file and --all were specified, --all took precedence and --pathspec-from-file was ignored. Before --pathspec-from-file was implemented, this case was prevented by this check in parse_and_validate_options() :

    die(_("paths '%s ...' with -a does not make sense"), argv[0]);

It is unfortunate that these two cases are disconnected. This came as result of how the code was laid out before my patches, where pathspec is parsed outside of parse_and_validate_options(). This branch is already full of refactoring patches and I did not dare to go for another one.

Fix by mirroring die() for --pathspec-from-file as well.

[1] Commit e440fc58 ("commit: support the --pathspec-from-file option" 2019-11-19)

---
 builtin/commit.c                | 3 +++
 t/t7526-commit-pathspec-file.sh | 3 +++
 2 files changed, 6 insertions(+)

index 2db2ad0de4..893a9f29b2 100644
@@ -347,6 +347,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 		if (interactive)
 			die(_("--pathspec-from-file is incompatible with --interactive/--patch"));
 
+		if (all)
+			die(_("--pathspec-from-file with -a does not make sense"));
+
 		if (pathspec.nr)
 			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
 

index 68920e8ff9..ba769e0e5d 100755
@@ -72,6 +72,9 @@ test_expect_success 'error conditions' '
 	test_must_fail git commit --pathspec-from-file=- --patch -m "Commit" <list 2>err &&
 	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-interactive/\-\-patch" err &&
 
+	test_must_fail git commit --pathspec-from-file=- --all -m "Commit" <list 2>err &&
+	test_i18ngrep "\-\-pathspec-from-file with \-a does not make sense" err &&
+
 	test_must_fail git commit --pathspec-from-file=- -m "Commit" -- fileA.t <list 2>err &&
 	test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Some code blocks were moved down to be able to test for pathspec.nr in the next patch. Blocks are moved as is without any changes. This is done as separate patch to reduce the amount of diffs in next patch.

---
 builtin/add.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

index d4686d5218..3d1791dd82 100644
@@ -430,10 +430,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (addremove && take_worktree_changes)
 		die(_("-A and -u are mutually incompatible"));
 
-	if (!take_worktree_changes && addremove_explicit < 0 && argc)
-		/* Turn "git add pathspec..." to "git add -A pathspec..." */
-		addremove = 1;
-
 	if (!show_only && ignore_missing)
 		die(_("Option --ignore-missing can only be used together with --dry-run"));
 

@@ -446,19 +442,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
 
-	flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
-		 (show_only ? ADD_CACHE_PRETEND : 0) |
-		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
-		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
-		 (!(addremove || take_worktree_changes)
-		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
-
-	if (require_pathspec && argc == 0) {
-		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
-		return 0;
-	}
-
 	/*
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.

@@ -468,6 +451,23 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
 
+	if (require_pathspec && argc == 0) {
+		fprintf(stderr, _("Nothing specified, nothing added.\n"));
+		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
+		return 0;
+	}
+
+	if (!take_worktree_changes && addremove_explicit < 0 && argc)
+		/* Turn "git add pathspec..." to "git add -A pathspec..." */
+		addremove = 1;
+
+	flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
+		 (show_only ? ADD_CACHE_PRETEND : 0) |
+		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
+		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
+		 (!(addremove || take_worktree_changes)
+		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
+
 	if (read_cache_preload(&pathspec) < 0)
 		die(_("index file corrupt"));
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Decisions taken for simplicity:
1) For now, --pathspec-from-file is declared incompatible with --interactive/--patch/--edit, even when <file> is not stdin. Such use case it not really expected. Also, it would require changes to interactive_add() and edit_patch().
2) It is not allowed to pass pathspec in both args and file.

---
 Documentation/git-add.txt    | 16 ++++++-
 builtin/add.c                | 30 +++++++++++--
 t/t3704-add-pathspec-file.sh | 86 ++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+), 5 deletions(-)
 create mode 100755 t/t3704-add-pathspec-file.sh

index 8b0e4c7fa8..be5e3ac54b 100644
@@ -11,7 +11,8 @@ SYNOPSIS
 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
 	  [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
 	  [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize]
-	  [--chmod=(+|-)x] [--] [<pathspec>...]
+	  [--chmod=(+|-)x] [--pathspec-from-file=<file> [--pathspec-file-nul]]
+	  [--] [<pathspec>...]
 
 DESCRIPTION
 -----------

@@ -187,6 +188,19 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
 	bit is only changed in the index, the files on disk are left
 	unchanged.
 
+--pathspec-from-file=<file>::
+	Pathspec is passed in `<file>` instead of commandline args. If
+	`<file>` is exactly `-` then standard input is used. Pathspec
+	elements are separated by LF or CR/LF. Pathspec elements can be
+	quoted as explained for the configuration variable `core.quotePath`
+	(see linkgit:git-config[1]). See also `--pathspec-file-nul` and
+	global `--literal-pathspecs`.
+
+--pathspec-file-nul::
+	Only meaningful with `--pathspec-from-file`. Pathspec elements are
+	separated with NUL character and all other characters are taken
+	literally (including newlines and quotes).
+
 \--::
 	This option can be used to separate command-line options from
 	the list of files, (useful when filenames might be mistaken

index 3d1791dd82..7c21ad492b 100644
@@ -29,6 +29,8 @@ static const char * const builtin_add_usage[] = {
 static int patch_interactive, add_interactive, edit_interactive;
 static int take_worktree_changes;
 static int add_renormalize;
+static int pathspec_file_nul;
+static const char *pathspec_from_file;
 
 struct update_callback_data {
 	int flags;

@@ -320,6 +322,8 @@ static struct option builtin_add_options[] = {
 		   N_("override the executable bit of the listed files")),
 	OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo,
 			N_("warn when adding an embedded repository")),
+	OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
+	OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
 	OPT_END(),
 };
 

@@ -414,11 +418,17 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);
 	if (patch_interactive)
 		add_interactive = 1;
-	if (add_interactive)
+	if (add_interactive) {
+		if (pathspec_from_file)
+			die(_("--pathspec-from-file is incompatible with --interactive/--patch"));
 		exit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive));
+	}
 
-	if (edit_interactive)
+	if (edit_interactive) {
+		if (pathspec_from_file)
+			die(_("--pathspec-from-file is incompatible with --edit"));
 		return(edit_patch(argc, argv, prefix));
+	}
 	argc--;
 	argv++;
 

@@ -451,13 +461,25 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
 
-	if (require_pathspec && argc == 0) {
+	if (pathspec_from_file) {
+		if (pathspec.nr)
+			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
+
+		parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
+				    PATHSPEC_PREFER_FULL |
+				    PATHSPEC_SYMLINK_LEADING_PATH,
+				    prefix, pathspec_from_file, pathspec_file_nul);
+	} else if (pathspec_file_nul) {
+		die(_("--pathspec-file-nul requires --pathspec-from-file"));
+	}
+
+	if (require_pathspec && pathspec.nr == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
 		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
 		return 0;
 	}
 
-	if (!take_worktree_changes && addremove_explicit < 0 && argc)
+	if (!take_worktree_changes && addremove_explicit < 0 && pathspec.nr)
 		/* Turn "git add pathspec..." to "git add -A pathspec..." */
 		addremove = 1;
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

In my previous patches, parse_pathspec_file() was tested indirectly by invoking git reset and git commit with properly crafted inputs. This has some disadvantages:
1) A number of tests are copy&pasted for every command where --pathspec-from-file is supported. With just two commands, it wasn't too bad, but I'm going to extend support to many more commands, which would make a handful of low-value tests.
2) Tests are located in suboptimal test packages
3) Tests are indirect

Fix this by testing parse_pathspec_file() directly via a new test helper.

While working on it, I also noticed that quotes testing via "\"file\\101.t\"" was somewhat incorrect: I escaped \ one time while I had to escape it two times! Tests still worked due to " which prevented pathspec from matching files.

Fix this by properly escaping one more time.

---
 Makefile                            |  1 +
 t/helper/test-parse-pathspec-file.c | 34 +++++++++++
 t/helper/test-tool.c                |  1 +
 t/helper/test-tool.h                |  1 +
 t/t0067-parse_pathspec_file.sh      | 89 +++++++++++++++++++++++++++
 t/t7107-reset-pathspec-file.sh      | 94 +++--------------------------
 t/t7526-commit-pathspec-file.sh     | 70 +--------------------
 7 files changed, 136 insertions(+), 154 deletions(-)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh

index b7d7374dac..fd7bcaac9c 100644
@@ -721,6 +721,7 @@ TEST_BUILTINS_OBJS += test-mktemp.o
 TEST_BUILTINS_OBJS += test-oidmap.o
 TEST_BUILTINS_OBJS += test-online-cpus.o
 TEST_BUILTINS_OBJS += test-parse-options.o
+TEST_BUILTINS_OBJS += test-parse-pathspec-file.o
 TEST_BUILTINS_OBJS += test-path-utils.o
 TEST_BUILTINS_OBJS += test-pkt-line.o
 TEST_BUILTINS_OBJS += test-prio-queue.o

new file mode 100644
index 0000000000..e7f525feb9
@@ -0,0 +1,34 @@
+#include "test-tool.h"
+#include "parse-options.h"
+#include "pathspec.h"
+#include "gettext.h"
+
+int cmd__parse_pathspec_file(int argc, const char **argv)
+{
+	struct pathspec pathspec;
+	const char *pathspec_from_file = 0;
+	int pathspec_file_nul = 0, i;
+
+	static const char *const usage[] = {
+		"test-tool parse-pathspec-file --pathspec-from-file [--pathspec-file-nul]",
+		NULL
+	};
+
+	struct option options[] = {
+		OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
+		OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
+		OPT_END()
+	};
+
+	parse_options(argc, argv, 0, options, usage, 0);
+
+	parse_pathspec_file(&pathspec, 0, 0, 0, pathspec_from_file,
+			    pathspec_file_nul);
+
+	for (i = 0; i < pathspec.nr; i++) {
+		printf("%s\n", pathspec.items[i].original);
+	}
+
+	clear_pathspec(&pathspec);
+	return 0;
+}

index f20989d449..c9a232d238 100644
@@ -39,6 +39,7 @@ static struct test_cmd cmds[] = {
 	{ "oidmap", cmd__oidmap },
 	{ "online-cpus", cmd__online_cpus },
 	{ "parse-options", cmd__parse_options },
+	{ "parse-pathspec-file", cmd__parse_pathspec_file },
 	{ "path-utils", cmd__path_utils },
 	{ "pkt-line", cmd__pkt_line },
 	{ "prio-queue", cmd__prio_queue },

index 8ed2af71d1..c8549fd87f 100644
@@ -29,6 +29,7 @@ int cmd__mktemp(int argc, const char **argv);
 int cmd__oidmap(int argc, const char **argv);
 int cmd__online_cpus(int argc, const char **argv);
 int cmd__parse_options(int argc, const char **argv);
+int cmd__parse_pathspec_file(int argc, const char** argv);
 int cmd__path_utils(int argc, const char **argv);
 int cmd__pkt_line(int argc, const char **argv);
 int cmd__prio_queue(int argc, const char **argv);

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

git add shows an example of good writing, follow it.

---
 Documentation/git-checkout.txt | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

index d47046e050..93124f3ad9 100644
@@ -12,13 +12,13 @@ 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' (-p|--patch) [<tree-ish>] [--] [<paths>...]
+'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <pathspec>...
+'git checkout' (-p|--patch) [<tree-ish>] [--] [<pathspec>...]
 
 DESCRIPTION
 -----------
 Updates files in the working tree to match the version in the index
-or the specified tree.  If no paths are given, 'git checkout' will
+or the specified tree.  If no pathspec was given, 'git checkout' will
 also update `HEAD` to set the specified branch as the current
 branch.
 

@@ -78,13 +78,13 @@ 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' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...::
+'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<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
-	commit).  When a `<tree-ish>` is given, the paths that
-	match the `<pathspec>` are updated both in the index and in
-	the working tree.
+	Overwrite the contents of the files that match the pathspec.
+	When the `<tree-ish>` (most often a commit) is not given, 
+	overwrite working tree with the contents in the index.
+	When the `<tree-ish>` is given, overwrite both the index and
+	the working tree with the contents at the `<tree-ish>`.
 +
 The index may contain unmerged entries because of a previous failed merge.
 By default, if you try to check out such an entry from the index, the

@@ -336,7 +336,13 @@ leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 	Tree to checkout from (when paths are given). If not specified,
 	the index will be used.
 
+\--::
+	Do not interpret any more arguments as options.
 
+<pathspec>...::
+	Limits the paths affected by the operation.
++
+For more details, see the 'pathspec' entry in linkgit:gitglossary[7].
 
 DETACHED HEAD
 -------------

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

---
 Documentation/git-checkout.txt | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

index 2011fdbb1d..d47046e050 100644
@@ -95,12 +95,10 @@ 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>...]::
-	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
-	the "diff" output and choose which hunks to use in the
-	result.  See below for the description of `--patch` option.
-
+	This is similar to the previous mode, but lets you use the
+	interactive interface to show the "diff" output and choose which
+	hunks to use in the result.  See below for the description of
+	`--patch` option.
 
 OPTIONS
 -------

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

dash_dash_pos was only calculated under opts->accept_pathspec. This is unexpected to readers and made it harder to reason about the code. Fix this by restoring the expected meaning.

Simplify the code by dropping argcount and useless argc / argv manipulations.

This should not change behavior in any way.

---
 builtin/checkout.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

index b847695d2b..f35fe2cc26 100644
@@ -1152,7 +1152,6 @@ static int parse_branchname_arg(int argc, const char **argv,
 				struct object_id *rev)
 {
 	const char **new_branch = &opts->new_branch;
-	int argcount = 0;
 	const char *arg;
 	int dash_dash_pos;
 	int has_dash_dash = 0;

@@ -1211,17 +1210,21 @@ static int parse_branchname_arg(int argc, const char **argv,
 	arg = argv[0];
 	dash_dash_pos = -1;
 	for (i = 0; i < argc; i++) {
-		if (opts->accept_pathspec && !strcmp(argv[i], "--")) {
+		if (!strcmp(argv[i], "--")) {
 			dash_dash_pos = i;
 			break;
 		}
 	}
-	if (dash_dash_pos == 0)
-		return 1; /* case (2) */
-	else if (dash_dash_pos == 1)
-		has_dash_dash = 1; /* case (3) or (1) */
-	else if (dash_dash_pos >= 2)
-		die(_("only one reference expected, %d given."), dash_dash_pos);
+
+	if (opts->accept_pathspec) {
+	    if (dash_dash_pos == 0)
+		    return 1; /* case (2) */
+	    else if (dash_dash_pos == 1)
+		    has_dash_dash = 1; /* case (3) or (1) */
+	    else if (dash_dash_pos >= 2)
+		    die(_("only one reference expected, %d given."), dash_dash_pos);
+	}
+
 	opts->count_checkout_paths = !opts->quiet && !has_dash_dash;
 
 	if (!strcmp(arg, "-"))

@@ -1268,15 +1271,10 @@ static int parse_branchname_arg(int argc, const char **argv,
 		if (!recover_with_dwim) {
 			if (has_dash_dash)
 				die(_("invalid reference: %s"), arg);
-			return argcount;
+			return 0;
 		}
 	}
 
-	/* we can't end up being in (2) anymore, eat the argument */
-	argcount++;
-	argv++;
-	argc--;
-
 	setup_new_branch_info_and_source_tree(new_branch_info, opts, rev, arg);
 
 	if (!opts->source_tree)                   /* case (1): want a tree */

@@ -1289,15 +1287,11 @@ static int parse_branchname_arg(int argc, const char **argv,
 		 * even if there happen to be a file called 'branch';
 		 * it would be extremely annoying.
 		 */
-		if (argc)
+		if (argc > 1)
 			verify_non_filename(opts->prefix, arg);
-	} else if (opts->accept_pathspec) {
-		argcount++;
-		argv++;
-		argc--;
 	}
 
-	return argcount;
+	return (dash_dash_pos == 1) ? 2 : 1;
 }
 
 static int switch_unborn_to_new_branch(const struct checkout_opts *opts)

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

has_dash_dash unexpectedly takes opts->accept_pathspec into account. While this may sound clever at first sight, it becomes pretty hard to reason (and not be a victim) about code, especially in combination with argc here:

	if (!(argc == 1 && !has_dash_dash) &&
	    !(argc == 2 && has_dash_dash) &&
	    opts->accept_pathspec)
		recover_with_dwim = 0;

Introduce a new non-obfuscated variable to reduce the amount of diffs in next patch.

This should not change behavior in any way.

---
 builtin/checkout.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

index f35fe2cc26..bd0efa9140 100644
@@ -1154,7 +1154,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 	const char **new_branch = &opts->new_branch;
 	const char *arg;
 	int dash_dash_pos;
-	int has_dash_dash = 0;
+	int has_dash_dash = 0, expect_commit_only = 0;
 	int i;
 
 	/*

@@ -1225,7 +1225,10 @@ static int parse_branchname_arg(int argc, const char **argv,
 		    die(_("only one reference expected, %d given."), dash_dash_pos);
 	}
 
-	opts->count_checkout_paths = !opts->quiet && !has_dash_dash;
+	if (has_dash_dash)
+	    expect_commit_only = 1;
+
+	opts->count_checkout_paths = !opts->quiet && !expect_commit_only;
 
 	if (!strcmp(arg, "-"))
 		arg = "@{-1}";

@@ -1241,10 +1244,10 @@ static int parse_branchname_arg(int argc, const char **argv,
 		 */
 		int recover_with_dwim = dwim_new_local_branch_ok;
 
-		int could_be_checkout_paths = !has_dash_dash &&
+		int could_be_checkout_paths = !expect_commit_only &&
 			check_filename(opts->prefix, arg);
 
-		if (!has_dash_dash && !no_wildcard(arg))
+		if (!expect_commit_only && !no_wildcard(arg))
 			recover_with_dwim = 0;
 
 		/*

@@ -1269,7 +1272,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 		}
 
 		if (!recover_with_dwim) {
-			if (has_dash_dash)
+			if (expect_commit_only)
 				die(_("invalid reference: %s"), arg);
 			return 0;
 		}

@@ -1280,7 +1283,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 	if (!opts->source_tree)                   /* case (1): want a tree */
 		die(_("reference is not a tree: %s"), arg);
 
-	if (!has_dash_dash) {	/* case (3).(d) -> (1) */
+	if (!expect_commit_only) {	/* case (3).(d) -> (1) */
 		/*
 		 * Do not complain the most common case
 		 *	git checkout branch

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

After working on parse_branchname_arg() I think that these cases are worth testing.

---
 t/t2024-checkout-dwim.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

index c35d67b697..fd993bf45d 100755
@@ -156,6 +156,33 @@ test_expect_success 'checkout of branch from a single remote succeeds #2' '
 	test_branch_upstream baz repo_b baz
 '
 
+test_expect_success 'checkout of branch from a single remote succeeds with --' '
+	git checkout -B master &&
+	test_might_fail git branch -D baz &&
+
+	git checkout baz -- &&
+	status_uno_is_clean &&
+	test_branch baz &&
+	test_cmp_rev remotes/other_b/baz HEAD &&
+	test_branch_upstream baz repo_b baz
+'
+
+test_expect_success 'dont DWIM with pathspec #1' '
+	git checkout -B master &&
+	test_might_fail git branch -D baz &&
+
+	test_must_fail git checkout baz nonExistingFile 2>err &&
+	test_i18ngrep "did not match any file(s) known to git" err
+'
+
+test_expect_success 'dont DWIM with pathspec #2' '
+	git checkout -B master &&
+	test_might_fail git branch -D baz &&
+
+	test_must_fail git checkout baz -- nonExistingFile 2>err &&
+	test_i18ngrep "fatal: invalid reference: baz" err
+'
+
 test_expect_success '--no-guess suppresses branch auto-vivification' '
 	git checkout -B master &&
 	status_uno_is_clean &&

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

These parts repeat git documentation:
... if <something> is A...B <...>
... remote named in checkout.defaultRemote ...

Some parts repeat the code below. With next patch, code will be easier to understand, so this is no longer needed.

This is a separate patch to reduce the amount of diffs in next patch.

---
 builtin/checkout.c | 86 +++++++++++-----------------------------------
 1 file changed, 21 insertions(+), 65 deletions(-)

index bd0efa9140..6072f7cef7 100644
@@ -1158,45 +1158,21 @@ static int parse_branchname_arg(int argc, const char **argv,
 	int i;
 
 	/*
-	 * case 1: git checkout <ref> -- [<paths>]
-	 *
-	 *   <ref> must be a valid tree, everything after the '--' must be
-	 *   a path.
-	 *
-	 * case 2: git checkout -- [<paths>]
-	 *
-	 *   everything after the '--' must be paths.
-	 *
-	 * case 3: git checkout <something> [--]
-	 *
-	 *   (a) If <something> is a commit, that is to
-	 *       switch to the branch or detach HEAD at it.  As a special case,
-	 *       if <something> is A...B (missing A or B means HEAD but you can
-	 *       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 (or if the branch exists on the
-	 *       remote named in checkout.defaultRemote), 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>
-	 *
-	 *   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.
-	 *
+	 * Resolve ambiguity where argv[0] may be <pathspec> or <commit>.
+	 * High-level approach is:
+	 * 1) Use various things to reduce ambiguity, examples:
+	 *    * '--' is present
+	 *    * command doesn't accept <pathspec>
+	 *    * additional options like '-b' were given
+	 * 2) If ambiguous and matches both existing <commit> and existing
+	 *    file, complain. However, in 1-argument 'git checkout <arg>'
+	 *    treat as <commit> to avoid annoying users.
+	 * 3) Otherwise, if it matches some existing <commit>, treat as
+	 *    <commit>.
+	 * 4) Otherwise, if it matches a remote branch, and it's considered
+	 *    reasonable to DWIM to create a local branch from remote branch,
+	 *    do that and proceed with (2)(3).
+	 * 5) Otherwise, let caller proceed with <pathspec> interpretation.
 	 */
 	if (!argc)
 		return 0;

@@ -1218,9 +1194,9 @@ static int parse_branchname_arg(int argc, const char **argv,
 
 	if (opts->accept_pathspec) {
 	    if (dash_dash_pos == 0)
-		    return 1; /* case (2) */
+		    return 1;
 	    else if (dash_dash_pos == 1)
-		    has_dash_dash = 1; /* case (3) or (1) */
+		    has_dash_dash = 1;
 	    else if (dash_dash_pos >= 2)
 		    die(_("only one reference expected, %d given."), dash_dash_pos);
 	}

@@ -1234,14 +1210,6 @@ static int parse_branchname_arg(int argc, const char **argv,
 		arg = "@{-1}";
 
 	if (get_oid_mb(arg, rev)) {
-		/*
-		 * Either case (3) or (4), with <something> not being
-		 * a commit, or an attempt to use case (1) with an
-		 * invalid ref.
-		 *
-		 * It's likely an error, but we need to find out if
-		 * we should auto-create the branch, case (3).(b).
-		 */
 		int recover_with_dwim = dwim_new_local_branch_ok;
 
 		int could_be_checkout_paths = !expect_commit_only &&

@@ -1250,10 +1218,6 @@ static int parse_branchname_arg(int argc, const char **argv,
 		if (!expect_commit_only && !no_wildcard(arg))
 			recover_with_dwim = 0;
 
-		/*
-		 * Accept "git checkout foo", "git checkout foo --"
-		 * and "git switch foo" as candidates for dwim.
-		 */
 		if (!(argc == 1 && !has_dash_dash) &&
 		    !(argc == 2 && has_dash_dash) &&
 		    opts->accept_pathspec)

@@ -1265,7 +1229,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 			if (remote) {
 				*new_branch = arg;
 				arg = remote;
-				/* DWIMmed to create local branch, case (3).(b) */
+				/* DWIMmed to create local branch */
 			} else {
 				recover_with_dwim = 0;
 			}

@@ -1280,19 +1244,11 @@ static int parse_branchname_arg(int argc, const char **argv,
 
 	setup_new_branch_info_and_source_tree(new_branch_info, opts, rev, arg);
 
-	if (!opts->source_tree)                   /* case (1): want a tree */
+	if (!opts->source_tree)
 		die(_("reference is not a tree: %s"), arg);
 
-	if (!expect_commit_only) {	/* case (3).(d) -> (1) */
-		/*
-		 * Do not complain the most common case
-		 *	git checkout branch
-		 * even if there happen to be a file called 'branch';
-		 * it would be extremely annoying.
-		 */
-		if (argc > 1)
-			verify_non_filename(opts->prefix, arg);
-	}
+	if (!expect_commit_only && argc > 1)
+		verify_non_filename(opts->prefix, arg);
 
 	return (dash_dash_pos == 1) ? 2 : 1;
 }

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Decisions taken for simplicity:
1) For now, --pathspec-from-file is declared incompatible with --patch, even when <file> is not stdin. Such use case it not really expected.
2) It is not allowed to pass pathspec in both args and file.

you must specify path(s) to restore block was moved down to be able to test for pathspec.nr instead, because testing for argc is no longer correct.

git switch does not support the new options because it doesn't expect <pathspec> arguments.

---
 Documentation/git-checkout.txt    | 15 +++++
 Documentation/git-restore.txt     | 14 +++++
 builtin/checkout.c                | 33 +++++++++--
 t/t2026-checkout-pathspec-file.sh | 90 ++++++++++++++++++++++++++++++
 t/t2072-restore-pathspec-file.sh  | 91 +++++++++++++++++++++++++++++++
 t/t9902-completion.sh             |  2 +
 6 files changed, 240 insertions(+), 5 deletions(-)
 create mode 100755 t/t2026-checkout-pathspec-file.sh
 create mode 100755 t/t2072-restore-pathspec-file.sh

index 93124f3ad9..ffe3c1bff2 100644
@@ -13,6 +13,7 @@ SYNOPSIS
 '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>] [--] <pathspec>...
+'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] --pathspec-from-file=<file> [--pathspec-file-nul]
 'git checkout' (-p|--patch) [<tree-ish>] [--] [<pathspec>...]
 
 DESCRIPTION

@@ -79,6 +80,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' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <pathspec>...::
+'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] --pathspec-from-file=<file> [--pathspec-file-nul]::
 
 	Overwrite the contents of the files that match the pathspec.
 	When the `<tree-ish>` (most often a commit) is not given, 

@@ -306,6 +308,19 @@ Note that this option uses the no overlay mode by default (see also
 	working tree, but not in `<tree-ish>` are removed, to make them
 	match `<tree-ish>` exactly.
 
+--pathspec-from-file=<file>::
+	Pathspec is passed in `<file>` instead of commandline args. If
+	`<file>` is exactly `-` then standard input is used. Pathspec
+	elements are separated by LF or CR/LF. Pathspec elements can be
+	quoted as explained for the configuration variable `core.quotePath`
+	(see linkgit:git-config[1]). See also `--pathspec-file-nul` and
+	global `--literal-pathspecs`.
+
+--pathspec-file-nul::
+	Only meaningful with `--pathspec-from-file`. Pathspec elements are
+	separated with NUL character and all other characters are taken
+	literally (including newlines and quotes).
+
 <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

index d7bf016bba..5bf60d4943 100644
@@ -9,6 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git restore' [<options>] [--source=<tree>] [--staged] [--worktree] [--] <pathspec>...
+'git restore' [<options>] [--source=<tree>] [--staged] [--worktree] --pathspec-from-file=<file> [--pathspec-file-nul]
 'git restore' (-p|--patch) [<options>] [--source=<tree>] [--staged] [--worktree] [--] [<pathspec>...]
 
 DESCRIPTION

@@ -113,6 +114,19 @@ in linkgit:git-checkout[1] for details.
 	appear in the `--source` tree are removed, to make them match
 	`<tree>` exactly. The default is no-overlay mode.
 
+--pathspec-from-file=<file>::
+	Pathspec is passed in `<file>` instead of commandline args. If
+	`<file>` is exactly `-` then standard input is used. Pathspec
+	elements are separated by LF or CR/LF. Pathspec elements can be
+	quoted as explained for the configuration variable `core.quotePath`
+	(see linkgit:git-config[1]). See also `--pathspec-file-nul` and
+	global `--literal-pathspecs`.
+
+--pathspec-file-nul::
+	Only meaningful with `--pathspec-from-file`. Pathspec elements are
+	separated with NUL character and all other characters are taken
+	literally (including newlines and quotes).
+
 \--::
 	Do not interpret any more arguments as options.
 

index aa4ff14ec2..b02425f49b 100644
@@ -70,6 +70,8 @@ struct checkout_opts {
 	int checkout_worktree;
 	const char *ignore_unmerged_opt;
 	int ignore_unmerged;
+	int pathspec_file_nul;
+	const char *pathspec_from_file;
 
 	const char *new_branch;
 	const char *new_branch_force;

@@ -1202,7 +1204,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 		 * Absence of '--' leaves <pathspec>/<commit> ambiguity.
 		 * Try to resolve it with additional knowledge about pathspec args.
 		 */
-		expect_commit_only = !opts->accept_pathspec;
+		expect_commit_only = !opts->accept_pathspec || opts->pathspec_from_file;
 	} else if (dash_dash_pos == 0) {
 		/* 'git checkout/switch/restore -- [...]' */
 		return 1;  /* Eat '--' */

@@ -1476,6 +1478,8 @@ static struct option *add_checkout_path_options(struct checkout_opts *opts,
 		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_PATHSPEC_FROM_FILE(&opts->pathspec_from_file),
+		OPT_PATHSPEC_FILE_NUL(&opts->pathspec_file_nul),
 		OPT_END()
 	};
 	struct option *newopts = parse_options_concat(prevopts, options);

@@ -1612,10 +1616,6 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			die(_("reference is not a tree: %s"), opts->from_treeish);
 	}
 
-	if (opts->accept_pathspec && !opts->empty_pathspec_ok && !argc &&
-	    !opts->patch_mode)	/* patch mode is special */
-		die(_("you must specify path(s) to restore"));
-
 	if (argc) {
 		parse_pathspec(&opts->pathspec, 0,
 			       opts->patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0,

@@ -1635,10 +1635,33 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 		if (opts->force_detach)
 			die(_("git checkout: --detach does not take a path argument '%s'"),
 			    argv[0]);
+	}
+
+	if (opts->pathspec_from_file) {
+		if (opts->pathspec.nr)
+			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
+
+		if (opts->force_detach)
+			die(_("--pathspec-from-file is incompatible with --detach"));
 
+		if (opts->patch_mode)
+			die(_("--pathspec-from-file is incompatible with --patch"));
+
+		parse_pathspec_file(&opts->pathspec, 0,
+				    0,
+				    prefix, opts->pathspec_from_file, opts->pathspec_file_nul);
+	} else if (opts->pathspec_file_nul) {
+		die(_("--pathspec-file-nul requires --pathspec-from-file"));
+	}
+
+	if (opts->pathspec.nr) {
 		if (1 < !!opts->writeout_stage + !!opts->force + !!opts->merge)
 			die(_("git checkout: --ours/--theirs, --force and --merge are incompatible when\n"
 			      "checking out of the index."));
+	} else {
+		if (opts->accept_pathspec && !opts->empty_pathspec_ok &&
+		    !opts->patch_mode)	/* patch mode is special */
+			die(_("you must specify path(s) to restore"));
 	}
 
 	if (opts->new_branch) {

index ec3eccfd3d..93877ba9cd 100755
@@ -1438,6 +1438,8 @@ test_expect_success 'double dash "git checkout"' '
 	--no-guess Z
 	--no-... Z
 	--overlay Z
+	--pathspec-file-nul Z
+	--pathspec-from-file=Z
 	EOF
 '
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Before this patch, when there were multiple DWIM candidates for remote branch, git decided to try the argument as pathspec instead. I believe that such behavior is a surprise: adding another remote suddenly causes git to discard file contents, because it was unsure which branch to pick. There was an incomplete attempt to prevent that in [3].

I understand that this was never intended:

[1] introduces the unexpected behavior. Before, there was fallback from not-a-ref to pathspec. This is reasonable DWIM. After, there is another fallback from ambiguous-remote to pathspec. I understand that it was kind of copy&paste oversight.

[2] noticed the unexpected behavior but chose to semi-document it instead of forbidding, because the goal of the patch series was focused on something else.

[3] adds die() when there is ambiguity between branch and file. The case of multiple tracking branches is seemingly overlooked.

Change to complain about ambiguity instead of doing unexpected things.

[1] Commit 70c9ac2f ("DWIM "git checkout frotz" to "git checkout -b frotz origin/frotz"" 2009-10-18)
https://public-inbox.org/git/[email protected]/
[2] Commit ad8d5104 ("checkout: add advice for ambiguous "checkout <branch>"", 2018-06-05)

    https://public-inbox.org/git/[email protected]/
[3] Commit be4908f1 ("checkout: disambiguate dwim tracking branches and local files", 2018-11-13)
    https://public-inbox.org/git/[email protected]/

---
 builtin/checkout.c       | 56 ++++++++++++++++++----------------------
 t/t2024-checkout-dwim.sh | 28 ++++++++++++++++++--
 2 files changed, 51 insertions(+), 33 deletions(-)

index e1b9df1543..b847695d2b 100644
@@ -1115,10 +1115,10 @@ static void setup_new_branch_info_and_source_tree(
 
 static const char *parse_remote_branch(const char *arg,
 				       struct object_id *rev,
-				       int could_be_checkout_paths,
-				       int *dwim_remotes_matched)
+				       int could_be_checkout_paths)
 {
-	const char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);
+	int num_matches = 0;
+	const char *remote = unique_tracking_name(arg, rev, &num_matches);
 	
 	if (remote && could_be_checkout_paths) {
 		die(_("'%s' could be both a local file and a tracking branch.\n"

@@ -1126,6 +1126,22 @@ static const char *parse_remote_branch(const char *arg,
 		    arg);
 	}
 
+	if (!remote && num_matches > 1) {
+	    if (advice_checkout_ambiguous_remote_branch_name) {
+		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+			     "you can do so by fully qualifying the name with the --track option:\n"
+			     "\n"
+			     "    git checkout --track origin/<name>\n"
+			     "\n"
+			     "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+			     "one remote, e.g. the 'origin' remote, consider setting\n"
+			     "checkout.defaultRemote=origin in your config."));
+	    }
+
+	    die(_("'%s' matched multiple (%d) remote tracking branches"),
+		arg, num_matches);
+	}
+
 	return remote;
 }
 

@@ -1133,8 +1149,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 				int dwim_new_local_branch_ok,
 				struct branch_info *new_branch_info,
 				struct checkout_opts *opts,
-				struct object_id *rev,
-				int *dwim_remotes_matched)
+				struct object_id *rev)
 {
 	const char **new_branch = &opts->new_branch;
 	int argcount = 0;

@@ -1240,8 +1255,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 
 		if (recover_with_dwim) {
 			const char *remote = parse_remote_branch(arg, rev,
-								 could_be_checkout_paths,
-								 dwim_remotes_matched);
+								 could_be_checkout_paths);
 			if (remote) {
 				*new_branch = arg;
 				arg = remote;

@@ -1505,7 +1519,6 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			 const char * const usagestr[])
 {
 	struct branch_info new_branch_info;
-	int dwim_remotes_matched = 0;
 	int parseopt_flags = 0;
 
 	memset(&new_branch_info, 0, sizeof(new_branch_info));

@@ -1613,8 +1626,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			opts->track == BRANCH_TRACK_UNSPECIFIED &&
 			!opts->new_branch;
 		int n = parse_branchname_arg(argc, argv, dwim_ok,
-					     &new_branch_info, opts, &rev,
-					     &dwim_remotes_matched);
+					     &new_branch_info, opts, &rev);
 		argv += n;
 		argc -= n;
 	} else if (!opts->accept_ref && opts->from_treeish) {

@@ -1672,28 +1684,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 	}
 
 	UNLEAK(opts);
-	if (opts->patch_mode || opts->pathspec.nr) {
-		int ret = checkout_paths(opts, new_branch_info.name);
-		if (ret && dwim_remotes_matched > 1 &&
-		    advice_checkout_ambiguous_remote_branch_name)
-			advise(_("'%s' matched more than one remote tracking branch.\n"
-				 "We found %d remotes with a reference that matched. So we fell back\n"
-				 "on trying to resolve the argument as a path, but failed there too!\n"
-				 "\n"
-				 "If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
-				 "you can do so by fully qualifying the name with the --track option:\n"
-				 "\n"
-				 "    git checkout --track origin/<name>\n"
-				 "\n"
-				 "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
-				 "one remote, e.g. the 'origin' remote, consider setting\n"
-				 "checkout.defaultRemote=origin in your config."),
-			       argv[0],
-			       dwim_remotes_matched);
-		return ret;
-	} else {
+	if (opts->patch_mode || opts->pathspec.nr)
+		return checkout_paths(opts, new_branch_info.name);
+	else
 		return checkout_branch(opts, &new_branch_info);
-	}
 }
 
 int cmd_checkout(int argc, const char **argv, const char *prefix)

index fa0718c730..c35d67b697 100755
@@ -37,7 +37,9 @@ test_expect_success 'setup' '
 		git checkout -b foo &&
 		test_commit a_foo &&
 		git checkout -b bar &&
-		test_commit a_bar
+		test_commit a_bar &&
+		git checkout -b ambiguous_branch_and_file &&
+		test_commit a_ambiguous_branch_and_file
 	) &&
 	git init repo_b &&
 	(

@@ -46,7 +48,9 @@ test_expect_success 'setup' '
 		git checkout -b foo &&
 		test_commit b_foo &&
 		git checkout -b baz &&
-		test_commit b_baz
+		test_commit b_baz &&
+		git checkout -b ambiguous_branch_and_file &&
+		test_commit b_ambiguous_branch_and_file
 	) &&
 	git remote add repo_a repo_a &&
 	git remote add repo_b repo_b &&

@@ -75,6 +79,26 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '
 	test_branch master
 '
 
+test_expect_success 'when arg matches multiple remotes, do not fallback to interpreting as pathspec' '
+	# create a file with name matching remote branch name
+	git checkout -b t_ambiguous_branch_and_file &&
+	>ambiguous_branch_and_file &&
+	git add ambiguous_branch_and_file &&
+	git commit -m "ambiguous_branch_and_file" &&
+
+	# modify file to verify that it will not be touched by checkout
+	test_when_finished "git checkout -- ambiguous_branch_and_file" &&
+	echo "file contents" >ambiguous_branch_and_file &&
+	cp ambiguous_branch_and_file expect &&
+
+	test_must_fail git checkout ambiguous_branch_and_file 2>err &&
+
+	test_i18ngrep "matched multiple (2) remote tracking branches" err &&
+	
+	# file must not be altered
+	test_cmp expect ambiguous_branch_and_file
+'
+
 test_expect_success 'checkout of branch from multiple remotes fails with advice' '
 	git checkout -B master &&
 	test_might_fail git branch -D foo &&

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

git add shows an example of good writing, follow it.

---
 Documentation/git-restore.txt | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

index 1ab2e40ea9..d7bf016bba 100644
@@ -8,8 +8,8 @@ git-restore - Restore working tree files
 SYNOPSIS
 --------
 [verse]
-'git restore' [<options>] [--source=<tree>] [--staged] [--worktree] <pathspec>...
-'git restore' (-p|--patch) [<options>] [--source=<tree>] [--staged] [--worktree] [<pathspec>...]
+'git restore' [<options>] [--source=<tree>] [--staged] [--worktree] [--] <pathspec>...
+'git restore' (-p|--patch) [<options>] [--source=<tree>] [--staged] [--worktree] [--] [<pathspec>...]
 
 DESCRIPTION
 -----------

@@ -113,6 +113,14 @@ in linkgit:git-checkout[1] for details.
 	appear in the `--source` tree are removed, to make them match
 	`<tree>` exactly. The default is no-overlay mode.
 
+\--::
+	Do not interpret any more arguments as options.
+
+<pathspec>...::
+	Limits the paths affected by the operation.
++
+For more details, see the 'pathspec' entry in linkgit:gitglossary[7].
+
 EXAMPLES
 --------
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

It was added in [1]. I understand that the duplicate change was not intentional and comes from an oversight.

Also, in explanation, there was only one section for two synopsis entries.

Fix both problems by removing duplicate synopsis.

<paths> vs <pathspec> is resolved in next patch.

[1] Commit b59698ae ("checkout doc: clarify command line args for "checkout paths" mode" 2017-10-11)

---
 Documentation/git-checkout.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index cf3cac0a2b..2011fdbb1d 100644
@@ -13,7 +13,6 @@ SYNOPSIS
 '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>...]
 
 DESCRIPTION

@@ -79,7 +78,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' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...::
 
 	Overwrite paths in the working tree by replacing with the
 	contents in the index or in the `<tree-ish>` (most often a

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

This is done for the next commit to avoid crazy 7x tab code padding.

---
 builtin/checkout.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

index 3634a3dac1..e1b9df1543 100644
@@ -1113,6 +1113,22 @@ static void setup_new_branch_info_and_source_tree(
 	}
 }
 
+static const char *parse_remote_branch(const char *arg,
+				       struct object_id *rev,
+				       int could_be_checkout_paths,
+				       int *dwim_remotes_matched)
+{
+	const char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);
+	
+	if (remote && 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);
+	}
+
+	return remote;
+}
+
 static int parse_branchname_arg(int argc, const char **argv,
 				int dwim_new_local_branch_ok,
 				struct branch_info *new_branch_info,

@@ -1223,13 +1239,10 @@ static int parse_branchname_arg(int argc, const char **argv,
 			recover_with_dwim = 0;
 
 		if (recover_with_dwim) {
-			const char *remote = unique_tracking_name(arg, rev,
-								  dwim_remotes_matched);
+			const char *remote = parse_remote_branch(arg, rev,
+								 could_be_checkout_paths,
+								 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) */

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Make it easier to understand which branches handle which cases.

Drop obfuscated variable has_dash_dash which also took opts->accept_pathspec into account, making it pretty hard to reason about code, especially when used together with argc and opts->accept_pathspec here:

	if (!(argc == 1 && !has_dash_dash) &&
		!(argc == 2 && has_dash_dash) &&
		opts->accept_pathspec)
		recover_with_dwim = 0;

Avoid double-negation in the code mentioned above ("it is not OK to proceed if it's not one of those cases").

Avoid hard-to-understand condition opts->accept_pathspec in the code mentioned above.

There are some minor die() message changes for:
git switch <commit> <unexpected>
git switch <commit> -- <unexpected>

---
 builtin/checkout.c | 74 ++++++++++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 29 deletions(-)

index 6072f7cef7..aa4ff14ec2 100644
@@ -1153,9 +1153,8 @@ static int parse_branchname_arg(int argc, const char **argv,
 {
 	const char **new_branch = &opts->new_branch;
 	const char *arg;
-	int dash_dash_pos;
-	int has_dash_dash = 0, expect_commit_only = 0;
-	int i;
+	int dash_dash_pos, i;
+	int recover_with_dwim, expect_commit_only;
 
 	/*
 	 * Resolve ambiguity where argv[0] may be <pathspec> or <commit>.

@@ -1174,15 +1173,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 	 *    do that and proceed with (2)(3).
 	 * 5) Otherwise, let caller proceed with <pathspec> interpretation.
 	 */
-	if (!argc)
-		return 0;
-
-	if (!opts->accept_pathspec) {
-		if (argc > 1)
-			die(_("only one reference expected"));
-		has_dash_dash = 1; /* helps disambiguate */
-	}
-
+	 
 	arg = argv[0];
 	dash_dash_pos = -1;
 	for (i = 0; i < argc; i++) {

@@ -1192,17 +1183,49 @@ static int parse_branchname_arg(int argc, const char **argv,
 		}
 	}
 
-	if (opts->accept_pathspec) {
-	    if (dash_dash_pos == 0)
-		    return 1;
-	    else if (dash_dash_pos == 1)
-		    has_dash_dash = 1;
-	    else if (dash_dash_pos >= 2)
-		    die(_("only one reference expected, %d given."), dash_dash_pos);
-	}
+	if (dash_dash_pos == -1) {
+		if (argc == 0) {
+			/* 'git checkout/switch/restore' */
+			return 0;
+		} else if (argc == 1) {
+			/* 'git checkout/switch/restore <something>' */
+			recover_with_dwim = dwim_new_local_branch_ok;
+		} else if (!opts->accept_pathspec) {
+			/* 'git switch <commit> <unexpected> [...]' */
+			die(_("only one reference expected, %d given."), argc);
+		} else {
+			/* 'git checkout/restore <something> <pathspec> [...]' */
+			recover_with_dwim = 0;
+		}
+
+		/*
+		 * Absence of '--' leaves <pathspec>/<commit> ambiguity.
+		 * Try to resolve it with additional knowledge about pathspec args.
+		 */
+		expect_commit_only = !opts->accept_pathspec;
+	} else if (dash_dash_pos == 0) {
+		/* 'git checkout/switch/restore -- [...]' */
+		return 1;  /* Eat '--' */
+	} else if (dash_dash_pos == 1) {
+		if (!opts->accept_pathspec) {
+			/* 'git switch <commit> -- [...]' */
+			die(_("incompatible with pathspec arguments"));
+		}
+
+		if (argc == 2) {
+			/* 'git checkout/restore <commit> --' */
+			recover_with_dwim = dwim_new_local_branch_ok;
+		} else {
+			/* 'git checkout/restore <commit> -- <pathspec> [...]' */
+			recover_with_dwim = 0;
+		}
 
-	if (has_dash_dash)
-	    expect_commit_only = 1;
+		/* Presence of '--' makes it certain that arg is <commit> */
+		expect_commit_only = 1;
+	} else {
+		/* 'git checkout/switch/restore <commit> <unxpected> [...] -- [...]' */
+		die(_("only one reference expected, %d given."), dash_dash_pos);
+	}
 
 	opts->count_checkout_paths = !opts->quiet && !expect_commit_only;
 

@@ -1210,19 +1233,12 @@ static int parse_branchname_arg(int argc, const char **argv,
 		arg = "@{-1}";
 
 	if (get_oid_mb(arg, rev)) {
-		int recover_with_dwim = dwim_new_local_branch_ok;
-
 		int could_be_checkout_paths = !expect_commit_only &&
 			check_filename(opts->prefix, arg);
 
 		if (!expect_commit_only && !no_wildcard(arg))
 			recover_with_dwim = 0;
 
-		if (!(argc == 1 && !has_dash_dash) &&
-		    !(argc == 2 && has_dash_dash) &&
-		    opts->accept_pathspec)
-			recover_with_dwim = 0;
-
 		if (recover_with_dwim) {
 			const char *remote = parse_remote_branch(arg, rev,
 								 could_be_checkout_paths);

gitgitgadget

This topic continues the effort to support --pathspec-from-file in various commands [1][2]. It also includes some refactorings that I developed while working on it - previously submitted separately [3][4] which was probably a mistake.

Anatomy of the branch:
checkout, restore: support the --pathspec-from-file option
Extends --pathspec-from-file to git checkout/restore.

t2024: cover more cases
Some new tests for cases that I deemed worthy while working on parse_branchname_arg() refactoring.

parse_branchname_arg(): refactor the decision making parse_branchname_arg(): update code comments parse_branchname_arg(): introduce expect_commit_only parse_branchname_arg(): easier to understand variables
These patches prepare for || opts->pathspec_from_file addition in git checkout/restore patch. Without this refactoring, I found it pretty hard to modify the old code.

checkout: die() on ambiguous tracking branches parse_branchname_arg(): extract part as new function
Initially I was trying to remove some inconsistency standing in the way of git checkout/restore patch. Later I figured that this change is worthy on its own: it prevents some pretty surprising behavior of git.

doc: restore: synchronize <pathspec> description
doc: checkout: synchronize <pathspec> description
doc: checkout: fix broken text reference
doc: checkout: remove duplicate synopsis
Some polishing of docs in preparation for git checkout/restore patch.

add: support the --pathspec-from-file option
cmd_add: prepare for next patch
Extends --pathspec-from-file to git add.

commit: forbid --pathspec-from-file --all t7107, t7526: directly test parse_pathspec_file()
Some polishing of merged topic [1].

CC'ing people who shown interest in any of the previous topics, thanks for your reviews!

[1] https://public-inbox.org/git/[email protected]/
[2] https://public-inbox.org/git/[email protected]/
[3] https://public-inbox.org/git/[email protected]/
[4] https://public-inbox.org/git/[email protected]/

Changes since V1:
----------------
@Junio please note that V1 was already substantially different from what you merged into next.

1) Added tests for error scenarios related to --pathspec-from-file.
2) Restored tests for --pathspec-file-nul: they are valuable for every command, because they verify that specific commands handle the commandline option correctly.
3) Dropped old tests for git restore that I forgot to delete when I made commit t7107, t7526: directly test parse_pathspec_file().

Alexandr Miloslavskiy (18):
t7107, t7526: directly test parse_pathspec_file()
t7526: add tests for error conditions
t7107: add tests for error conditions
commit: forbid --pathspec-from-file --all
cmd_add: prepare for next patch
add: support the --pathspec-from-file option
doc: checkout: remove duplicate synopsis
doc: checkout: fix broken text reference
doc: checkout: synchronize <pathspec> description
doc: restore: synchronize <pathspec> description parse_branchname_arg(): extract part as new function
checkout: die() on ambiguous tracking branches parse_branchname_arg(): easier to understand variables parse_branchname_arg(): introduce expect_commit_only parse_branchname_arg(): update code comments parse_branchname_arg(): refactor the decision making
t2024: cover more cases
checkout, restore: support the --pathspec-from-file option

 Documentation/git-add.txt           |  16 +-
 Documentation/git-checkout.txt      |  50 +++--
 Documentation/git-restore.txt       |  26 ++-
 Makefile                            |   1 +
 builtin/add.c                       |  60 ++++--
 builtin/checkout.c                  | 279 ++++++++++++++--------------
 builtin/commit.c                    |   3 +
 t/helper/test-parse-pathspec-file.c |  34 ++++
 t/helper/test-tool.c                |   1 +
 t/helper/test-tool.h                |   1 +
 t/t0067-parse_pathspec_file.sh      |  89 +++++++++
 t/t2024-checkout-dwim.sh            |  55 +++++-
 t/t2026-checkout-pathspec-file.sh   |  90 +++++++++
 t/t2072-restore-pathspec-file.sh    |  91 +++++++++
 t/t3704-add-pathspec-file.sh        |  86 +++++++++
 t/t7107-reset-pathspec-file.sh      |  98 ++--------
 t/t7526-commit-pathspec-file.sh     |  83 +++------
 t/t9902-completion.sh               |   2 +
 18 files changed, 743 insertions(+), 322 deletions(-)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh
 create mode 100755 t/t2026-checkout-pathspec-file.sh
 create mode 100755 t/t2072-restore-pathspec-file.sh
 create mode 100755 t/t3704-add-pathspec-file.sh

base-commit: ad05a3d8e5a6a06443836b5e40434262d992889a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-490%2FSyntevoAlex%2F%230207_pathspec_from_file_2-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-490/SyntevoAlex/#0207_pathspec_from_file_2-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/490

Range-diff vs v1:

1: bfdd7a4b56 ! 1: 8d5fb9f40b t7107, t7526: directly test parse_pathspec_file()

     @@ -225,18 +225,14 @@
      -'
      -
      -test_expect_success 'NUL delimiters' '
     --	restore_checkpoint &&
     --
     --	git rm fileA.t fileB.t &&
     --	printf "fileA.t\0fileB.t\0" | git reset --pathspec-from-file=- --pathspec-file-nul &&
     --
     --	cat >expect <<-\EOF &&
     --	 D fileA.t
     --	 D fileB.t
     --	EOF
     --	verify_expect
     --'
     --
     ++test_expect_success '--pathspec-file-nul' '
     + 	restore_checkpoint &&
     + 
     + 	git rm fileA.t fileB.t &&
     +@@
     + 	verify_expect
     + '
     + 
      -test_expect_success 'LF delimiters' '
      -	restore_checkpoint &&
      -
     @@ -361,17 +357,14 @@
      -'
      -
      -test_expect_success 'NUL delimiters' '
     --	restore_checkpoint &&
     --
     --	printf "fileA.t\0fileB.t\0" | git commit --pathspec-from-file=- --pathspec-file-nul -m "Commit" &&
     --
     --	cat >expect <<-\EOF &&
     --	A	fileA.t
     --	A	fileB.t
     --	EOF
     --	verify_expect
     --'
     --
     ++test_expect_success '--pathspec-file-nul' '
     + 	restore_checkpoint &&
     + 
     + 	printf "fileA.t\0fileB.t\0" | git commit --pathspec-from-file=- --pathspec-file-nul -m "Commit" &&
     +@@
     + 	verify_expect
     + '
     + 
      -test_expect_success 'LF delimiters' '
      -	restore_checkpoint &&
      -
  -:  ---------- >  2:  c7cd46d3a3 t7526: add tests for error conditions
  -:  ---------- >  3:  b09d74c347 t7107: add tests for error conditions
  2:  c0980519ed !  4:  deeb860a85 commit: forbid --pathspec-from-file --all
     @@ -20,7 +20,7 @@
      
          [1] Commit e440fc58 ("commit: support the --pathspec-from-file option" 2019-11-19)
      
     -    Co-authored-by: Phillip Wood <[email protected]>
     +    Reported-by: Phillip Wood <[email protected]>
          Signed-off-by: Alexandr Miloslavskiy <[email protected]>
      
       diff --git a/builtin/commit.c b/builtin/commit.c
     @@ -36,3 +36,17 @@
       		if (pathspec.nr)
       			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
       
     +
     + diff --git a/t/t7526-commit-pathspec-file.sh b/t/t7526-commit-pathspec-file.sh
     + --- a/t/t7526-commit-pathspec-file.sh
     + +++ b/t/t7526-commit-pathspec-file.sh
     +@@
     + 	test_must_fail git commit --pathspec-from-file=- --patch -m "Commit" <list 2>err &&
     + 	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-interactive/\-\-patch" err &&
     + 
     ++	test_must_fail git commit --pathspec-from-file=- --all -m "Commit" <list 2>err &&
     ++	test_i18ngrep "\-\-pathspec-from-file with \-a does not make sense" err &&
     ++
     + 	test_must_fail git commit --pathspec-from-file=- -m "Commit" -- fileA.t <list 2>err &&
     + 	test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&
     + 
  3:  3690964942 =  5:  204a0a4446 cmd_add: prepare for next patch
  4:  0328fdafbb !  6:  1ea5f17847 add: support the --pathspec-from-file option
     @@ -156,6 +156,18 @@
      +	verify_expect
      +'
      +
     ++test_expect_success '--pathspec-file-nul' '
     ++	restore_checkpoint &&
     ++
     ++	printf "fileA.t\0fileB.t\0" | git add --pathspec-from-file=- --pathspec-file-nul &&
     ++
     ++	cat >expect <<-\EOF &&
     ++	A  fileA.t
     ++	A  fileB.t
     ++	EOF
     ++	verify_expect
     ++'
     ++
      +test_expect_success 'only touches what was listed' '
      +	restore_checkpoint &&
      +
     @@ -168,4 +180,29 @@
      +	verify_expect
      +'
      +
     ++test_expect_success 'error conditions' '
     ++	restore_checkpoint &&
     ++	echo fileA.t >list &&
     ++	>empty_list &&
     ++
     ++	test_must_fail git add --pathspec-from-file=- --interactive <list 2>err &&
     ++	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-interactive/\-\-patch" err &&
     ++
     ++	test_must_fail git add --pathspec-from-file=- --patch <list 2>err &&
     ++	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-interactive/\-\-patch" err &&
     ++
     ++	test_must_fail git add --pathspec-from-file=- --edit <list 2>err &&
     ++	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-edit" err &&
     ++
     ++	test_must_fail git add --pathspec-from-file=- -- fileA.t <list 2>err &&
     ++	test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&
     ++
     ++	test_must_fail git add --pathspec-file-nul 2>err &&
     ++	test_i18ngrep "\-\-pathspec-file-nul requires \-\-pathspec-from-file" err &&
     ++	
     ++	# This case succeeds, but still prints to stderr
     ++	git add --pathspec-from-file=- <empty_list 2>err &&
     ++	test_i18ngrep "Nothing specified, nothing added." err
     ++'
     ++
      +test_done
  5:  8677db4b9e =  7:  3d0fcf6ba5 doc: checkout: remove duplicate synopsis
  6:  2d87b6433b =  8:  85f7ccc4e0 doc: checkout: fix broken text reference
  7:  6288a7fa36 =  9:  db6e40d004 doc: checkout: synchronize <pathspec> description
  8:  d2f277bb58 = 10:  c88cbf453a doc: restore: synchronize <pathspec> description
  9:  891d1b91cf = 11:  2c23bd602d parse_branchname_arg(): extract part as new function
 10:  271e8ed3ab = 12:  efd6876874 checkout: die() on ambiguous tracking branches
 11:  a0a884b384 = 13:  2498825230 parse_branchname_arg(): easier to understand variables
 12:  a6b77f9b23 = 14:  2350dc282e parse_branchname_arg(): introduce expect_commit_only
 13:  4e2bb6663c = 15:  46f676b8e0 parse_branchname_arg(): update code comments
 14:  010fd76331 = 16:  319151e4e9 parse_branchname_arg(): refactor the decision making
 15:  ffbc405920 = 17:  542eb709ca t2024: cover more cases
 16:  9af2de98de ! 18:  c293d72832 checkout, restore: support the --pathspec-from-file option
     @@ -218,6 +218,18 @@
      +	verify_expect
      +'
      +
     ++test_expect_success '--pathspec-file-nul' '
     ++	restore_checkpoint &&
     ++
     ++	printf "fileA.t\0fileB.t\0" | git checkout --pathspec-from-file=- --pathspec-file-nul HEAD^1 &&
     ++
     ++	cat >expect <<-\EOF &&
     ++	M  fileA.t
     ++	M  fileB.t
     ++	EOF
     ++	verify_expect
     ++'
     ++
      +test_expect_success 'only touches what was listed' '
      +	restore_checkpoint &&
      +
     @@ -230,6 +242,23 @@
      +	verify_expect
      +'
      +
     ++test_expect_success 'error conditions' '
     ++	restore_checkpoint &&
     ++	echo fileA.t >list &&
     ++
     ++	test_must_fail git checkout --pathspec-from-file=- --detach <list 2>err &&
     ++	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-detach" err &&
     ++
     ++	test_must_fail git checkout --pathspec-from-file=- --patch <list 2>err &&
     ++	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-patch" err &&
     ++
     ++	test_must_fail git checkout --pathspec-from-file=- -- fileA.t <list 2>err &&
     ++	test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&
     ++
     ++	test_must_fail git checkout --pathspec-file-nul 2>err &&
     ++	test_i18ngrep "\-\-pathspec-file-nul requires \-\-pathspec-from-file" err
     ++'
     ++
      +test_done
      
       diff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh
     @@ -274,7 +303,7 @@
      +	test_cmp expect actual
      +}
      +
     -+test_expect_success '--pathspec-from-file from stdin' '
     ++test_expect_success 'simplest' '
      +	restore_checkpoint &&
      +
      +	echo fileA.t | git restore --pathspec-from-file=- --source=HEAD^1 &&
     @@ -285,19 +314,7 @@
      +	verify_expect
      +'
      +
     -+test_expect_success '--pathspec-from-file from file' '
     -+	restore_checkpoint &&
     -+
     -+	echo fileA.t >list &&
     -+	git restore --pathspec-from-file=list --source=HEAD^1 &&
     -+
     -+	cat >expect <<-\EOF &&
     -+	 M fileA.t
     -+	EOF
     -+	verify_expect
     -+'
     -+
     -+test_expect_success 'NUL delimiters' '
     ++test_expect_success '--pathspec-file-nul' '
      +	restore_checkpoint &&
      +
      +	printf "fileA.t\0fileB.t\0" | git restore --pathspec-from-file=- --pathspec-file-nul --source=HEAD^1 &&
     @@ -309,70 +326,34 @@
      +	verify_expect
      +'
      +
     -+test_expect_success 'LF delimiters' '
     -+	restore_checkpoint &&
     -+
     -+	printf "fileA.t\nfileB.t\n" | git restore --pathspec-from-file=- --source=HEAD^1 &&
     -+
     -+	cat >expect <<-\EOF &&
     -+	 M fileA.t
     -+	 M fileB.t
     -+	EOF
     -+	verify_expect
     -+'
     -+
     -+test_expect_success 'no trailing delimiter' '
     -+	restore_checkpoint &&
     -+
     -+	printf "fileA.t\nfileB.t" | git restore --pathspec-from-file=- --source=HEAD^1 &&
     -+
     -+	cat >expect <<-\EOF &&
     -+	 M fileA.t
     -+	 M fileB.t
     -+	EOF
     -+	verify_expect
     -+'
     -+
     -+test_expect_success 'CRLF delimiters' '
     ++test_expect_success 'only touches what was listed' '
      +	restore_checkpoint &&
      +
     -+	printf "fileA.t\r\nfileB.t\r\n" | git restore --pathspec-from-file=- --source=HEAD^1 &&
     ++	printf "fileB.t\nfileC.t\n" | git restore --pathspec-from-file=- --source=HEAD^1 &&
      +
      +	cat >expect <<-\EOF &&
     -+	 M fileA.t
      +	 M fileB.t
     ++	 M fileC.t
      +	EOF
      +	verify_expect
      +'
      +
     -+test_expect_success 'quotes' '
     -+	restore_checkpoint &&
     -+
     -+	printf "\"file\\101.t\"" | git restore --pathspec-from-file=- --source=HEAD^1 &&
     -+
     -+	cat >expect <<-\EOF &&
     -+	 M fileA.t
     -+	EOF
     -+	verify_expect
     -+'
     -+
     -+test_expect_success 'quotes not compatible with --pathspec-file-nul' '
     ++test_expect_success 'error conditions' '
      +	restore_checkpoint &&
     ++	echo fileA.t >list &&
     ++	>empty_list &&
      +
     -+	printf "\"file\\101.t\"" >list &&
     -+	test_must_fail git restore --pathspec-from-file=list --pathspec-file-nul --source=HEAD^1
     -+'
     ++	test_must_fail git restore --pathspec-from-file=- --patch --source=HEAD^1 <list 2>err &&
     ++	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-patch" err &&
      +
     -+test_expect_success 'only touches what was listed' '
     -+	restore_checkpoint &&
     ++	test_must_fail git restore --pathspec-from-file=- --source=HEAD^1 -- fileA.t <list 2>err &&
     ++	test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&
      +
     -+	printf "fileB.t\nfileC.t\n" | git restore --pathspec-from-file=- --source=HEAD^1 &&
     ++	test_must_fail git restore --pathspec-file-nul --source=HEAD^1 2>err &&
     ++	test_i18ngrep "\-\-pathspec-file-nul requires \-\-pathspec-from-file" err &&
      +
     -+	cat >expect <<-\EOF &&
     -+	 M fileB.t
     -+	 M fileC.t
     -+	EOF
     -+	verify_expect
     ++	test_must_fail git restore --pathspec-from-file=- --source=HEAD^1 <empty_list 2>err &&
     ++	test_i18ngrep "you must specify path(s) to restore" err
      +'
      +
      +test_done

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

---
 t/t7107-reset-pathspec-file.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

index f36fce27b9..da833925a6 100755
@@ -74,4 +74,18 @@ test_expect_success '--pathspec-from-file is not compatible with --soft or --har
 	test_must_fail git reset --hard --pathspec-from-file=list
 '
 
+test_expect_success 'error conditions' '
+	restore_checkpoint &&
+	echo fileA.t >list &&
+
+	test_must_fail git reset --pathspec-from-file=- --patch <list 2>err &&
+	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-patch" err &&
+
+	test_must_fail git reset --pathspec-from-file=- -- fileA.t <list 2>err &&
+	test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&
+
+	test_must_fail git reset --pathspec-file-nul 2>err &&
+	test_i18ngrep "\-\-pathspec-file-nul requires \-\-pathspec-from-file" err
+'
+
 test_done

gitgitgadget

Thanks, I have addressed both issues in V2.

Hi Alexandr

This looks good, thanks for the test

Best Wishes

Phillip

"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>

That cuts both ways. For a developer who is too narrowly focused (because s/he spent enough time staring at the code), testing the underlying machinery in a more direct way does feel attractive, but at the same time, what matters to the end users is how well the feature, when integrated into the commands they use (not the test scaffolding like the "test-parse-pathspec-file" command), works.

So "indirect" is not necessarily a bad thing.

"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>

	$ grep -e "-this string begins with and has-many-dashes-in-it" file
	$ grep "[-]this string begins with and has-many-dashes-in-it" file

Thanks, both. I will take this separately and queue directly on top of am/pathspec-from-file to fast-track it, rather than leaving it as a part of larger topic that would take more time to mature.

Sigh... the test part of this patch is taken hostage to an earlier patches in this series that are iffy, so I cannot quite apply this fix alone at this moment.

Yuck.

Here is what I'll queue directly on top of am/pathspec-from-file e440fc58 ("commit: support the --pathspec-from-file option", 2019-11-19), to be fast-tracked.

-- >8 --
Subject: [PATCH] commit: forbid --pathspec-from-file --all

I forgot this in my previous patch --pathspec-from-file for git commit [1]. When both --pathspec-from-file and --all were specified, --all took precedence and --pathspec-from-file was ignored. Before --pathspec-from-file was implemented, this case was prevented by this check in parse_and_validate_options() :

    die(_("paths '%s ...' with -a does not make sense"), argv[0]);

It is unfortunate that these two cases are disconnected. This came as result of how the code was laid out before my patches, where pathspec is parsed outside of parse_and_validate_options(). This branch is already full of refactoring patches and I did not dare to go for another one.

Fix by mirroring die() for --pathspec-from-file as well.

[1] Commit e440fc58 ("commit: support the --pathspec-from-file option" 2019-11-19)

---
 builtin/commit.c                | 3 +++
 t/t7526-commit-pathspec-file.sh | 6 ++++++
 2 files changed, 9 insertions(+)

index ed40729355..c040dc92a4 100644
@@ -347,6 +347,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 		if (interactive)
 			die(_("--pathspec-from-file is incompatible with --interactive/--patch"));
 
+		if (all)
+			die(_("--pathspec-from-file with -a does not make sense"));
+
 		if (pathspec.nr)
 			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
 

index a06b683534..4b58901ed6 100755
@@ -127,4 +127,10 @@ test_expect_success 'only touches what was listed' '
 	verify_expect
 '
 
+test_expect_success '--pathspec-from-file and --all cannot be used together' '
+	restore_checkpoint &&
+	test_must_fail git commit --pathspec-from-file=- --all -m "Commit" 2>err &&
+	test_i18ngrep "[-]-pathspec-from-file with -a does not make sense" err
+'
+
 test_done

2.24.1-722-g0b9e186032

Just to avoid misunderstanding, I am not opposed to adding tests and test helpers that allows direct access to the guts of the machinery to check the behaviour of the lower level codepath. I am merely saying that such tests would not make it unnecessary to have end-to-end tests that validates end-user visible effects.

Thanks.

I agree that it cuts both ways.

Just recently I had an (unrelated) discussion with Johannes Schindelin who forced me to drop 2 of 3 tests (where #3 also by chance covered #1 #2) in some other PR, because too many tests is also evil.

To verify: I see 13 git commands that could benefit from --pathspec-from-file. There are 6 tests that in fact test underlying machinery, which can't be easily influenced by bugs in command's code. That makes 12*6 = 72 tests that are copy&pasted and doesn't test anything new.

Do you suggest to return _all_ tests back into every command? (but also keep the new direct tests, I assume)

Roger.

I will keep my branch as is currently and will rebase it on top of your patch once it's in master.

From: Alexandr Miloslavskiy <[email protected]>

---
 t/t7526-commit-pathspec-file.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

index 4e592f7472..b71c1013e7 100755
@@ -61,4 +61,28 @@ test_expect_success 'only touches what was listed' '
 	verify_expect
 '
 
+test_expect_success 'error conditions' '
+	restore_checkpoint &&
+	echo fileA.t >list &&
+	>empty_list &&
+
+	test_must_fail git commit --pathspec-from-file=- --interactive -m "Commit" <list 2>err &&
+	test_i18ngrep -e "--pathspec-from-file is incompatible with --interactive/--patch" err &&
+
+	test_must_fail git commit --pathspec-from-file=- --patch -m "Commit" <list 2>err &&
+	test_i18ngrep -e "--pathspec-from-file is incompatible with --interactive/--patch" err &&
+
+	test_must_fail git commit --pathspec-from-file=- -m "Commit" -- fileA.t <list 2>err &&
+	test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err &&
+
+	test_must_fail git commit --pathspec-file-nul -m "Commit" 2>err &&
+	test_i18ngrep -e "--pathspec-file-nul requires --pathspec-from-file" err &&
+
+	test_must_fail git commit --pathspec-from-file=- --include -m "Commit" <empty_list 2>err &&
+	test_i18ngrep -e "No paths with --include/--only does not make sense." err &&
+
+	test_must_fail git commit --pathspec-from-file=- --only -m "Commit" <empty_list 2>err &&
+	test_i18ngrep -e "No paths with --include/--only does not make sense." err
+'
+
 test_done

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

In my previous patches, parse_pathspec_file() was tested indirectly by invoking git reset and git commit with properly crafted inputs. This has some disadvantages:
1) A number of tests are copy&pasted for every command where --pathspec-from-file is supported. With just two commands, it wasn't too bad, but I'm going to extend support to many more commands, which would make a handful of low-value tests.
2) Tests are located in suboptimal test packages
3) Tests are indirect

Fix this by testing parse_pathspec_file() directly via a new test helper.

While working on it, I also noticed that quotes testing via "\"file\\101.t\"" was somewhat incorrect: I escaped \ one time while I had to escape it two times! Tests still worked due to " which prevented pathspec from matching files.

Fix this by properly escaping one more time.

---
 Makefile                            |  1 +
 t/helper/test-parse-pathspec-file.c | 34 +++++++++++
 t/helper/test-tool.c                |  1 +
 t/helper/test-tool.h                |  1 +
 t/t0067-parse_pathspec_file.sh      | 89 +++++++++++++++++++++++++++
 t/t7107-reset-pathspec-file.sh      | 94 +++--------------------------
 t/t7526-commit-pathspec-file.sh     | 70 +--------------------
 7 files changed, 136 insertions(+), 154 deletions(-)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh

index 42a061d3fb..24d0271709 100644
@@ -721,6 +721,7 @@ TEST_BUILTINS_OBJS += test-mktemp.o
 TEST_BUILTINS_OBJS += test-oidmap.o
 TEST_BUILTINS_OBJS += test-online-cpus.o
 TEST_BUILTINS_OBJS += test-parse-options.o
+TEST_BUILTINS_OBJS += test-parse-pathspec-file.o
 TEST_BUILTINS_OBJS += test-path-utils.o
 TEST_BUILTINS_OBJS += test-pkt-line.o
 TEST_BUILTINS_OBJS += test-prio-queue.o

new file mode 100644
index 0000000000..e7f525feb9
@@ -0,0 +1,34 @@
+#include "test-tool.h"
+#include "parse-options.h"
+#include "pathspec.h"
+#include "gettext.h"
+
+int cmd__parse_pathspec_file(int argc, const char **argv)
+{
+	struct pathspec pathspec;
+	const char *pathspec_from_file = 0;
+	int pathspec_file_nul = 0, i;
+
+	static const char *const usage[] = {
+		"test-tool parse-pathspec-file --pathspec-from-file [--pathspec-file-nul]",
+		NULL
+	};
+
+	struct option options[] = {
+		OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
+		OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
+		OPT_END()
+	};
+
+	parse_options(argc, argv, 0, options, usage, 0);
+
+	parse_pathspec_file(&pathspec, 0, 0, 0, pathspec_from_file,
+			    pathspec_file_nul);
+
+	for (i = 0; i < pathspec.nr; i++) {
+		printf("%s\n", pathspec.items[i].original);
+	}
+
+	clear_pathspec(&pathspec);
+	return 0;
+}

index f20989d449..c9a232d238 100644
@@ -39,6 +39,7 @@ static struct test_cmd cmds[] = {
 	{ "oidmap", cmd__oidmap },
 	{ "online-cpus", cmd__online_cpus },
 	{ "parse-options", cmd__parse_options },
+	{ "parse-pathspec-file", cmd__parse_pathspec_file },
 	{ "path-utils", cmd__path_utils },
 	{ "pkt-line", cmd__pkt_line },
 	{ "prio-queue", cmd__prio_queue },

index 8ed2af71d1..c8549fd87f 100644
@@ -29,6 +29,7 @@ int cmd__mktemp(int argc, const char **argv);
 int cmd__oidmap(int argc, const char **argv);
 int cmd__online_cpus(int argc, const char **argv);
 int cmd__parse_options(int argc, const char **argv);
+int cmd__parse_pathspec_file(int argc, const char** argv);
 int cmd__path_utils(int argc, const char **argv);
 int cmd__pkt_line(int argc, const char **argv);
 int cmd__prio_queue(int argc, const char **argv);

gitgitgadget

This topic continues the effort to support --pathspec-from-file in various commands [1][2]. It also includes some refactorings that I developed while working on it - previously submitted separately [3][4] which was probably a mistake.

Anatomy of the branch:
checkout, restore: support the --pathspec-from-file option
Extends --pathspec-from-file to git checkout/restore.

t2024: cover more cases
Some new tests for cases that I deemed worthy while working on parse_branchname_arg() refactoring.

parse_branchname_arg(): refactor the decision making parse_branchname_arg(): update code comments parse_branchname_arg(): introduce expect_commit_only parse_branchname_arg(): easier to understand variables
These patches prepare for || opts->pathspec_from_file addition in git checkout/restore patch. Without this refactoring, I found it pretty hard to modify the old code.

checkout: die() on ambiguous tracking branches parse_branchname_arg(): extract part as new function
Initially I was trying to remove some inconsistency standing in the way of git checkout/restore patch. Later I figured that this change is worthy on its own: it prevents some pretty surprising behavior of git.

doc: restore: synchronize <pathspec> description
doc: checkout: synchronize <pathspec> description
doc: checkout: fix broken text reference
doc: checkout: remove duplicate synopsis
Some polishing of docs in preparation for git checkout/restore patch.

add: support the --pathspec-from-file option
cmd_add: prepare for next patch
Extends --pathspec-from-file to git add.

commit: forbid --pathspec-from-file --all t7107, t7526: directly test parse_pathspec_file()
Some polishing of merged topic [1].

CC'ing people who shown interest in any of the previous topics, thanks for your reviews!

[1] https://public-inbox.org/git/[email protected]/
[2] https://public-inbox.org/git/[email protected]/
[3] https://public-inbox.org/git/[email protected]/
[4] https://public-inbox.org/git/[email protected]/

Changes since V1:
----------------
@Junio please note that V1 was already substantially different from what you merged into next.

1) Added tests for error scenarios related to --pathspec-from-file.
2) Restored tests for --pathspec-file-nul: they are valuable for every command, because they verify that specific commands handle the commandline option correctly.
3) Dropped old tests for git restore that I forgot to delete when I made commit t7107, t7526: directly test parse_pathspec_file().

Changes since V2:
----------------
Rebased branch on top of modern master.
Improved code according to code review suggestions in [4] and this topic:
1) Shuffled changes between parse_branchname_arg() commits
2) New commit parse_branchname_arg(): simplify argument eating with a detailed commit message.
3) Further improved commit parse_branchname_arg(): easier to understand variables
4) Fixed an oversight where after refactoring, parse_branchname_arg() could eat -- in git switch - this is more of a theoretical problem because -- is not expected there anyway.
5) Changed aggressive \-\- escaping in tests to use test_i18ngrep -e instead

Alexandr Miloslavskiy (18):
t7107, t7526: directly test parse_pathspec_file()
t7526: add tests for error conditions
t7107: add tests for error conditions
commit: forbid --pathspec-from-file --all
cmd_add: prepare for next patch
add: support the --pathspec-from-file option
doc: checkout: remove duplicate synopsis
doc: checkout: fix broken text reference
doc: checkout: synchronize <pathspec> description
doc: restore: synchronize <pathspec> description parse_branchname_arg(): extract part as new function
checkout: die() on ambiguous tracking branches parse_branchname_arg(): easier to understand variables parse_branchname_arg(): simplify argument eating parse_branchname_arg(): update code comments parse_branchname_arg(): refactor the decision making
t2024: cover more cases
checkout, restore: support the --pathspec-from-file option

 Documentation/git-add.txt           |  16 +-
 Documentation/git-checkout.txt      |  50 +++--
 Documentation/git-restore.txt       |  26 ++-
 Makefile                            |   1 +
 builtin/add.c                       |  60 ++++--
 builtin/checkout.c                  | 277 ++++++++++++++--------------
 builtin/commit.c                    |   3 +
 t/helper/test-parse-pathspec-file.c |  34 ++++
 t/helper/test-tool.c                |   1 +
 t/helper/test-tool.h                |   1 +
 t/t0067-parse_pathspec_file.sh      |  89 +++++++++
 t/t2024-checkout-dwim.sh            |  55 +++++-
 t/t2026-checkout-pathspec-file.sh   |  90 +++++++++
 t/t2072-restore-pathspec-file.sh    |  91 +++++++++
 t/t3704-add-pathspec-file.sh        |  86 +++++++++
 t/t7107-reset-pathspec-file.sh      |  98 ++--------
 t/t7526-commit-pathspec-file.sh     |  83 +++------
 t/t9902-completion.sh               |   2 +
 18 files changed, 742 insertions(+), 321 deletions(-)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh
 create mode 100755 t/t2026-checkout-pathspec-file.sh
 create mode 100755 t/t2072-restore-pathspec-file.sh
 create mode 100755 t/t3704-add-pathspec-file.sh

base-commit: 12029dc57db23baef008e77db1909367599210ee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-490%2FSyntevoAlex%2F%230207_pathspec_from_file_2-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-490/SyntevoAlex/#0207_pathspec_from_file_2-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/490

Range-diff vs v2:

  1:  8d5fb9f40b =  1:  8526d1d805 t7107, t7526: directly test parse_pathspec_file()
  2:  c7cd46d3a3 !  2:  14d30dd0e1 t7526: add tests for error conditions
     @@ -18,22 +18,22 @@
      +	>empty_list &&
      +
      +	test_must_fail git commit --pathspec-from-file=- --interactive -m "Commit" <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-interactive/\-\-patch" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with --interactive/--patch" err &&
      +
      +	test_must_fail git commit --pathspec-from-file=- --patch -m "Commit" <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-interactive/\-\-patch" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with --interactive/--patch" err &&
      +
      +	test_must_fail git commit --pathspec-from-file=- -m "Commit" -- fileA.t <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err &&
      +
      +	test_must_fail git commit --pathspec-file-nul -m "Commit" 2>err &&
     -+	test_i18ngrep "\-\-pathspec-file-nul requires \-\-pathspec-from-file" err &&
     ++	test_i18ngrep -e "--pathspec-file-nul requires --pathspec-from-file" err &&
      +
      +	test_must_fail git commit --pathspec-from-file=- --include -m "Commit" <empty_list 2>err &&
     -+	test_i18ngrep "No paths with \-\-include/\-\-only does not make sense." err &&
     ++	test_i18ngrep -e "No paths with --include/--only does not make sense." err &&
      +
      +	test_must_fail git commit --pathspec-from-file=- --only -m "Commit" <empty_list 2>err &&
     -+	test_i18ngrep "No paths with \-\-include/\-\-only does not make sense." err
     ++	test_i18ngrep -e "No paths with --include/--only does not make sense." err
      +'
      +
       test_done
  3:  b09d74c347 !  3:  b1cc4a960d t7107: add tests for error conditions
     @@ -17,13 +17,13 @@
      +	echo fileA.t >list &&
      +
      +	test_must_fail git reset --pathspec-from-file=- --patch <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-patch" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with --patch" err &&
      +
      +	test_must_fail git reset --pathspec-from-file=- -- fileA.t <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err &&
      +
      +	test_must_fail git reset --pathspec-file-nul 2>err &&
     -+	test_i18ngrep "\-\-pathspec-file-nul requires \-\-pathspec-from-file" err
     ++	test_i18ngrep -e "--pathspec-file-nul requires --pathspec-from-file" err
      +'
      +
       test_done
  4:  deeb860a85 !  4:  f0be90601e commit: forbid --pathspec-from-file --all
     @@ -42,11 +42,11 @@
       +++ b/t/t7526-commit-pathspec-file.sh
      @@
       	test_must_fail git commit --pathspec-from-file=- --patch -m "Commit" <list 2>err &&
     - 	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-interactive/\-\-patch" err &&
     + 	test_i18ngrep -e "--pathspec-from-file is incompatible with --interactive/--patch" err &&
       
      +	test_must_fail git commit --pathspec-from-file=- --all -m "Commit" <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file with \-a does not make sense" err &&
     ++	test_i18ngrep -e "--pathspec-from-file with -a does not make sense" err &&
      +
       	test_must_fail git commit --pathspec-from-file=- -m "Commit" -- fileA.t <list 2>err &&
     - 	test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&
     + 	test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err &&

  5:  204a0a4446 =  5:  2153350ac4 cmd_add: prepare for next patch
  6:  1ea5f17847 !  6:  c8e59903f7 add: support the --pathspec-from-file option
     @@ -186,23 +186,23 @@
      +	>empty_list &&
      +
      +	test_must_fail git add --pathspec-from-file=- --interactive <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-interactive/\-\-patch" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with --interactive/--patch" err &&
      +
      +	test_must_fail git add --pathspec-from-file=- --patch <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-interactive/\-\-patch" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with --interactive/--patch" err &&
      +
      +	test_must_fail git add --pathspec-from-file=- --edit <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-edit" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with --edit" err &&
      +
      +	test_must_fail git add --pathspec-from-file=- -- fileA.t <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err &&
      +
      +	test_must_fail git add --pathspec-file-nul 2>err &&
     -+	test_i18ngrep "\-\-pathspec-file-nul requires \-\-pathspec-from-file" err &&
     ++	test_i18ngrep -e "--pathspec-file-nul requires --pathspec-from-file" err &&
      +	
      +	# This case succeeds, but still prints to stderr
      +	git add --pathspec-from-file=- <empty_list 2>err &&
     -+	test_i18ngrep "Nothing specified, nothing added." err
     ++	test_i18ngrep -e "Nothing specified, nothing added." err
      +'
      +
      +test_done
  7:  3d0fcf6ba5 =  7:  5a4a538fa6 doc: checkout: remove duplicate synopsis
  8:  85f7ccc4e0 =  8:  daf0d6c536 doc: checkout: fix broken text reference
  9:  db6e40d004 =  9:  1d981b199f doc: checkout: synchronize <pathspec> description
 10:  c88cbf453a = 10:  137b697327 doc: restore: synchronize <pathspec> description
 11:  2c23bd602d = 11:  f53bb13e43 parse_branchname_arg(): extract part as new function
 12:  efd6876874 = 12:  58b65d2011 checkout: die() on ambiguous tracking branches
 14:  2350dc282e ! 13:  dd35cad0d9 parse_branchname_arg(): introduce expect_commit_only
     @@ -1,8 +1,13 @@
      Author: Alexandr Miloslavskiy <[email protected]>
      
     -    parse_branchname_arg(): introduce expect_commit_only
     +    parse_branchname_arg(): easier to understand variables
      
     -    `has_dash_dash` unexpectedly takes `opts->accept_pathspec` into account.
     +    `dash_dash_pos` was only calculated under `opts->accept_pathspec`. This
     +    is unexpected to readers and made it harder to reason about the code.
     +
     +    Fix this by restoring the expected meaning.
     +
     +    `has_dash_dash` also takes `opts->accept_pathspec` into account.
          While this may sound clever at first sight, it becomes pretty hard to
          reason (and not be a victim) about code, especially in combination with
          `argc` here:
     @@ -12,10 +17,11 @@
                      opts->accept_pathspec)
                          recover_with_dwim = 0;
      
     -    Introduce a new non-obfuscated variable to reduce the amount of diffs in
     -    next patch.
     +    Fix this by giving variable a better name and rewriting the above
     +    mentioned condition (it's easier to verify if you notice that it only
     +    holds when `opts->accept_pathspec` is true).
      
     -    This should not change behavior in any way.
     +    This patch is not expected to change behavior in any way.
      
          Signed-off-by: Alexandr Miloslavskiy <[email protected]>
      
     @@ -23,23 +29,49 @@
       --- a/builtin/checkout.c
       +++ b/builtin/checkout.c
      @@
     - 	const char **new_branch = &opts->new_branch;
     + 	int argcount = 0;
       	const char *arg;
       	int dash_dash_pos;
      -	int has_dash_dash = 0;
     -+	int has_dash_dash = 0, expect_commit_only = 0;
     ++	int arg0_cant_be_pathspec = 0;
       	int i;
       
       	/*
      @@
     - 		    die(_("only one reference expected, %d given."), dash_dash_pos);
     + 	if (!opts->accept_pathspec) {
     + 		if (argc > 1)
     + 			die(_("only one reference expected"));
     +-		has_dash_dash = 1; /* helps disambiguate */
     ++		arg0_cant_be_pathspec = 1;
       	}
       
     + 	arg = argv[0];
     + 	dash_dash_pos = -1;
     + 	for (i = 0; i < argc; i++) {
     +-		if (opts->accept_pathspec && !strcmp(argv[i], "--")) {
     ++		if (!strcmp(argv[i], "--")) {
     + 			dash_dash_pos = i;
     + 			break;
     + 		}
     + 	}
     +-	if (dash_dash_pos == 0)
     +-		return 1; /* case (2) */
     +-	else if (dash_dash_pos == 1)
     +-		has_dash_dash = 1; /* case (3) or (1) */
     +-	else if (dash_dash_pos >= 2)
     +-		die(_("only one reference expected, %d given."), dash_dash_pos);
      -	opts->count_checkout_paths = !opts->quiet && !has_dash_dash;
     -+	if (has_dash_dash)
     -+	    expect_commit_only = 1;
      +
     -+	opts->count_checkout_paths = !opts->quiet && !expect_commit_only;
     ++	if (opts->accept_pathspec) {
     ++		if (dash_dash_pos == 0)
     ++			return 1; /* case (2) */
     ++		else if (dash_dash_pos == 1)
     ++			arg0_cant_be_pathspec = 1; /* case (3) or (1) */
     ++		else if (dash_dash_pos >= 2)
     ++			die(_("only one reference expected, %d given."), dash_dash_pos);
     ++	}
     ++
     ++	opts->count_checkout_paths = !opts->quiet && !arg0_cant_be_pathspec;
       
       	if (!strcmp(arg, "-"))
       		arg = "@{-1}";
     @@ -48,29 +80,39 @@
       		int recover_with_dwim = dwim_new_local_branch_ok;
       
      -		int could_be_checkout_paths = !has_dash_dash &&
     -+		int could_be_checkout_paths = !expect_commit_only &&
     ++		int could_be_checkout_paths = !arg0_cant_be_pathspec &&
       			check_filename(opts->prefix, arg);
       
      -		if (!has_dash_dash && !no_wildcard(arg))
     -+		if (!expect_commit_only && !no_wildcard(arg))
     ++		if (!arg0_cant_be_pathspec && !no_wildcard(arg))
       			recover_with_dwim = 0;
       
       		/*
     + 		 * Accept "git checkout foo", "git checkout foo --"
     + 		 * and "git switch foo" as candidates for dwim.
     + 		 */
     +-		if (!(argc == 1 && !has_dash_dash) &&
     +-		    !(argc == 2 && has_dash_dash) &&
     ++		if (!(argc == 1 && dash_dash_pos == -1) &&
     ++		    !(argc == 2 && dash_dash_pos == 1) &&
     + 		    opts->accept_pathspec)
     + 			recover_with_dwim = 0;
     + 
      @@
       		}
       
       		if (!recover_with_dwim) {
      -			if (has_dash_dash)
     -+			if (expect_commit_only)
     ++			if (arg0_cant_be_pathspec)
       				die(_("invalid reference: %s"), arg);
     - 			return 0;
     + 			return argcount;
       		}
      @@
       	if (!opts->source_tree)                   /* case (1): want a tree */
       		die(_("reference is not a tree: %s"), arg);
       
      -	if (!has_dash_dash) {	/* case (3).(d) -> (1) */
     -+	if (!expect_commit_only) {	/* case (3).(d) -> (1) */
     ++	if (!arg0_cant_be_pathspec) {	/* case (3).(d) -> (1) */
       		/*
       		 * Do not complain the most common case
       		 *	git checkout branch

13: 2498825230 ! 14: dc6e351796 parse_branchname_arg(): easier to understand variables
@@ -1,15 +1,27 @@
Author: Alexandr Miloslavskiy <[email protected]>

- parse_branchname_arg(): easier to understand variables
+ parse_branchname_arg(): simplify argument eating

- dash_dash_pos was only calculated under opts->accept_pathspec. This
- is unexpected to readers and made it harder to reason about the code.
- Fix this by restoring the expected meaning.
+ This patch abolishes two pieces of obfuscated code.

- Simplify the code by dropping argcount and useless argc / argv
- manipulations.
+ First is if (argc) condition, which in fact means "more then one arg"
+ due to argc++ above.

- This should not change behavior in any way.
+ Second is by far harder to grasp:
+ if (!arg0_cant_be_pathspec) {...} else if (opts->accept_pathspec)
+ that is,
+ if (opts->accept_pathspec && arg0_cant_be_pathspec)
+ which (quite unexpectedly) actually means
+ if (opts->accept_pathspec && dash_dash_pos == 1)
+ and aims to "eat" that --.
+
+ Make both pieces easier to read by rewriting obfuscated conditions.
+
+ With both solved, I could keep argcount++/argv++/argc-- in the very end
+ of the function, but that was obviously useless code in this case, so I
+ deleted them as well.
+
+ This patch is not expected to change behavior in any way.

@@ -23,39 +35,10 @@
- int argcount = 0;
const char *arg;
int dash_dash_pos;
- int has_dash_dash = 0;
-@@
- arg = argv[0];
- dash_dash_pos = -1;
- for (i = 0; i < argc; i++) {
-- if (opts->accept_pathspec && !strcmp(argv[i], "--")) {
-+ if (!strcmp(argv[i], "--")) {
- dash_dash_pos = i;
- break;
- }
- }
-- if (dash_dash_pos == 0)
-- return 1; /* case (2) */
-- else if (dash_dash_pos == 1)
-- has_dash_dash = 1; /* case (3) or (1) */
-- else if (dash_dash_pos >= 2)
-- die(_("only one reference expected, %d given."), dash_dash_pos);
-+
-+ if (opts->accept_pathspec) {
-+ if (dash_dash_pos == 0)
-+ return 1; /* case (2) */
-+ else if (dash_dash_pos == 1)
-+ has_dash_dash = 1; /* case (3) or (1) */
-+ else if (dash_dash_pos >= 2)
-+ die(_("only one reference expected, %d given."), dash_dash_pos);
-+ }
-+
- opts->count_checkout_paths = !opts->quiet && !has_dash_dash;
-
- if (!strcmp(arg, "-"))
+ int arg0_cant_be_pathspec = 0;
@@
if (!recover_with_dwim) {
- if (has_dash_dash)
+ if (arg0_cant_be_pathspec)
die(_("invalid reference: %s"), arg);
- return argcount;
+ return 0;
@@ -84,7 +67,7 @@
}

- return argcount;
-+ return (dash_dash_pos == 1) ? 2 : 1;
++ return (opts->accept_pathspec && dash_dash_pos == 1) ? 2 : 1;
}

static int switch_unborn_to_new_branch(const struct checkout_opts *opts)
15: 46f676b8e0 ! 15: 7989f0c5cf parse_branchname_arg(): update code comments

     @@ -80,14 +80,14 @@
      @@
       
       	if (opts->accept_pathspec) {
     - 	    if (dash_dash_pos == 0)
     --		    return 1; /* case (2) */
     -+		    return 1;
     - 	    else if (dash_dash_pos == 1)
     --		    has_dash_dash = 1; /* case (3) or (1) */
     -+		    has_dash_dash = 1;
     - 	    else if (dash_dash_pos >= 2)
     - 		    die(_("only one reference expected, %d given."), dash_dash_pos);
     + 		if (dash_dash_pos == 0)
     +-			return 1; /* case (2) */
     ++			return 1;
     + 		else if (dash_dash_pos == 1)
     +-			arg0_cant_be_pathspec = 1; /* case (3) or (1) */
     ++			arg0_cant_be_pathspec = 1;
     + 		else if (dash_dash_pos >= 2)
     + 			die(_("only one reference expected, %d given."), dash_dash_pos);
       	}
      @@
       		arg = "@{-1}";
     @@ -103,17 +103,17 @@
      -		 */
       		int recover_with_dwim = dwim_new_local_branch_ok;
       
     - 		int could_be_checkout_paths = !expect_commit_only &&
     + 		int could_be_checkout_paths = !arg0_cant_be_pathspec &&
      @@
     - 		if (!expect_commit_only && !no_wildcard(arg))
     + 		if (!arg0_cant_be_pathspec && !no_wildcard(arg))
       			recover_with_dwim = 0;
       
      -		/*
      -		 * Accept "git checkout foo", "git checkout foo --"
      -		 * and "git switch foo" as candidates for dwim.
      -		 */
     - 		if (!(argc == 1 && !has_dash_dash) &&
     - 		    !(argc == 2 && has_dash_dash) &&
     + 		if (!(argc == 1 && dash_dash_pos == -1) &&
     + 		    !(argc == 2 && dash_dash_pos == 1) &&
       		    opts->accept_pathspec)
      @@
       			if (remote) {
     @@ -132,7 +132,7 @@
      +	if (!opts->source_tree)
       		die(_("reference is not a tree: %s"), arg);
       
     --	if (!expect_commit_only) {	/* case (3).(d) -> (1) */
     +-	if (!arg0_cant_be_pathspec) {	/* case (3).(d) -> (1) */
      -		/*
      -		 * Do not complain the most common case
      -		 *	git checkout branch
     @@ -142,8 +142,8 @@
      -		if (argc > 1)
      -			verify_non_filename(opts->prefix, arg);
      -	}
     -+	if (!expect_commit_only && argc > 1)
     ++	if (!arg0_cant_be_pathspec && argc > 1)
      +		verify_non_filename(opts->prefix, arg);
       
     - 	return (dash_dash_pos == 1) ? 2 : 1;
     + 	return (opts->accept_pathspec && dash_dash_pos == 1) ? 2 : 1;
       }
 16:  319151e4e9 ! 16:  e0bdd5bd1e parse_branchname_arg(): refactor the decision making
     @@ -34,10 +34,10 @@
       	const char **new_branch = &opts->new_branch;
       	const char *arg;
      -	int dash_dash_pos;
     --	int has_dash_dash = 0, expect_commit_only = 0;
     +-	int arg0_cant_be_pathspec = 0;
      -	int i;
      +	int dash_dash_pos, i;
     -+	int recover_with_dwim, expect_commit_only;
     ++	int recover_with_dwim, arg0_cant_be_pathspec;
       
       	/*
       	 * Resolve ambiguity where argv[0] may be <pathspec> or <commit>.
     @@ -51,25 +51,22 @@
      -	if (!opts->accept_pathspec) {
      -		if (argc > 1)
      -			die(_("only one reference expected"));
     --		has_dash_dash = 1; /* helps disambiguate */
     +-		arg0_cant_be_pathspec = 1;
      -	}
     --
     -+	 
     + 
       	arg = argv[0];
       	dash_dash_pos = -1;
     - 	for (i = 0; i < argc; i++) {
      @@
       		}
       	}
       
      -	if (opts->accept_pathspec) {
     --	    if (dash_dash_pos == 0)
     --		    return 1;
     --	    else if (dash_dash_pos == 1)
     --		    has_dash_dash = 1;
     --	    else if (dash_dash_pos >= 2)
     --		    die(_("only one reference expected, %d given."), dash_dash_pos);
     --	}
     +-		if (dash_dash_pos == 0)
     +-			return 1;
     +-		else if (dash_dash_pos == 1)
     +-			arg0_cant_be_pathspec = 1;
     +-		else if (dash_dash_pos >= 2)
     +-			die(_("only one reference expected, %d given."), dash_dash_pos);
      +	if (dash_dash_pos == -1) {
      +		if (argc == 0) {
      +			/* 'git checkout/switch/restore' */
     @@ -89,7 +86,7 @@
      +		 * Absence of '--' leaves <pathspec>/<commit> ambiguity.
      +		 * Try to resolve it with additional knowledge about pathspec args.
      +		 */
     -+		expect_commit_only = !opts->accept_pathspec;
     ++		arg0_cant_be_pathspec = !opts->accept_pathspec;
      +	} else if (dash_dash_pos == 0) {
      +		/* 'git checkout/switch/restore -- [...]' */
      +		return 1;  /* Eat '--' */
     @@ -106,32 +103,29 @@
      +			/* 'git checkout/restore <commit> -- <pathspec> [...]' */
      +			recover_with_dwim = 0;
      +		}
     - 
     --	if (has_dash_dash)
     --	    expect_commit_only = 1;
     ++
      +		/* Presence of '--' makes it certain that arg is <commit> */
     -+		expect_commit_only = 1;
     ++		arg0_cant_be_pathspec = 1;
      +	} else {
      +		/* 'git checkout/switch/restore <commit> <unxpected> [...] -- [...]' */
      +		die(_("only one reference expected, %d given."), dash_dash_pos);
     -+	}
     - 
     - 	opts->count_checkout_paths = !opts->quiet && !expect_commit_only;
     + 	}
       
     + 	opts->count_checkout_paths = !opts->quiet && !arg0_cant_be_pathspec;
      @@
       		arg = "@{-1}";
       
       	if (get_oid_mb(arg, rev)) {
      -		int recover_with_dwim = dwim_new_local_branch_ok;
      -
     - 		int could_be_checkout_paths = !expect_commit_only &&
     + 		int could_be_checkout_paths = !arg0_cant_be_pathspec &&
       			check_filename(opts->prefix, arg);
       
     - 		if (!expect_commit_only && !no_wildcard(arg))
     + 		if (!arg0_cant_be_pathspec && !no_wildcard(arg))
       			recover_with_dwim = 0;
       
     --		if (!(argc == 1 && !has_dash_dash) &&
     --		    !(argc == 2 && has_dash_dash) &&
     +-		if (!(argc == 1 && dash_dash_pos == -1) &&
     +-		    !(argc == 2 && dash_dash_pos == 1) &&
      -		    opts->accept_pathspec)
      -			recover_with_dwim = 0;
      -
 17:  542eb709ca = 17:  121d3f06a6 t2024: cover more cases
 18:  c293d72832 ! 18:  7324e091ba checkout, restore: support the --pathspec-from-file option
     @@ -105,8 +105,8 @@
       		 * Absence of '--' leaves <pathspec>/<commit> ambiguity.
       		 * Try to resolve it with additional knowledge about pathspec args.
       		 */
     --		expect_commit_only = !opts->accept_pathspec;
     -+		expect_commit_only = !opts->accept_pathspec || opts->pathspec_from_file;
     +-		arg0_cant_be_pathspec = !opts->accept_pathspec;
     ++		arg0_cant_be_pathspec = !opts->accept_pathspec || opts->pathspec_from_file;
       	} else if (dash_dash_pos == 0) {
       		/* 'git checkout/switch/restore -- [...]' */
       		return 1;  /* Eat '--' */
     @@ -247,16 +247,16 @@
      +	echo fileA.t >list &&
      +
      +	test_must_fail git checkout --pathspec-from-file=- --detach <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-detach" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with --detach" err &&
      +
      +	test_must_fail git checkout --pathspec-from-file=- --patch <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-patch" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with --patch" err &&
      +
      +	test_must_fail git checkout --pathspec-from-file=- -- fileA.t <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err &&
      +
      +	test_must_fail git checkout --pathspec-file-nul 2>err &&
     -+	test_i18ngrep "\-\-pathspec-file-nul requires \-\-pathspec-from-file" err
     ++	test_i18ngrep -e "--pathspec-file-nul requires --pathspec-from-file" err
      +'
      +
      +test_done
     @@ -344,16 +344,16 @@
      +	>empty_list &&
      +
      +	test_must_fail git restore --pathspec-from-file=- --patch --source=HEAD^1 <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-patch" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with --patch" err &&
      +
      +	test_must_fail git restore --pathspec-from-file=- --source=HEAD^1 -- fileA.t <list 2>err &&
     -+	test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&
     ++	test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err &&
      +
      +	test_must_fail git restore --pathspec-file-nul --source=HEAD^1 2>err &&
     -+	test_i18ngrep "\-\-pathspec-file-nul requires \-\-pathspec-from-file" err &&
     ++	test_i18ngrep -e "--pathspec-file-nul requires --pathspec-from-file" err &&
      +
      +	test_must_fail git restore --pathspec-from-file=- --source=HEAD^1 <empty_list 2>err &&
     -+	test_i18ngrep "you must specify path(s) to restore" err
     ++	test_i18ngrep -e "you must specify path(s) to restore" err
      +'
      +
      +test_done

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

---
 t/t7107-reset-pathspec-file.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

index f36fce27b9..e91b0ff62d 100755
@@ -74,4 +74,18 @@ test_expect_success '--pathspec-from-file is not compatible with --soft or --har
 	test_must_fail git reset --hard --pathspec-from-file=list
 '
 
+test_expect_success 'error conditions' '
+	restore_checkpoint &&
+	echo fileA.t >list &&
+
+	test_must_fail git reset --pathspec-from-file=- --patch <list 2>err &&
+	test_i18ngrep -e "--pathspec-from-file is incompatible with --patch" err &&
+
+	test_must_fail git reset --pathspec-from-file=- -- fileA.t <list 2>err &&
+	test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err &&
+
+	test_must_fail git reset --pathspec-file-nul 2>err &&
+	test_i18ngrep -e "--pathspec-file-nul requires --pathspec-from-file" err
+'
+
 test_done

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

I forgot this in my previous patch --pathspec-from-file for git commit [1]. When both --pathspec-from-file and --all were specified, --all took precedence and --pathspec-from-file was ignored. Before --pathspec-from-file was implemented, this case was prevented by this check in parse_and_validate_options() :

    die(_("paths '%s ...' with -a does not make sense"), argv[0]);

It is unfortunate that these two cases are disconnected. This came as result of how the code was laid out before my patches, where pathspec is parsed outside of parse_and_validate_options(). This branch is already full of refactoring patches and I did not dare to go for another one.

Fix by mirroring die() for --pathspec-from-file as well.

[1] Commit e440fc58 ("commit: support the --pathspec-from-file option" 2019-11-19)

---
 builtin/commit.c                | 3 +++
 t/t7526-commit-pathspec-file.sh | 3 +++
 2 files changed, 6 insertions(+)

index e48c1fd90a..aa1332308a 100644
@@ -347,6 +347,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 		if (interactive)
 			die(_("--pathspec-from-file is incompatible with --interactive/--patch"));
 
+		if (all)
+			die(_("--pathspec-from-file with -a does not make sense"));
+
 		if (pathspec.nr)
 			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
 

index b71c1013e7..bc65125741 100755
@@ -72,6 +72,9 @@ test_expect_success 'error conditions' '
 	test_must_fail git commit --pathspec-from-file=- --patch -m "Commit" <list 2>err &&
 	test_i18ngrep -e "--pathspec-from-file is incompatible with --interactive/--patch" err &&
 
+	test_must_fail git commit --pathspec-from-file=- --all -m "Commit" <list 2>err &&
+	test_i18ngrep -e "--pathspec-from-file with -a does not make sense" err &&
+
 	test_must_fail git commit --pathspec-from-file=- -m "Commit" -- fileA.t <list 2>err &&
 	test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err &&
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Some code blocks were moved down to be able to test for pathspec.nr in the next patch. Blocks are moved as is without any changes. This is done as separate patch to reduce the amount of diffs in next patch.

---
 builtin/add.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

index d4686d5218..3d1791dd82 100644
@@ -430,10 +430,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (addremove && take_worktree_changes)
 		die(_("-A and -u are mutually incompatible"));
 
-	if (!take_worktree_changes && addremove_explicit < 0 && argc)
-		/* Turn "git add pathspec..." to "git add -A pathspec..." */
-		addremove = 1;
-
 	if (!show_only && ignore_missing)
 		die(_("Option --ignore-missing can only be used together with --dry-run"));
 

@@ -446,19 +442,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
 
-	flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
-		 (show_only ? ADD_CACHE_PRETEND : 0) |
-		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
-		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
-		 (!(addremove || take_worktree_changes)
-		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
-
-	if (require_pathspec && argc == 0) {
-		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
-		return 0;
-	}
-
 	/*
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.

@@ -468,6 +451,23 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
 
+	if (require_pathspec && argc == 0) {
+		fprintf(stderr, _("Nothing specified, nothing added.\n"));
+		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
+		return 0;
+	}
+
+	if (!take_worktree_changes && addremove_explicit < 0 && argc)
+		/* Turn "git add pathspec..." to "git add -A pathspec..." */
+		addremove = 1;
+
+	flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
+		 (show_only ? ADD_CACHE_PRETEND : 0) |
+		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
+		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
+		 (!(addremove || take_worktree_changes)
+		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
+
 	if (read_cache_preload(&pathspec) < 0)
 		die(_("index file corrupt"));
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

It was added in [1]. I understand that the duplicate change was not intentional and comes from an oversight.

Also, in explanation, there was only one section for two synopsis entries.

Fix both problems by removing duplicate synopsis.

<paths> vs <pathspec> is resolved in next patch.

[1] Commit b59698ae ("checkout doc: clarify command line args for "checkout paths" mode" 2017-10-11)

---
 Documentation/git-checkout.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index cf3cac0a2b..2011fdbb1d 100644
@@ -13,7 +13,6 @@ SYNOPSIS
 '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>...]
 
 DESCRIPTION

@@ -79,7 +78,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' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...::
 
 	Overwrite paths in the working tree by replacing with the
 	contents in the index or in the `<tree-ish>` (most often a

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

---
 Documentation/git-checkout.txt | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

index 2011fdbb1d..d47046e050 100644
@@ -95,12 +95,10 @@ 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>...]::
-	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
-	the "diff" output and choose which hunks to use in the
-	result.  See below for the description of `--patch` option.
-
+	This is similar to the previous mode, but lets you use the
+	interactive interface to show the "diff" output and choose which
+	hunks to use in the result.  See below for the description of
+	`--patch` option.
 
 OPTIONS
 -------

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Decisions taken for simplicity:
1) For now, --pathspec-from-file is declared incompatible with --interactive/--patch/--edit, even when <file> is not stdin. Such use case it not really expected. Also, it would require changes to interactive_add() and edit_patch().
2) It is not allowed to pass pathspec in both args and file.

---
 Documentation/git-add.txt    | 16 ++++++-
 builtin/add.c                | 30 +++++++++++--
 t/t3704-add-pathspec-file.sh | 86 ++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+), 5 deletions(-)
 create mode 100755 t/t3704-add-pathspec-file.sh

index 8b0e4c7fa8..be5e3ac54b 100644
@@ -11,7 +11,8 @@ SYNOPSIS
 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
 	  [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
 	  [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize]
-	  [--chmod=(+|-)x] [--] [<pathspec>...]
+	  [--chmod=(+|-)x] [--pathspec-from-file=<file> [--pathspec-file-nul]]
+	  [--] [<pathspec>...]
 
 DESCRIPTION
 -----------

@@ -187,6 +188,19 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
 	bit is only changed in the index, the files on disk are left
 	unchanged.
 
+--pathspec-from-file=<file>::
+	Pathspec is passed in `<file>` instead of commandline args. If
+	`<file>` is exactly `-` then standard input is used. Pathspec
+	elements are separated by LF or CR/LF. Pathspec elements can be
+	quoted as explained for the configuration variable `core.quotePath`
+	(see linkgit:git-config[1]). See also `--pathspec-file-nul` and
+	global `--literal-pathspecs`.
+
+--pathspec-file-nul::
+	Only meaningful with `--pathspec-from-file`. Pathspec elements are
+	separated with NUL character and all other characters are taken
+	literally (including newlines and quotes).
+
 \--::
 	This option can be used to separate command-line options from
 	the list of files, (useful when filenames might be mistaken

index 3d1791dd82..7c21ad492b 100644
@@ -29,6 +29,8 @@ static const char * const builtin_add_usage[] = {
 static int patch_interactive, add_interactive, edit_interactive;
 static int take_worktree_changes;
 static int add_renormalize;
+static int pathspec_file_nul;
+static const char *pathspec_from_file;
 
 struct update_callback_data {
 	int flags;

@@ -320,6 +322,8 @@ static struct option builtin_add_options[] = {
 		   N_("override the executable bit of the listed files")),
 	OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo,
 			N_("warn when adding an embedded repository")),
+	OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
+	OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
 	OPT_END(),
 };
 

@@ -414,11 +418,17 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);
 	if (patch_interactive)
 		add_interactive = 1;
-	if (add_interactive)
+	if (add_interactive) {
+		if (pathspec_from_file)
+			die(_("--pathspec-from-file is incompatible with --interactive/--patch"));
 		exit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive));
+	}
 
-	if (edit_interactive)
+	if (edit_interactive) {
+		if (pathspec_from_file)
+			die(_("--pathspec-from-file is incompatible with --edit"));
 		return(edit_patch(argc, argv, prefix));
+	}
 	argc--;
 	argv++;
 

@@ -451,13 +461,25 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
 
-	if (require_pathspec && argc == 0) {
+	if (pathspec_from_file) {
+		if (pathspec.nr)
+			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
+
+		parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
+				    PATHSPEC_PREFER_FULL |
+				    PATHSPEC_SYMLINK_LEADING_PATH,
+				    prefix, pathspec_from_file, pathspec_file_nul);
+	} else if (pathspec_file_nul) {
+		die(_("--pathspec-file-nul requires --pathspec-from-file"));
+	}
+
+	if (require_pathspec && pathspec.nr == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
 		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
 		return 0;
 	}
 
-	if (!take_worktree_changes && addremove_explicit < 0 && argc)
+	if (!take_worktree_changes && addremove_explicit < 0 && pathspec.nr)
 		/* Turn "git add pathspec..." to "git add -A pathspec..." */
 		addremove = 1;
 

gitgitgadget