t: rework tests for --pathspec-from-file

Please refer to commit messages for rationale.

This branch is a follow-up for [1] where part of branch was merged into master via [2].

Previously in [3] there were some concerns on whether removing copy&pasted tests is good. I still think that yes, it 's a good thing, mostly because of high volume of potential 13*6=78 duplicate tests.

Still, I separated this change as last patch, so that the remaining part of the branch can be taken without it.

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

Alexandr Miloslavskiy (3):
t: fix quotes tests for --pathspec-from-file
t: directly test parse_pathspec_file()
t: drop copy&pasted tests for --pathspec-from-file

 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/t2026-checkout-pathspec-file.sh   | 70 +----------------------
 t/t2072-restore-pathspec-file.sh    | 70 +----------------------
 t/t3704-add-pathspec-file.sh        | 70 +----------------------
 t/t7107-reset-pathspec-file.sh      | 79 +++----------------------
 t/t7526-commit-pathspec-file.sh     | 70 +----------------------
 10 files changed, 142 insertions(+), 343 deletions(-)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh

base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-503%2FSyntevoAlex%2F%230207(git)_2b_test_parse_directly-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-503/SyntevoAlex/#0207(git)_2b_test_parse_directly-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/503

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

While working on the next patch, 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 " being preserved which in turn prevented pathspec from matching files.

Fix this by properly escaping one more time.

---
 t/t2026-checkout-pathspec-file.sh | 9 +++++++--
 t/t2072-restore-pathspec-file.sh  | 9 +++++++--
 t/t3704-add-pathspec-file.sh      | 9 +++++++--
 t/t7107-reset-pathspec-file.sh    | 9 +++++++--
 t/t7526-commit-pathspec-file.sh   | 9 +++++++--
 5 files changed, 35 insertions(+), 10 deletions(-)

index f62fd27440..2dc8901bca 100755
@@ -109,7 +109,10 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	# git    takes    \101 and spits     A
+	printf "\"file\\\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
 
 	cat >expect <<-\EOF &&
 	M  fileA.t

@@ -120,7 +123,9 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	printf "\"file\\\\101.t\"" >list &&
 	test_must_fail git checkout --pathspec-from-file=list --pathspec-file-nul HEAD^1
 '
 

index db58e83735..70e95ef3b6 100755
@@ -109,7 +109,10 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git restore --pathspec-from-file=- --source=HEAD^1 &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	# git    takes    \101 and spits     A
+	printf "\"file\\\\101.t\"" | git restore --pathspec-from-file=- --source=HEAD^1 &&
 
 	cat >expect <<-\EOF &&
 	 M fileA.t

@@ -120,7 +123,9 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	printf "\"file\\\\101.t\"" >list &&
 	test_must_fail git restore --pathspec-from-file=list --pathspec-file-nul --source=HEAD^1
 '
 

index 3cfdb669b7..2e0141fcce 100755
@@ -97,7 +97,10 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git add --pathspec-from-file=- &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	# git    takes    \101 and spits     A
+	printf "\"file\\\\101.t\"" | git add --pathspec-from-file=- &&
 
 	cat >expect <<-\EOF &&
 	A  fileA.t

@@ -108,7 +111,9 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	printf "\"file\\\\101.t\"" >list &&
 	test_must_fail git add --pathspec-from-file=list --pathspec-file-nul
 '
 

index 6b1a731fff..52a44f033d 100755
@@ -105,8 +105,11 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	# git    takes    \101 and spits     A
 	git rm fileA.t &&
-	printf "\"file\\101.t\"" | git reset --pathspec-from-file=- &&
+	printf "\"file\\\\101.t\"" | git reset --pathspec-from-file=- &&
 
 	cat >expect <<-\EOF &&
 	 D fileA.t

@@ -117,8 +120,10 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
 	git rm fileA.t &&
-	printf "\"file\\101.t\"" >list &&
+	printf "\"file\\\\101.t\"" >list &&
 	# Note: "git reset" has not yet learned to fail on wrong pathspecs
 	git reset --pathspec-from-file=list --pathspec-file-nul &&
 

index 4b58901ed6..e7dc2ff8b1 100755
@@ -100,7 +100,10 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git commit --pathspec-from-file=- -m "Commit" &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	# git    takes    \101 and spits     A
+	printf "\"file\\\\101.t\"" | git commit --pathspec-from-file=- -m "Commit" &&
 
 	cat >expect <<-\EOF &&
 	A	fileA.t

@@ -111,7 +114,9 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	printf "\"file\\\\101.t\"" >list &&
 	test_must_fail git commit --pathspec-from-file=list --pathspec-file-nul -m "Commit"
 '
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Previously, parse_pathspec_file() was tested indirectly by invoking git commands with properly crafted inputs. As demonstrated by the previous bugfix, testing complicated black boxes indirectly can lead to tests that silently test the wrong thing.

Introduce direct tests for parse_pathspec_file().

---
 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 +++++++++++++++++++++++++++++
 5 files changed, 126 insertions(+)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh

index 09f98b777c..0061f96e8a 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

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

	printf "fileA.t\0fileB.t\0" |
	test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&

	cat >expect <<-\EOF &&
	fileA.t
	fileB.t
	EOF

	test_cmp expect actual

	cat >expect <<-\EOF &&
	fileA.t
	fileB.t
	EOF

	printf "fileA.t\0fileB.t\0" |
	test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&

	test_cmp expect actual

Thanks for having a look! I think I have fixed all mentioned issues in V2.

From: Alexandr Miloslavskiy <[email protected]>

With direct tests for parse_pathspec_file() already in place, it is not very reasonable to copy&paste 6 tests for parse_pathspec_file() for every git command that uses it (I counted 13 commands that could use it eventually).

I believe that indirect tests are redundant because I don't expect direct tests to ever disagree with indirect tests.

---
 t/t2026-checkout-pathspec-file.sh | 75 +--------------------------
 t/t2072-restore-pathspec-file.sh  | 75 +--------------------------
 t/t3704-add-pathspec-file.sh      | 75 +--------------------------
 t/t7107-reset-pathspec-file.sh    | 84 +++----------------------------
 t/t7526-commit-pathspec-file.sh   | 75 +--------------------------
 5 files changed, 16 insertions(+), 368 deletions(-)

index 2dc8901bca..559b4528d7 100755
@@ -35,7 +35,7 @@ verify_expect () {
 	test_cmp expect actual
 }
 
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
 	restore_checkpoint &&
 
 	echo fileA.t | git checkout --pathspec-from-file=- HEAD^1 &&

@@ -46,19 +46,7 @@ test_expect_success '--pathspec-from-file from stdin' '
 	verify_expect
 '
 
-test_expect_success '--pathspec-from-file from file' '
-	restore_checkpoint &&
-
-	echo fileA.t >list &&
-	git checkout --pathspec-from-file=list 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 checkout --pathspec-from-file=- --pathspec-file-nul HEAD^1 &&

@@ -70,65 +58,6 @@ test_expect_success 'NUL delimiters' '
 	verify_expect
 '
 
-test_expect_success 'LF delimiters' '
-	restore_checkpoint &&
-
-	printf "fileA.t\nfileB.t\n" | git checkout --pathspec-from-file=- 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 checkout --pathspec-from-file=- HEAD^1 &&
-
-	cat >expect <<-\EOF &&
-	M  fileA.t
-	M  fileB.t
-	EOF
-	verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
-	restore_checkpoint &&
-
-	printf "fileA.t\r\nfileB.t\r\n" | git checkout --pathspec-from-file=- HEAD^1 &&
-
-	cat >expect <<-\EOF &&
-	M  fileA.t
-	M  fileB.t
-	EOF
-	verify_expect
-'
-
-test_expect_success 'quotes' '
-	restore_checkpoint &&
-
-	# shell  takes \\\\101 and spits \\101
-	# printf takes   \\101 and spits  \101
-	# git    takes    \101 and spits     A
-	printf "\"file\\\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
-
-	cat >expect <<-\EOF &&
-	M  fileA.t
-	EOF
-	verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
-	restore_checkpoint &&
-
-	# shell  takes \\\\101 and spits \\101
-	# printf takes   \\101 and spits  \101
-	printf "\"file\\\\101.t\"" >list &&
-	test_must_fail git checkout --pathspec-from-file=list --pathspec-file-nul HEAD^1
-'
-
 test_expect_success 'only touches what was listed' '
 	restore_checkpoint &&
 

gitgitgadget

Please refer to commit messages for rationale.

This branch is a follow-up for [1] where part of branch was merged into master via [2].

Previously in [3] there were some concerns on whether removing copy&pasted tests is good. I still think that yes, it 's a good thing, mostly because of high volume of potential 13*6=78 duplicate tests.

Still, I separated this change as last patch, so that the remaining part of the branch can be taken without it.

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

Changes since V1
----------------
Small code formatting changes suggested in V1.

Alexandr Miloslavskiy (3):
t: fix quotes tests for --pathspec-from-file
t: directly test parse_pathspec_file()
t: drop copy&pasted tests for --pathspec-from-file

 Makefile                            |   1 +
 t/helper/test-parse-pathspec-file.c |  33 +++++++++
 t/helper/test-tool.c                |   1 +
 t/helper/test-tool.h                |   1 +
 t/t0067-parse_pathspec_file.sh      | 104 ++++++++++++++++++++++++++++
 t/t2026-checkout-pathspec-file.sh   |  70 +------------------
 t/t2072-restore-pathspec-file.sh    |  70 +------------------
 t/t3704-add-pathspec-file.sh        |  70 +------------------
 t/t7107-reset-pathspec-file.sh      |  79 +++------------------
 t/t7526-commit-pathspec-file.sh     |  70 +------------------
 10 files changed, 156 insertions(+), 343 deletions(-)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh

base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-503%2FSyntevoAlex%2F%230207(git)_2b_test_parse_directly-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-503/SyntevoAlex/#0207(git)_2b_test_parse_directly-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/503

Range-diff vs v1:

 1:  ab9519298d = 1:  6193dc7396 t: fix quotes tests for --pathspec-from-file
 2:  27383a5b08 ! 2:  ab449ac15a t: directly test parse_pathspec_file()
     @@ -55,9 +55,8 @@
      +	parse_pathspec_file(&pathspec, 0, 0, 0, pathspec_from_file,
      +			    pathspec_file_nul);
      +
     -+	for (i = 0; i < pathspec.nr; i++) {
     ++	for (i = 0; i < pathspec.nr; i++)
      +		printf("%s\n", pathspec.items[i].original);
     -+	}
      +
      +	clear_pathspec(&pathspec);
      +	return 0;
     @@ -99,84 +98,99 @@
      +. ./test-lib.sh
      +
      +test_expect_success 'one item from stdin' '
     -+	echo fileA.t | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
     -+
      +	cat >expect <<-\EOF &&
      +	fileA.t
      +	EOF
     ++
     ++	echo fileA.t |
     ++	test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
     ++
      +	test_cmp expect actual
      +'
      +
      +test_expect_success 'one item from file' '
     -+	echo fileA.t >list &&
     -+	test-tool parse-pathspec-file --pathspec-from-file=list >actual &&
     -+
      +	cat >expect <<-\EOF &&
      +	fileA.t
      +	EOF
     ++
     ++	echo fileA.t >list &&
     ++	test-tool parse-pathspec-file --pathspec-from-file=list >actual &&
     ++
      +	test_cmp expect actual
      +'
      +
      +test_expect_success 'NUL delimiters' '
     -+	printf "fileA.t\0fileB.t\0" | test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
     -+
      +	cat >expect <<-\EOF &&
      +	fileA.t
      +	fileB.t
      +	EOF
     ++
     ++	printf "fileA.t\0fileB.t\0" |
     ++	test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
     ++
      +	test_cmp expect actual
      +'
      +
      +test_expect_success 'LF delimiters' '
     -+	printf "fileA.t\nfileB.t\n" | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
     -+
      +	cat >expect <<-\EOF &&
      +	fileA.t
      +	fileB.t
      +	EOF
     ++
     ++	printf "fileA.t\nfileB.t\n" |
     ++	test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
     ++
      +	test_cmp expect actual
      +'
      +
      +test_expect_success 'no trailing delimiter' '
     -+	printf "fileA.t\nfileB.t" | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
     -+
      +	cat >expect <<-\EOF &&
      +	fileA.t
      +	fileB.t
      +	EOF
     ++
     ++	printf "fileA.t\nfileB.t" |
     ++	test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
     ++
      +	test_cmp expect actual
      +'
      +
      +test_expect_success 'CRLF delimiters' '
     -+	printf "fileA.t\r\nfileB.t\r\n" | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
     -+
      +	cat >expect <<-\EOF &&
      +	fileA.t
      +	fileB.t
      +	EOF
     ++
     ++	printf "fileA.t\r\nfileB.t\r\n" |
     ++	test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
     ++
      +	test_cmp expect actual
      +'
      +
      +test_expect_success 'quotes' '
     ++	cat >expect <<-\EOF &&
     ++	fileA.t
     ++	EOF
     ++
      +	# shell  takes \\\\101 and spits \\101
      +	# printf takes   \\101 and spits  \101
      +	# git    takes    \101 and spits     A
     -+	printf "\"file\\\\101.t\"" | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
     ++	printf "\"file\\\\101.t\"" |
     ++	test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
      +
     -+	cat >expect <<-\EOF &&
     -+	fileA.t
     -+	EOF
      +	test_cmp expect actual
      +'
      +
      +test_expect_success '--pathspec-file-nul takes quotes literally' '
     -+	# shell  takes \\\\101 and spits \\101
     -+	# printf takes   \\101 and spits  \101
     -+	printf "\"file\\\\101.t\"" | test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
     -+
      +	cat >expect <<-\EOF &&
      +	"file\101.t"
      +	EOF
     ++
     ++	# shell  takes \\\\101 and spits \\101
     ++	# printf takes   \\101 and spits  \101
     ++	printf "\"file\\\\101.t\"" |
     ++	test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
     ++
      +	test_cmp expect actual
      +'
      +

3: daef256db3 = 3: 88086cebce t: drop copy&pasted tests for --pathspec-from-file

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

While working on the next patch, 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 " being preserved which in turn prevented pathspec from matching files.

Fix this by properly escaping one more time.

---
 t/t2026-checkout-pathspec-file.sh | 9 +++++++--
 t/t2072-restore-pathspec-file.sh  | 9 +++++++--
 t/t3704-add-pathspec-file.sh      | 9 +++++++--
 t/t7107-reset-pathspec-file.sh    | 9 +++++++--
 t/t7526-commit-pathspec-file.sh   | 9 +++++++--
 5 files changed, 35 insertions(+), 10 deletions(-)

index f62fd27440..2dc8901bca 100755
@@ -109,7 +109,10 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	# git    takes    \101 and spits     A
+	printf "\"file\\\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
 
 	cat >expect <<-\EOF &&
 	M  fileA.t

@@ -120,7 +123,9 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	printf "\"file\\\\101.t\"" >list &&
 	test_must_fail git checkout --pathspec-from-file=list --pathspec-file-nul HEAD^1
 '
 

index db58e83735..70e95ef3b6 100755
@@ -109,7 +109,10 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git restore --pathspec-from-file=- --source=HEAD^1 &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	# git    takes    \101 and spits     A
+	printf "\"file\\\\101.t\"" | git restore --pathspec-from-file=- --source=HEAD^1 &&
 
 	cat >expect <<-\EOF &&
 	 M fileA.t

@@ -120,7 +123,9 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	printf "\"file\\\\101.t\"" >list &&
 	test_must_fail git restore --pathspec-from-file=list --pathspec-file-nul --source=HEAD^1
 '
 

index 3cfdb669b7..2e0141fcce 100755
@@ -97,7 +97,10 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git add --pathspec-from-file=- &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	# git    takes    \101 and spits     A
+	printf "\"file\\\\101.t\"" | git add --pathspec-from-file=- &&
 
 	cat >expect <<-\EOF &&
 	A  fileA.t

@@ -108,7 +111,9 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	printf "\"file\\\\101.t\"" >list &&
 	test_must_fail git add --pathspec-from-file=list --pathspec-file-nul
 '
 

index 6b1a731fff..52a44f033d 100755
@@ -105,8 +105,11 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	# git    takes    \101 and spits     A
 	git rm fileA.t &&
-	printf "\"file\\101.t\"" | git reset --pathspec-from-file=- &&
+	printf "\"file\\\\101.t\"" | git reset --pathspec-from-file=- &&
 
 	cat >expect <<-\EOF &&
 	 D fileA.t

@@ -117,8 +120,10 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
 	git rm fileA.t &&
-	printf "\"file\\101.t\"" >list &&
+	printf "\"file\\\\101.t\"" >list &&
 	# Note: "git reset" has not yet learned to fail on wrong pathspecs
 	git reset --pathspec-from-file=list --pathspec-file-nul &&
 

index 4b58901ed6..e7dc2ff8b1 100755
@@ -100,7 +100,10 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git commit --pathspec-from-file=- -m "Commit" &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	# git    takes    \101 and spits     A
+	printf "\"file\\\\101.t\"" | git commit --pathspec-from-file=- -m "Commit" &&
 
 	cat >expect <<-\EOF &&
 	A	fileA.t

@@ -111,7 +114,9 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	# shell  takes \\\\101 and spits \\101
+	# printf takes   \\101 and spits  \101
+	printf "\"file\\\\101.t\"" >list &&
 	test_must_fail git commit --pathspec-from-file=list --pathspec-file-nul -m "Commit"
 '
 

gitgitgadget

For portability, that would be

	printf "%s\n" "\"file\101.t\"" | ...

because some implementations of echo interpret escapes by default.

Thanks,
Jonathan

Luckily, trailing \n didn't matter much, and I could also send input via commandline argument instead of stdin, so here-doc is really the most readable solution here.

Fixed it in V3, thanks for your suggestion!

From: Alexandr Miloslavskiy <[email protected]>

Previously, parse_pathspec_file() was tested indirectly by invoking git commands with properly crafted inputs. As demonstrated by the previous bugfix, testing complicated black boxes indirectly can lead to tests that silently test the wrong thing.

Introduce direct tests for parse_pathspec_file().

---
 Makefile                            |   1 +
 t/helper/test-parse-pathspec-file.c |  33 +++++++++
 t/helper/test-tool.c                |   1 +
 t/helper/test-tool.h                |   1 +
 t/t0067-parse_pathspec_file.sh      | 104 ++++++++++++++++++++++++++++
 5 files changed, 140 insertions(+)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh

index 09f98b777c..0061f96e8a 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..02f4ccfd2a
@@ -0,0 +1,33 @@
+#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]>

With direct tests for parse_pathspec_file() already in place, it is not very reasonable to copy&paste 6 tests for parse_pathspec_file() for every git command that uses it (I counted 13 commands that could use it eventually).

I believe that indirect tests are redundant because I don't expect direct tests to ever disagree with indirect tests.

---
 t/t2026-checkout-pathspec-file.sh | 75 +--------------------------
 t/t2072-restore-pathspec-file.sh  | 75 +--------------------------
 t/t3704-add-pathspec-file.sh      | 75 +--------------------------
 t/t7107-reset-pathspec-file.sh    | 84 +++----------------------------
 t/t7526-commit-pathspec-file.sh   | 75 +--------------------------
 5 files changed, 16 insertions(+), 368 deletions(-)

index 2dc8901bca..559b4528d7 100755
@@ -35,7 +35,7 @@ verify_expect () {
 	test_cmp expect actual
 }
 
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
 	restore_checkpoint &&
 
 	echo fileA.t | git checkout --pathspec-from-file=- HEAD^1 &&

@@ -46,19 +46,7 @@ test_expect_success '--pathspec-from-file from stdin' '
 	verify_expect
 '
 
-test_expect_success '--pathspec-from-file from file' '
-	restore_checkpoint &&
-
-	echo fileA.t >list &&
-	git checkout --pathspec-from-file=list 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 checkout --pathspec-from-file=- --pathspec-file-nul HEAD^1 &&

@@ -70,65 +58,6 @@ test_expect_success 'NUL delimiters' '
 	verify_expect
 '
 
-test_expect_success 'LF delimiters' '
-	restore_checkpoint &&
-
-	printf "fileA.t\nfileB.t\n" | git checkout --pathspec-from-file=- 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 checkout --pathspec-from-file=- HEAD^1 &&
-
-	cat >expect <<-\EOF &&
-	M  fileA.t
-	M  fileB.t
-	EOF
-	verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
-	restore_checkpoint &&
-
-	printf "fileA.t\r\nfileB.t\r\n" | git checkout --pathspec-from-file=- HEAD^1 &&
-
-	cat >expect <<-\EOF &&
-	M  fileA.t
-	M  fileB.t
-	EOF
-	verify_expect
-'
-
-test_expect_success 'quotes' '
-	restore_checkpoint &&
-
-	# shell  takes \\\\101 and spits \\101
-	# printf takes   \\101 and spits  \101
-	# git    takes    \101 and spits     A
-	printf "\"file\\\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
-
-	cat >expect <<-\EOF &&
-	M  fileA.t
-	EOF
-	verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
-	restore_checkpoint &&
-
-	# shell  takes \\\\101 and spits \\101
-	# printf takes   \\101 and spits  \101
-	printf "\"file\\\\101.t\"" >list &&
-	test_must_fail git checkout --pathspec-from-file=list --pathspec-file-nul HEAD^1
-'
-
 test_expect_success 'only touches what was listed' '
 	restore_checkpoint &&
 

gitgitgadget

Please refer to commit messages for rationale.

This branch is a follow-up for [1] where part of branch was merged into master via [2].

Previously in [3] there were some concerns on whether removing copy&pasted tests is good. I still think that yes, it 's a good thing, mostly because of high volume of potential 13*6=78 duplicate tests.

Still, I separated this change as last patch, so that the remaining part of the branch can be taken without it.

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

Changes since V1
----------------
Small code formatting changes suggested in V1.

Alexandr Miloslavskiy (3):
t: fix quotes tests for --pathspec-from-file
t: directly test parse_pathspec_file()
t: drop copy&pasted tests for --pathspec-from-file

 Makefile                            |   1 +
 t/helper/test-parse-pathspec-file.c |  33 +++++++++
 t/helper/test-tool.c                |   1 +
 t/helper/test-tool.h                |   1 +
 t/t0067-parse_pathspec_file.sh      | 108 ++++++++++++++++++++++++++++
 t/t2026-checkout-pathspec-file.sh   |  70 +-----------------
 t/t2072-restore-pathspec-file.sh    |  70 +-----------------
 t/t3704-add-pathspec-file.sh        |  70 +-----------------
 t/t7107-reset-pathspec-file.sh      |  79 +++-----------------
 t/t7526-commit-pathspec-file.sh     |  70 +-----------------
 10 files changed, 160 insertions(+), 343 deletions(-)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh

base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-503%2FSyntevoAlex%2F%230207(git)_2b_test_parse_directly-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-503/SyntevoAlex/#0207(git)_2b_test_parse_directly-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/503

Range-diff vs v2:

1: 6193dc7396 ! 1: 88790669ce t: fix quotes tests for --pathspec-from-file

     @@ -7,7 +7,7 @@
          I had to escape it two times! Tests still worked due to `"` being
          preserved which in turn prevented pathspec from matching files.
      
     -    Fix this by properly escaping one more time.
     +    Fix this by using here-doc instead.
      
          Signed-off-by: Alexandr Miloslavskiy <[email protected]>
      
     @@ -19,10 +19,11 @@
       	restore_checkpoint &&
       
      -	printf "\"file\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
     -+	# shell  takes \\\\101 and spits \\101
     -+	# printf takes   \\101 and spits  \101
     -+	# git    takes    \101 and spits     A
     -+	printf "\"file\\\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
     ++	cat >list <<-\EOF &&
     ++	"file\101.t"
     ++	EOF
     ++
     ++	git checkout --pathspec-from-file=list HEAD^1 &&
       
       	cat >expect <<-\EOF &&
       	M  fileA.t
     @@ -31,9 +32,10 @@
       	restore_checkpoint &&
       
      -	printf "\"file\\101.t\"" >list &&
     -+	# shell  takes \\\\101 and spits \\101
     -+	# printf takes   \\101 and spits  \101
     -+	printf "\"file\\\\101.t\"" >list &&
     ++	cat >list <<-\EOF &&
     ++	"file\101.t"
     ++	EOF
     ++
       	test_must_fail git checkout --pathspec-from-file=list --pathspec-file-nul HEAD^1
       '
       
     @@ -46,10 +48,11 @@
       	restore_checkpoint &&
       
      -	printf "\"file\\101.t\"" | git restore --pathspec-from-file=- --source=HEAD^1 &&
     -+	# shell  takes \\\\101 and spits \\101
     -+	# printf takes   \\101 and spits  \101
     -+	# git    takes    \101 and spits     A
     -+	printf "\"file\\\\101.t\"" | git restore --pathspec-from-file=- --source=HEAD^1 &&
     ++	cat >list <<-\EOF &&
     ++	"file\101.t"
     ++	EOF
     ++
     ++	git restore --pathspec-from-file=list --source=HEAD^1 &&
       
       	cat >expect <<-\EOF &&
       	 M fileA.t
     @@ -58,9 +61,10 @@
       	restore_checkpoint &&
       
      -	printf "\"file\\101.t\"" >list &&
     -+	# shell  takes \\\\101 and spits \\101
     -+	# printf takes   \\101 and spits  \101
     -+	printf "\"file\\\\101.t\"" >list &&
     ++	cat >list <<-\EOF &&
     ++	"file\101.t"
     ++	EOF
     ++
       	test_must_fail git restore --pathspec-from-file=list --pathspec-file-nul --source=HEAD^1
       '
       
     @@ -73,10 +77,11 @@
       	restore_checkpoint &&
       
      -	printf "\"file\\101.t\"" | git add --pathspec-from-file=- &&
     -+	# shell  takes \\\\101 and spits \\101
     -+	# printf takes   \\101 and spits  \101
     -+	# git    takes    \101 and spits     A
     -+	printf "\"file\\\\101.t\"" | git add --pathspec-from-file=- &&
     ++	cat >list <<-\EOF &&
     ++	"file\101.t"
     ++	EOF
     ++
     ++	git add --pathspec-from-file=list &&
       
       	cat >expect <<-\EOF &&
       	A  fileA.t
     @@ -85,9 +90,10 @@
       	restore_checkpoint &&
       
      -	printf "\"file\\101.t\"" >list &&
     -+	# shell  takes \\\\101 and spits \\101
     -+	# printf takes   \\101 and spits  \101
     -+	printf "\"file\\\\101.t\"" >list &&
     ++	cat >list <<-\EOF &&
     ++	"file\101.t"
     ++	EOF
     ++
       	test_must_fail git add --pathspec-from-file=list --pathspec-file-nul
       '
       
     @@ -99,12 +105,13 @@
       test_expect_success 'quotes' '
       	restore_checkpoint &&
       
     -+	# shell  takes \\\\101 and spits \\101
     -+	# printf takes   \\101 and spits  \101
     -+	# git    takes    \101 and spits     A
     ++	cat >list <<-\EOF &&
     ++	"file\101.t"
     ++	EOF
     ++
       	git rm fileA.t &&
      -	printf "\"file\\101.t\"" | git reset --pathspec-from-file=- &&
     -+	printf "\"file\\\\101.t\"" | git reset --pathspec-from-file=- &&
     ++	git reset --pathspec-from-file=list &&
       
       	cat >expect <<-\EOF &&
       	 D fileA.t
     @@ -112,11 +119,12 @@
       test_expect_success 'quotes not compatible with --pathspec-file-nul' '
       	restore_checkpoint &&
       
     -+	# shell  takes \\\\101 and spits \\101
     -+	# printf takes   \\101 and spits  \101
     - 	git rm fileA.t &&
     +-	git rm fileA.t &&
      -	printf "\"file\\101.t\"" >list &&
     -+	printf "\"file\\\\101.t\"" >list &&
     ++	cat >list <<-\EOF &&
     ++	"file\101.t"
     ++	EOF
     ++
       	# Note: "git reset" has not yet learned to fail on wrong pathspecs
       	git reset --pathspec-from-file=list --pathspec-file-nul &&
       
     @@ -129,10 +137,11 @@
       	restore_checkpoint &&
       
      -	printf "\"file\\101.t\"" | git commit --pathspec-from-file=- -m "Commit" &&
     -+	# shell  takes \\\\101 and spits \\101
     -+	# printf takes   \\101 and spits  \101
     -+	# git    takes    \101 and spits     A
     -+	printf "\"file\\\\101.t\"" | git commit --pathspec-from-file=- -m "Commit" &&
     ++	cat >list <<-\EOF &&
     ++	"file\101.t"
     ++	EOF
     ++
     ++	git commit --pathspec-from-file=list -m "Commit" &&
       
       	cat >expect <<-\EOF &&
       	A	fileA.t
     @@ -141,9 +150,10 @@
       	restore_checkpoint &&
       
      -	printf "\"file\\101.t\"" >list &&
     -+	# shell  takes \\\\101 and spits \\101
     -+	# printf takes   \\101 and spits  \101
     -+	printf "\"file\\\\101.t\"" >list &&
     ++	cat >list <<-\EOF &&
     ++	"file\101.t"
     ++	EOF
     ++
       	test_must_fail git commit --pathspec-from-file=list --pathspec-file-nul -m "Commit"
       '

2: ab449ac15a ! 2: 68925c2712 t: directly test parse_pathspec_file()

     @@ -172,24 +172,28 @@
      +	fileA.t
      +	EOF
      +
     -+	# shell  takes \\\\101 and spits \\101
     -+	# printf takes   \\101 and spits  \101
     -+	# git    takes    \101 and spits     A
     -+	printf "\"file\\\\101.t\"" |
     -+	test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
     ++	cat >list <<-\EOF &&
     ++	"file\101.t"
     ++	EOF
     ++
     ++	test-tool parse-pathspec-file --pathspec-from-file=list >actual &&
      +
      +	test_cmp expect actual
      +'
      +
      +test_expect_success '--pathspec-file-nul takes quotes literally' '
     ++	# Note: there is an extra newline because --pathspec-file-nul takes
     ++	# input \n literally, too
      +	cat >expect <<-\EOF &&
      +	"file\101.t"
     ++
      +	EOF
      +
     -+	# shell  takes \\\\101 and spits \\101
     -+	# printf takes   \\101 and spits  \101
     -+	printf "\"file\\\\101.t\"" |
     -+	test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
     ++	cat >list <<-\EOF &&
     ++	"file\101.t"
     ++	EOF
     ++
     ++	test-tool parse-pathspec-file --pathspec-from-file=list --pathspec-file-nul >actual &&
      +
      +	test_cmp expect actual
      +'
 3:  88086cebce ! 3:  f71021b0dd t: drop copy&pasted tests for --pathspec-from-file
     @@ -88,10 +88,11 @@
      -test_expect_success 'quotes' '
      -	restore_checkpoint &&
      -
     --	# shell  takes \\\\101 and spits \\101
     --	# printf takes   \\101 and spits  \101
     --	# git    takes    \101 and spits     A
     --	printf "\"file\\\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
     +-	cat >list <<-\EOF &&
     +-	"file\101.t"
     +-	EOF
     +-
     +-	git checkout --pathspec-from-file=list HEAD^1 &&
      -
      -	cat >expect <<-\EOF &&
      -	M  fileA.t
     @@ -102,9 +103,10 @@
      -test_expect_success 'quotes not compatible with --pathspec-file-nul' '
      -	restore_checkpoint &&
      -
     --	# shell  takes \\\\101 and spits \\101
     --	# printf takes   \\101 and spits  \101
     --	printf "\"file\\\\101.t\"" >list &&
     +-	cat >list <<-\EOF &&
     +-	"file\101.t"
     +-	EOF
     +-
      -	test_must_fail git checkout --pathspec-from-file=list --pathspec-file-nul HEAD^1
      -'
      -
     @@ -188,10 +190,11 @@
      -test_expect_success 'quotes' '
      -	restore_checkpoint &&
      -
     --	# shell  takes \\\\101 and spits \\101
     --	# printf takes   \\101 and spits  \101
     --	# git    takes    \101 and spits     A
     --	printf "\"file\\\\101.t\"" | git restore --pathspec-from-file=- --source=HEAD^1 &&
     +-	cat >list <<-\EOF &&
     +-	"file\101.t"
     +-	EOF
     +-
     +-	git restore --pathspec-from-file=list --source=HEAD^1 &&
      -
      -	cat >expect <<-\EOF &&
      -	 M fileA.t
     @@ -202,9 +205,10 @@
      -test_expect_success 'quotes not compatible with --pathspec-file-nul' '
      -	restore_checkpoint &&
      -
     --	# shell  takes \\\\101 and spits \\101
     --	# printf takes   \\101 and spits  \101
     --	printf "\"file\\\\101.t\"" >list &&
     +-	cat >list <<-\EOF &&
     +-	"file\101.t"
     +-	EOF
     +-
      -	test_must_fail git restore --pathspec-from-file=list --pathspec-file-nul --source=HEAD^1
      -'
      -
     @@ -288,10 +292,11 @@
      -test_expect_success 'quotes' '
      -	restore_checkpoint &&
      -
     --	# shell  takes \\\\101 and spits \\101
     --	# printf takes   \\101 and spits  \101
     --	# git    takes    \101 and spits     A
     --	printf "\"file\\\\101.t\"" | git add --pathspec-from-file=- &&
     +-	cat >list <<-\EOF &&
     +-	"file\101.t"
     +-	EOF
     +-
     +-	git add --pathspec-from-file=list &&
      -
      -	cat >expect <<-\EOF &&
      -	A  fileA.t
     @@ -302,9 +307,10 @@
      -test_expect_success 'quotes not compatible with --pathspec-file-nul' '
      -	restore_checkpoint &&
      -
     --	# shell  takes \\\\101 and spits \\101
     --	# printf takes   \\101 and spits  \101
     --	printf "\"file\\\\101.t\"" >list &&
     +-	cat >list <<-\EOF &&
     +-	"file\101.t"
     +-	EOF
     +-
      -	test_must_fail git add --pathspec-from-file=list --pathspec-file-nul
      -'
      -
     @@ -398,11 +404,12 @@
      -test_expect_success 'quotes' '
      -	restore_checkpoint &&
      -
     --	# shell  takes \\\\101 and spits \\101
     --	# printf takes   \\101 and spits  \101
     --	# git    takes    \101 and spits     A
     +-	cat >list <<-\EOF &&
     +-	"file\101.t"
     +-	EOF
     +-
      -	git rm fileA.t &&
     --	printf "\"file\\\\101.t\"" | git reset --pathspec-from-file=- &&
     +-	git reset --pathspec-from-file=list &&
      -
      -	cat >expect <<-\EOF &&
      -	 D fileA.t
     @@ -413,10 +420,10 @@
      -test_expect_success 'quotes not compatible with --pathspec-file-nul' '
      -	restore_checkpoint &&
      -
     --	# shell  takes \\\\101 and spits \\101
     --	# printf takes   \\101 and spits  \101
     --	git rm fileA.t &&
     --	printf "\"file\\\\101.t\"" >list &&
     +-	cat >list <<-\EOF &&
     +-	"file\101.t"
     +-	EOF
     +-
      -	# Note: "git reset" has not yet learned to fail on wrong pathspecs
      -	git reset --pathspec-from-file=list --pathspec-file-nul &&
      -
     @@ -506,10 +513,11 @@
      -test_expect_success 'quotes' '
      -	restore_checkpoint &&
      -
     --	# shell  takes \\\\101 and spits \\101
     --	# printf takes   \\101 and spits  \101
     --	# git    takes    \101 and spits     A
     --	printf "\"file\\\\101.t\"" | git commit --pathspec-from-file=- -m "Commit" &&
     +-	cat >list <<-\EOF &&
     +-	"file\101.t"
     +-	EOF
     +-
     +-	git commit --pathspec-from-file=list -m "Commit" &&
      -
      -	cat >expect <<-\EOF &&
      -	A	fileA.t
     @@ -520,9 +528,10 @@
      -test_expect_success 'quotes not compatible with --pathspec-file-nul' '
      -	restore_checkpoint &&
      -
     --	# shell  takes \\\\101 and spits \\101
     --	# printf takes   \\101 and spits  \101
     --	printf "\"file\\\\101.t\"" >list &&
     +-	cat >list <<-\EOF &&
     +-	"file\101.t"
     +-	EOF
     +-
      -	test_must_fail git commit --pathspec-from-file=list --pathspec-file-nul -m "Commit"
      -'
      -

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

While working on the next patch, 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 " being preserved which in turn prevented pathspec from matching files.

Fix this by using here-doc instead.

---
 t/t2026-checkout-pathspec-file.sh | 11 +++++++++--
 t/t2072-restore-pathspec-file.sh  | 11 +++++++++--
 t/t3704-add-pathspec-file.sh      | 11 +++++++++--
 t/t7107-reset-pathspec-file.sh    | 12 +++++++++---
 t/t7526-commit-pathspec-file.sh   | 11 +++++++++--
 5 files changed, 45 insertions(+), 11 deletions(-)

index f62fd27440..adad71f631 100755
@@ -109,7 +109,11 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
+	git checkout --pathspec-from-file=list HEAD^1 &&
 
 	cat >expect <<-\EOF &&
 	M  fileA.t

@@ -120,7 +124,10 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
 	test_must_fail git checkout --pathspec-from-file=list --pathspec-file-nul HEAD^1
 '
 

index db58e83735..b407f6b779 100755
@@ -109,7 +109,11 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git restore --pathspec-from-file=- --source=HEAD^1 &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
+	git restore --pathspec-from-file=list --source=HEAD^1 &&
 
 	cat >expect <<-\EOF &&
 	 M fileA.t

@@ -120,7 +124,10 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
 	test_must_fail git restore --pathspec-from-file=list --pathspec-file-nul --source=HEAD^1
 '
 

index 3cfdb669b7..61b6e51009 100755
@@ -97,7 +97,11 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git add --pathspec-from-file=- &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
+	git add --pathspec-from-file=list &&
 
 	cat >expect <<-\EOF &&
 	A  fileA.t

@@ -108,7 +112,10 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
 	test_must_fail git add --pathspec-from-file=list --pathspec-file-nul
 '
 

index 6b1a731fff..b0e84cdb42 100755
@@ -105,8 +105,12 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
 	git rm fileA.t &&
-	printf "\"file\\101.t\"" | git reset --pathspec-from-file=- &&
+	git reset --pathspec-from-file=list &&
 
 	cat >expect <<-\EOF &&
 	 D fileA.t

@@ -117,8 +121,10 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	git rm fileA.t &&
-	printf "\"file\\101.t\"" >list &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
 	# Note: "git reset" has not yet learned to fail on wrong pathspecs
 	git reset --pathspec-from-file=list --pathspec-file-nul &&
 

index 4b58901ed6..4a7c11368d 100755
@@ -100,7 +100,11 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git commit --pathspec-from-file=- -m "Commit" &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
+	git commit --pathspec-from-file=list -m "Commit" &&
 
 	cat >expect <<-\EOF &&
 	A	fileA.t

@@ -111,7 +115,10 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
 	test_must_fail git commit --pathspec-from-file=list --pathspec-file-nul -m "Commit"
 '
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Previously, parse_pathspec_file() was tested indirectly by invoking git commands with properly crafted inputs. As demonstrated by the previous bugfix, testing complicated black boxes indirectly can lead to tests that silently test the wrong thing.

Introduce direct tests for parse_pathspec_file().

---
 Makefile                            |   1 +
 t/helper/test-parse-pathspec-file.c |  33 +++++++++
 t/helper/test-tool.c                |   1 +
 t/helper/test-tool.h                |   1 +
 t/t0067-parse_pathspec_file.sh      | 108 ++++++++++++++++++++++++++++
 5 files changed, 144 insertions(+)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh

index 09f98b777c..0061f96e8a 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..02f4ccfd2a
@@ -0,0 +1,33 @@
+#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]>

With direct tests for parse_pathspec_file() already in place, it is not very reasonable to copy&paste 6 tests for parse_pathspec_file() for every git command that uses it (I counted 13 commands that could use it eventually).

I believe that indirect tests are redundant because I don't expect direct tests to ever disagree with indirect tests.

---
 t/t2026-checkout-pathspec-file.sh | 77 +---------------------------
 t/t2072-restore-pathspec-file.sh  | 77 +---------------------------
 t/t3704-add-pathspec-file.sh      | 77 +---------------------------
 t/t7107-reset-pathspec-file.sh    | 85 +++----------------------------
 t/t7526-commit-pathspec-file.sh   | 77 +---------------------------
 5 files changed, 16 insertions(+), 377 deletions(-)

index adad71f631..559b4528d7 100755
@@ -35,7 +35,7 @@ verify_expect () {
 	test_cmp expect actual
 }
 
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
 	restore_checkpoint &&
 
 	echo fileA.t | git checkout --pathspec-from-file=- HEAD^1 &&

@@ -46,19 +46,7 @@ test_expect_success '--pathspec-from-file from stdin' '
 	verify_expect
 '
 
-test_expect_success '--pathspec-from-file from file' '
-	restore_checkpoint &&
-
-	echo fileA.t >list &&
-	git checkout --pathspec-from-file=list 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 checkout --pathspec-from-file=- --pathspec-file-nul HEAD^1 &&

@@ -70,67 +58,6 @@ test_expect_success 'NUL delimiters' '
 	verify_expect
 '
 
-test_expect_success 'LF delimiters' '
-	restore_checkpoint &&
-
-	printf "fileA.t\nfileB.t\n" | git checkout --pathspec-from-file=- 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 checkout --pathspec-from-file=- HEAD^1 &&
-
-	cat >expect <<-\EOF &&
-	M  fileA.t
-	M  fileB.t
-	EOF
-	verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
-	restore_checkpoint &&
-
-	printf "fileA.t\r\nfileB.t\r\n" | git checkout --pathspec-from-file=- HEAD^1 &&
-
-	cat >expect <<-\EOF &&
-	M  fileA.t
-	M  fileB.t
-	EOF
-	verify_expect
-'
-
-test_expect_success 'quotes' '
-	restore_checkpoint &&
-
-	cat >list <<-\EOF &&
-	"file\101.t"
-	EOF
-
-	git checkout --pathspec-from-file=list HEAD^1 &&
-
-	cat >expect <<-\EOF &&
-	M  fileA.t
-	EOF
-	verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
-	restore_checkpoint &&
-
-	cat >list <<-\EOF &&
-	"file\101.t"
-	EOF
-
-	test_must_fail git checkout --pathspec-from-file=list --pathspec-file-nul HEAD^1
-'
-
 test_expect_success 'only touches what was listed' '
 	restore_checkpoint &&
 

gitgitgadget

Please refer to commit messages for rationale.

This branch is a follow-up for [1] where part of branch was merged into master via [2].

Previously in [3] there were some concerns on whether removing copy&pasted tests is good. I still think that yes, it 's a good thing, mostly because of high volume of potential 13*6=78 duplicate tests.

Still, I separated this change as last patch, so that the remaining part of the branch can be taken without it.

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

Changes since V1
----------------
Small code formatting changes suggested in V1.

Changes since V2
----------------
Changed \\\\ escaping to use here-doc instead.

Changes since V3
----------------
Slightly improved commit message.

Alexandr Miloslavskiy (3):
t: fix quotes tests for --pathspec-from-file
t: directly test parse_pathspec_file()
t: drop copy&pasted tests for --pathspec-from-file

 Makefile                            |   1 +
 t/helper/test-parse-pathspec-file.c |  33 +++++++++
 t/helper/test-tool.c                |   1 +
 t/helper/test-tool.h                |   1 +
 t/t0067-parse_pathspec_file.sh      | 108 ++++++++++++++++++++++++++++
 t/t2026-checkout-pathspec-file.sh   |  70 +-----------------
 t/t2072-restore-pathspec-file.sh    |  70 +-----------------
 t/t3704-add-pathspec-file.sh        |  70 +-----------------
 t/t7107-reset-pathspec-file.sh      |  79 +++-----------------
 t/t7526-commit-pathspec-file.sh     |  70 +-----------------
 10 files changed, 160 insertions(+), 343 deletions(-)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh

base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-503%2FSyntevoAlex%2F%230207(git)_2b_test_parse_directly-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-503/SyntevoAlex/#0207(git)_2b_test_parse_directly-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/503

Range-diff vs v3:

 1:  88790669ce = 1:  ce0c592bb4 t: fix quotes tests for --pathspec-from-file
 2:  68925c2712 = 2:  8748f3baf1 t: directly test parse_pathspec_file()
 3:  f71021b0dd ! 3:  d02a1eac0b t: drop copy&pasted tests for --pathspec-from-file

@@ -3,9 +3,9 @@
t: drop copy&pasted tests for --pathspec-from-file

With direct tests for parse_pathspec_file() already in place, it is
- not very reasonable to copy&paste 6 tests for parse_pathspec_file()
- for every git command that uses it (I counted 13 commands that could use
- it eventually).
+ not very reasonable to copy&paste 6 similar indirect tests for every git
+ command that uses parse_pathspec_file(). I counted 13 potential git
+ commands, which could eventually lead to 6*13=78 duplicate tests.

I believe that indirect tests are redundant because I don't expect direct tests to ever disagree with indirect tests.

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

While working on the next patch, 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 " being preserved which in turn prevented pathspec from matching files.

Fix this by using here-doc instead.

---
 t/t2026-checkout-pathspec-file.sh | 11 +++++++++--
 t/t2072-restore-pathspec-file.sh  | 11 +++++++++--
 t/t3704-add-pathspec-file.sh      | 11 +++++++++--
 t/t7107-reset-pathspec-file.sh    | 12 +++++++++---
 t/t7526-commit-pathspec-file.sh   | 11 +++++++++--
 5 files changed, 45 insertions(+), 11 deletions(-)

index f62fd27440..adad71f631 100755
@@ -109,7 +109,11 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
+	git checkout --pathspec-from-file=list HEAD^1 &&
 
 	cat >expect <<-\EOF &&
 	M  fileA.t

@@ -120,7 +124,10 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
 	test_must_fail git checkout --pathspec-from-file=list --pathspec-file-nul HEAD^1
 '
 

index db58e83735..b407f6b779 100755
@@ -109,7 +109,11 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git restore --pathspec-from-file=- --source=HEAD^1 &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
+	git restore --pathspec-from-file=list --source=HEAD^1 &&
 
 	cat >expect <<-\EOF &&
 	 M fileA.t

@@ -120,7 +124,10 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
 	test_must_fail git restore --pathspec-from-file=list --pathspec-file-nul --source=HEAD^1
 '
 

index 3cfdb669b7..61b6e51009 100755
@@ -97,7 +97,11 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git add --pathspec-from-file=- &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
+	git add --pathspec-from-file=list &&
 
 	cat >expect <<-\EOF &&
 	A  fileA.t

@@ -108,7 +112,10 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
 	test_must_fail git add --pathspec-from-file=list --pathspec-file-nul
 '
 

index 6b1a731fff..b0e84cdb42 100755
@@ -105,8 +105,12 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
 	git rm fileA.t &&
-	printf "\"file\\101.t\"" | git reset --pathspec-from-file=- &&
+	git reset --pathspec-from-file=list &&
 
 	cat >expect <<-\EOF &&
 	 D fileA.t

@@ -117,8 +121,10 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	git rm fileA.t &&
-	printf "\"file\\101.t\"" >list &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
 	# Note: "git reset" has not yet learned to fail on wrong pathspecs
 	git reset --pathspec-from-file=list --pathspec-file-nul &&
 

index 4b58901ed6..4a7c11368d 100755
@@ -100,7 +100,11 @@ test_expect_success 'CRLF delimiters' '
 test_expect_success 'quotes' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" | git commit --pathspec-from-file=- -m "Commit" &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
+	git commit --pathspec-from-file=list -m "Commit" &&
 
 	cat >expect <<-\EOF &&
 	A	fileA.t

@@ -111,7 +115,10 @@ test_expect_success 'quotes' '
 test_expect_success 'quotes not compatible with --pathspec-file-nul' '
 	restore_checkpoint &&
 
-	printf "\"file\\101.t\"" >list &&
+	cat >list <<-\EOF &&
+	"file\101.t"
+	EOF
+
 	test_must_fail git commit --pathspec-from-file=list --pathspec-file-nul -m "Commit"
 '
 

gitgitgadget

From: Alexandr Miloslavskiy <[email protected]>

Previously, parse_pathspec_file() was tested indirectly by invoking git commands with properly crafted inputs. As demonstrated by the previous bugfix, testing complicated black boxes indirectly can lead to tests that silently test the wrong thing.

Introduce direct tests for parse_pathspec_file().

---
 Makefile                            |   1 +
 t/helper/test-parse-pathspec-file.c |  33 +++++++++
 t/helper/test-tool.c                |   1 +
 t/helper/test-tool.h                |   1 +
 t/t0067-parse_pathspec_file.sh      | 108 ++++++++++++++++++++++++++++
 5 files changed, 144 insertions(+)
 create mode 100644 t/helper/test-parse-pathspec-file.c
 create mode 100755 t/t0067-parse_pathspec_file.sh

index 09f98b777c..0061f96e8a 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..02f4ccfd2a
@@ -0,0 +1,33 @@
+#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]>

With direct tests for parse_pathspec_file() already in place, it is not very reasonable to copy&paste 6 similar indirect tests for every git command that uses parse_pathspec_file(). I counted 13 potential git commands, which could eventually lead to 6*13=78 duplicate tests.

I believe that indirect tests are redundant because I don't expect direct tests to ever disagree with indirect tests.

---
 t/t2026-checkout-pathspec-file.sh | 77 +---------------------------
 t/t2072-restore-pathspec-file.sh  | 77 +---------------------------
 t/t3704-add-pathspec-file.sh      | 77 +---------------------------
 t/t7107-reset-pathspec-file.sh    | 85 +++----------------------------
 t/t7526-commit-pathspec-file.sh   | 77 +---------------------------
 5 files changed, 16 insertions(+), 377 deletions(-)

index adad71f631..559b4528d7 100755
@@ -35,7 +35,7 @@ verify_expect () {
 	test_cmp expect actual
 }
 
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
 	restore_checkpoint &&
 
 	echo fileA.t | git checkout --pathspec-from-file=- HEAD^1 &&

@@ -46,19 +46,7 @@ test_expect_success '--pathspec-from-file from stdin' '
 	verify_expect
 '
 
-test_expect_success '--pathspec-from-file from file' '
-	restore_checkpoint &&
-
-	echo fileA.t >list &&
-	git checkout --pathspec-from-file=list 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 checkout --pathspec-from-file=- --pathspec-file-nul HEAD^1 &&

@@ -70,67 +58,6 @@ test_expect_success 'NUL delimiters' '
 	verify_expect
 '
 
-test_expect_success 'LF delimiters' '
-	restore_checkpoint &&
-
-	printf "fileA.t\nfileB.t\n" | git checkout --pathspec-from-file=- 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 checkout --pathspec-from-file=- HEAD^1 &&
-
-	cat >expect <<-\EOF &&
-	M  fileA.t
-	M  fileB.t
-	EOF
-	verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
-	restore_checkpoint &&
-
-	printf "fileA.t\r\nfileB.t\r\n" | git checkout --pathspec-from-file=- HEAD^1 &&
-
-	cat >expect <<-\EOF &&
-	M  fileA.t
-	M  fileB.t
-	EOF
-	verify_expect
-'
-
-test_expect_success 'quotes' '
-	restore_checkpoint &&
-
-	cat >list <<-\EOF &&
-	"file\101.t"
-	EOF
-
-	git checkout --pathspec-from-file=list HEAD^1 &&
-
-	cat >expect <<-\EOF &&
-	M  fileA.t
-	EOF
-	verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
-	restore_checkpoint &&
-
-	cat >list <<-\EOF &&
-	"file\101.t"
-	EOF
-
-	test_must_fail git checkout --pathspec-from-file=list --pathspec-file-nul HEAD^1
-'
-
 test_expect_success 'only touches what was listed' '
 	restore_checkpoint &&
 

gitgitgadget

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

With the third step the series won't merge cleanly with other topic you have in 'next' (t7107 gets somewhat heavy merge conflicts).

I'll queue the first two for now but let's clean them up post 2.25 release.

Thanks.

OK, I will re-submit the remaining patch after 2.25.

I will implement the next --pathspec-from-file patches as if this third patch was accepted (that is, without copy&pasted tests).

Thanks for accepting this and other polishing branches, I was already quite pessimistic about them.

I am not sure if that is a good idea. I'd rather see the planned new changes not to be taken hostage of the third step.

Besides, with the third step, your preference is not to test the behaviour of end-user facing commands that would learn the option at all and only test the underlying machinery with test-tool tests, no? If you are not adding tests for the higher-level end-user facing commands as part of these new series, would it make a difference if the codebase has the third step applied (i.e. missing tests for the end-user facing commands that have already learned the option) or not (i.e. the commands that have already learned the option are still tested end-to-end)?

In my understanding, the new patches will not be taken hostage, they will simply adopt the new approach. Everything will work just fine whether or not third step is present.

That's not exactly correct. Third step removes duplicate tests that give no real benefit. With test-tool tests in place and succceeding, these duplicate tests are super unlikely to fail.

I will still provide a few tests for every new command to make sure that said command works as intended. I will only skip indirectly testing global API again and again.

I will be adding good tests and skip useless tests. For new commands, it doesn't really matter if "third step" patch is applied or not.

Ah, OK. Then leaving those removed by the third step there may get in the way. So let's assume that we'll have an updated third step already applied and your new series are written on top of it.

OK, again. Thanks for a clarification.