* [PATCH v4 3/3] t: drop copy&pasted tests for --pathspec-from-file
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-31 10:15 UTC (permalink / raw)
To: git
Cc: Jonathan Nieder, Eric Sunshine, Alexandr Miloslavskiy,
Junio C Hamano, Alexandr Miloslavskiy
In-Reply-To: <pull.503.v4.git.1577787313.gitgitgadget@gmail.com>
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
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.
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
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(-)
diff --git a/t/t2026-checkout-pathspec-file.sh b/t/t2026-checkout-pathspec-file.sh
index adad71f631..559b4528d7 100755
--- a/t/t2026-checkout-pathspec-file.sh
+++ b/t/t2026-checkout-pathspec-file.sh
@@ -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 &&
diff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh
index b407f6b779..9b3125d582 100755
--- a/t/t2072-restore-pathspec-file.sh
+++ b/t/t2072-restore-pathspec-file.sh
@@ -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 restore --pathspec-from-file=- --source=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 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 &&
@@ -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 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' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git restore --pathspec-from-file=- --source=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 restore --pathspec-from-file=list --source=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 restore --pathspec-from-file=list --pathspec-file-nul --source=HEAD^1
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
diff --git a/t/t3704-add-pathspec-file.sh b/t/t3704-add-pathspec-file.sh
index 61b6e51009..9009f8a9ac 100755
--- a/t/t3704-add-pathspec-file.sh
+++ b/t/t3704-add-pathspec-file.sh
@@ -23,7 +23,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 add --pathspec-from-file=- &&
@@ -34,19 +34,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 add --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- A 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 add --pathspec-from-file=- --pathspec-file-nul &&
@@ -58,67 +46,6 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t\n" | git add --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t" | git add --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git add --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- git add --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- A 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 add --pathspec-from-file=list --pathspec-file-nul
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
diff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh
index b0e84cdb42..5b845f4f7c 100755
--- a/t/t7107-reset-pathspec-file.sh
+++ b/t/t7107-reset-pathspec-file.sh
@@ -25,7 +25,7 @@ verify_expect () {
test_cmp expect actual
}
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
restore_checkpoint &&
git rm fileA.t &&
@@ -37,20 +37,7 @@ test_expect_success '--pathspec-from-file from stdin' '
verify_expect
'
-test_expect_success '--pathspec-from-file from file' '
- restore_checkpoint &&
-
- git rm fileA.t &&
- echo fileA.t >list &&
- git reset --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'NUL delimiters' '
+test_expect_success '--pathspec-file-nul' '
restore_checkpoint &&
git rm fileA.t fileB.t &&
@@ -63,77 +50,21 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- git rm fileA.t fileB.t &&
- printf "fileA.t\nfileB.t\n" | git reset --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- D fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- git rm fileA.t fileB.t &&
- printf "fileA.t\nfileB.t" | git reset --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- D fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
+test_expect_success 'only touches what was listed' '
restore_checkpoint &&
- git rm fileA.t fileB.t &&
- printf "fileA.t\r\nfileB.t\r\n" | git reset --pathspec-from-file=- &&
+ git rm fileA.t fileB.t fileC.t fileD.t &&
+ printf "fileB.t\nfileC.t\n" | git reset --pathspec-from-file=- &&
cat >expect <<-\EOF &&
- D fileA.t
+ D fileA.t
D fileB.t
+ D fileC.t
+ D fileD.t
EOF
verify_expect
'
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- git rm fileA.t &&
- git reset --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- 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 &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- EOF
- test_must_fail verify_expect
-'
-
test_expect_success '--pathspec-from-file is not compatible with --soft or --hard' '
restore_checkpoint &&
diff --git a/t/t7526-commit-pathspec-file.sh b/t/t7526-commit-pathspec-file.sh
index 4a7c11368d..8d6c652690 100755
--- a/t/t7526-commit-pathspec-file.sh
+++ b/t/t7526-commit-pathspec-file.sh
@@ -26,7 +26,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 commit --pathspec-from-file=- -m "Commit" &&
@@ -37,19 +37,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 commit --pathspec-from-file=list -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A 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 commit --pathspec-from-file=- --pathspec-file-nul -m "Commit" &&
@@ -61,67 +49,6 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t\n" | git commit --pathspec-from-file=- -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t" | git commit --pathspec-from-file=- -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git commit --pathspec-from-file=- -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- git commit --pathspec-from-file=list -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- EOF
- verify_expect expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- test_must_fail git commit --pathspec-from-file=list --pathspec-file-nul -m "Commit"
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 2/3] t: directly test parse_pathspec_file()
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-31 10:15 UTC (permalink / raw)
To: git
Cc: Jonathan Nieder, Eric Sunshine, Alexandr Miloslavskiy,
Junio C Hamano, Alexandr Miloslavskiy
In-Reply-To: <pull.503.v4.git.1577787313.gitgitgadget@gmail.com>
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
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()`.
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
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
diff --git a/Makefile b/Makefile
index 09f98b777c..0061f96e8a 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/t/helper/test-parse-pathspec-file.c b/t/helper/test-parse-pathspec-file.c
new file mode 100644
index 0000000000..02f4ccfd2a
--- /dev/null
+++ b/t/helper/test-parse-pathspec-file.c
@@ -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;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index f20989d449..c9a232d238 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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 },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8ed2af71d1..c8549fd87f 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -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);
diff --git a/t/t0067-parse_pathspec_file.sh b/t/t0067-parse_pathspec_file.sh
new file mode 100755
index 0000000000..7bab49f361
--- /dev/null
+++ b/t/t0067-parse_pathspec_file.sh
@@ -0,0 +1,108 @@
+#!/bin/sh
+
+test_description='Test parse_pathspec_file()'
+
+. ./test-lib.sh
+
+test_expect_success 'one item from stdin' '
+ 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' '
+ 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' '
+ 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' '
+ 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' '
+ 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' '
+ 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
+
+ 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
+
+ cat >list <<-\EOF &&
+ "file\101.t"
+ EOF
+
+ test-tool parse-pathspec-file --pathspec-from-file=list --pathspec-file-nul >actual &&
+
+ test_cmp expect actual
+'
+
+test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 1/3] t: fix quotes tests for --pathspec-from-file
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-31 10:15 UTC (permalink / raw)
To: git
Cc: Jonathan Nieder, Eric Sunshine, Alexandr Miloslavskiy,
Junio C Hamano, Alexandr Miloslavskiy
In-Reply-To: <pull.503.v4.git.1577787313.gitgitgadget@gmail.com>
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
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.
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
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(-)
diff --git a/t/t2026-checkout-pathspec-file.sh b/t/t2026-checkout-pathspec-file.sh
index f62fd27440..adad71f631 100755
--- a/t/t2026-checkout-pathspec-file.sh
+++ b/t/t2026-checkout-pathspec-file.sh
@@ -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
'
diff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh
index db58e83735..b407f6b779 100755
--- a/t/t2072-restore-pathspec-file.sh
+++ b/t/t2072-restore-pathspec-file.sh
@@ -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
'
diff --git a/t/t3704-add-pathspec-file.sh b/t/t3704-add-pathspec-file.sh
index 3cfdb669b7..61b6e51009 100755
--- a/t/t3704-add-pathspec-file.sh
+++ b/t/t3704-add-pathspec-file.sh
@@ -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
'
diff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh
index 6b1a731fff..b0e84cdb42 100755
--- a/t/t7107-reset-pathspec-file.sh
+++ b/t/t7107-reset-pathspec-file.sh
@@ -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 &&
diff --git a/t/t7526-commit-pathspec-file.sh b/t/t7526-commit-pathspec-file.sh
index 4b58901ed6..4a7c11368d 100755
--- a/t/t7526-commit-pathspec-file.sh
+++ b/t/t7526-commit-pathspec-file.sh
@@ -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
^ permalink raw reply related
* [PATCH v4 0/3] t: rework tests for --pathspec-from-file
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-31 10:15 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder, Eric Sunshine, Alexandr Miloslavskiy,
Junio C Hamano
In-Reply-To: <pull.503.v3.git.1577786032.gitgitgadget@gmail.com>
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/pull.490.git.1576161385.gitgitgadget@gmail.com/
[2] https://public-inbox.org/git/pull.445.v4.git.1575381738.gitgitgadget@gmail.com/
[3] https://lore.kernel.org/git/xmqqwoatcn5u.fsf@gitster-ct.c.googlers.com/
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
^ permalink raw reply
* Re: [PATCH v2 1/3] t: fix quotes tests for --pathspec-from-file
From: Alexandr Miloslavskiy @ 2019-12-31 10:01 UTC (permalink / raw)
To: Jonathan Nieder, Eric Sunshine
Cc: Alexandr Miloslavskiy via GitGitGadget, Git List, Junio C Hamano
In-Reply-To: <20191231002607.GC13606@google.com>
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!
On 30.12.2019 22:55, Eric Sunshine wrote:
> So, you want git-checkout to receive the following, quotes, backslash,
> and no newline, on its standard input?
>
> "file\101.t"
>
> If so, another way to achieve the same without taxing the brain of the
> reader or the next person who works on this code would be:
>
> tr -d "\012" | git checkout --pathspec-from-file=- HEAD^1 <<-\EOF &&
> "file\101.t"
> EOF
>
> Although it's three lines long, the body of the here-doc is the
> literal text you want sent to the Git command, so no counting
> backslashes, and no need for a lengthy in-code comment.
>
> But is the "no newline" bit indeed intentional? If not, then a simple
> echo would be even easier (though with a bit more escaping):
>
> echo "\"file\101.t\"" | git checkout --pathspec-from-file=-
HEAD^1 &&
>
On 31.12.2019 1:26, Jonathan Nieder wrote:
>> But is the "no newline" bit indeed intentional? If not, then a simple
>> echo would be even easier (though with a bit more escaping):
>>
>> echo "\"file\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
>
> For portability, that would be
>
> printf "%s\n" "\"file\101.t\"" | ...
>
> because some implementations of echo interpret escapes by default.
^ permalink raw reply
* [PATCH v3 3/3] t: drop copy&pasted tests for --pathspec-from-file
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-31 9:53 UTC (permalink / raw)
To: git; +Cc: Alexandr Miloslavskiy, Junio C Hamano, Alexandr Miloslavskiy
In-Reply-To: <pull.503.v3.git.1577786032.gitgitgadget@gmail.com>
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
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.
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
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(-)
diff --git a/t/t2026-checkout-pathspec-file.sh b/t/t2026-checkout-pathspec-file.sh
index adad71f631..559b4528d7 100755
--- a/t/t2026-checkout-pathspec-file.sh
+++ b/t/t2026-checkout-pathspec-file.sh
@@ -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 &&
diff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh
index b407f6b779..9b3125d582 100755
--- a/t/t2072-restore-pathspec-file.sh
+++ b/t/t2072-restore-pathspec-file.sh
@@ -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 restore --pathspec-from-file=- --source=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 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 &&
@@ -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 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' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git restore --pathspec-from-file=- --source=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 restore --pathspec-from-file=list --source=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 restore --pathspec-from-file=list --pathspec-file-nul --source=HEAD^1
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
diff --git a/t/t3704-add-pathspec-file.sh b/t/t3704-add-pathspec-file.sh
index 61b6e51009..9009f8a9ac 100755
--- a/t/t3704-add-pathspec-file.sh
+++ b/t/t3704-add-pathspec-file.sh
@@ -23,7 +23,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 add --pathspec-from-file=- &&
@@ -34,19 +34,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 add --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- A 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 add --pathspec-from-file=- --pathspec-file-nul &&
@@ -58,67 +46,6 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t\n" | git add --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t" | git add --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git add --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- git add --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- A 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 add --pathspec-from-file=list --pathspec-file-nul
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
diff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh
index b0e84cdb42..5b845f4f7c 100755
--- a/t/t7107-reset-pathspec-file.sh
+++ b/t/t7107-reset-pathspec-file.sh
@@ -25,7 +25,7 @@ verify_expect () {
test_cmp expect actual
}
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
restore_checkpoint &&
git rm fileA.t &&
@@ -37,20 +37,7 @@ test_expect_success '--pathspec-from-file from stdin' '
verify_expect
'
-test_expect_success '--pathspec-from-file from file' '
- restore_checkpoint &&
-
- git rm fileA.t &&
- echo fileA.t >list &&
- git reset --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'NUL delimiters' '
+test_expect_success '--pathspec-file-nul' '
restore_checkpoint &&
git rm fileA.t fileB.t &&
@@ -63,77 +50,21 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- git rm fileA.t fileB.t &&
- printf "fileA.t\nfileB.t\n" | git reset --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- D fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- git rm fileA.t fileB.t &&
- printf "fileA.t\nfileB.t" | git reset --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- D fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
+test_expect_success 'only touches what was listed' '
restore_checkpoint &&
- git rm fileA.t fileB.t &&
- printf "fileA.t\r\nfileB.t\r\n" | git reset --pathspec-from-file=- &&
+ git rm fileA.t fileB.t fileC.t fileD.t &&
+ printf "fileB.t\nfileC.t\n" | git reset --pathspec-from-file=- &&
cat >expect <<-\EOF &&
- D fileA.t
+ D fileA.t
D fileB.t
+ D fileC.t
+ D fileD.t
EOF
verify_expect
'
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- git rm fileA.t &&
- git reset --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- 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 &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- EOF
- test_must_fail verify_expect
-'
-
test_expect_success '--pathspec-from-file is not compatible with --soft or --hard' '
restore_checkpoint &&
diff --git a/t/t7526-commit-pathspec-file.sh b/t/t7526-commit-pathspec-file.sh
index 4a7c11368d..8d6c652690 100755
--- a/t/t7526-commit-pathspec-file.sh
+++ b/t/t7526-commit-pathspec-file.sh
@@ -26,7 +26,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 commit --pathspec-from-file=- -m "Commit" &&
@@ -37,19 +37,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 commit --pathspec-from-file=list -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A 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 commit --pathspec-from-file=- --pathspec-file-nul -m "Commit" &&
@@ -61,67 +49,6 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t\n" | git commit --pathspec-from-file=- -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t" | git commit --pathspec-from-file=- -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git commit --pathspec-from-file=- -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- git commit --pathspec-from-file=list -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- EOF
- verify_expect expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- cat >list <<-\EOF &&
- "file\101.t"
- EOF
-
- test_must_fail git commit --pathspec-from-file=list --pathspec-file-nul -m "Commit"
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 1/3] t: fix quotes tests for --pathspec-from-file
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-31 9:53 UTC (permalink / raw)
To: git; +Cc: Alexandr Miloslavskiy, Junio C Hamano, Alexandr Miloslavskiy
In-Reply-To: <pull.503.v3.git.1577786032.gitgitgadget@gmail.com>
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
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.
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
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(-)
diff --git a/t/t2026-checkout-pathspec-file.sh b/t/t2026-checkout-pathspec-file.sh
index f62fd27440..adad71f631 100755
--- a/t/t2026-checkout-pathspec-file.sh
+++ b/t/t2026-checkout-pathspec-file.sh
@@ -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
'
diff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh
index db58e83735..b407f6b779 100755
--- a/t/t2072-restore-pathspec-file.sh
+++ b/t/t2072-restore-pathspec-file.sh
@@ -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
'
diff --git a/t/t3704-add-pathspec-file.sh b/t/t3704-add-pathspec-file.sh
index 3cfdb669b7..61b6e51009 100755
--- a/t/t3704-add-pathspec-file.sh
+++ b/t/t3704-add-pathspec-file.sh
@@ -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
'
diff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh
index 6b1a731fff..b0e84cdb42 100755
--- a/t/t7107-reset-pathspec-file.sh
+++ b/t/t7107-reset-pathspec-file.sh
@@ -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 &&
diff --git a/t/t7526-commit-pathspec-file.sh b/t/t7526-commit-pathspec-file.sh
index 4b58901ed6..4a7c11368d 100755
--- a/t/t7526-commit-pathspec-file.sh
+++ b/t/t7526-commit-pathspec-file.sh
@@ -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
^ permalink raw reply related
* [PATCH v3 2/3] t: directly test parse_pathspec_file()
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-31 9:53 UTC (permalink / raw)
To: git; +Cc: Alexandr Miloslavskiy, Junio C Hamano, Alexandr Miloslavskiy
In-Reply-To: <pull.503.v3.git.1577786032.gitgitgadget@gmail.com>
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
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()`.
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
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
diff --git a/Makefile b/Makefile
index 09f98b777c..0061f96e8a 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/t/helper/test-parse-pathspec-file.c b/t/helper/test-parse-pathspec-file.c
new file mode 100644
index 0000000000..02f4ccfd2a
--- /dev/null
+++ b/t/helper/test-parse-pathspec-file.c
@@ -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;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index f20989d449..c9a232d238 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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 },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8ed2af71d1..c8549fd87f 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -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);
diff --git a/t/t0067-parse_pathspec_file.sh b/t/t0067-parse_pathspec_file.sh
new file mode 100755
index 0000000000..7bab49f361
--- /dev/null
+++ b/t/t0067-parse_pathspec_file.sh
@@ -0,0 +1,108 @@
+#!/bin/sh
+
+test_description='Test parse_pathspec_file()'
+
+. ./test-lib.sh
+
+test_expect_success 'one item from stdin' '
+ 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' '
+ 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' '
+ 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' '
+ 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' '
+ 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' '
+ 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
+
+ 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
+
+ cat >list <<-\EOF &&
+ "file\101.t"
+ EOF
+
+ test-tool parse-pathspec-file --pathspec-from-file=list --pathspec-file-nul >actual &&
+
+ test_cmp expect actual
+'
+
+test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 0/3] t: rework tests for --pathspec-from-file
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-31 9:53 UTC (permalink / raw)
To: git; +Cc: Alexandr Miloslavskiy, Junio C Hamano
In-Reply-To: <pull.503.v2.git.1577733329.gitgitgadget@gmail.com>
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/pull.490.git.1576161385.gitgitgadget@gmail.com/
[2] https://public-inbox.org/git/pull.445.v4.git.1575381738.gitgitgadget@gmail.com/
[3] https://lore.kernel.org/git/xmqqwoatcn5u.fsf@gitster-ct.c.googlers.com/
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 <alexandr.miloslavskiy@syntevo.com>
@@ -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
^ permalink raw reply
* Re: [PATCH] sha1-file: remove OBJECT_INFO_SKIP_CACHED
From: Jonathan Nieder @ 2019-12-31 1:03 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <20191231003903.36486-1-jonathantanmy@google.com>
Jonathan Tan wrote:
> Jonathan Nieder wrote:
>> Hm, where does this dichotomy come from? E.g. is the latter a
>> lower-level function used by the former?
>
> I don't know the reason for the dichotomy - perhaps it was an oversight.
Ah, that makes sense.
[...]
> This might be a moot point, but what do you mean by the
> "'has_object_file || find_cached_object' pattern"?
Oh! Thanks, I should have been more precise. And calling it a pattern
was probably overreaching --- I can only find one instance, in
pretend_object_file:
if (has_object_file(oid) || find_cached_object(oid))
return 0;
Since there's only one, I was on the wrong track. (Except that with
this change, that can indeed change to
if (has_object_file(oid))
return 0;
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH] sha1-file: remove OBJECT_INFO_SKIP_CACHED
From: Jonathan Tan @ 2019-12-31 0:39 UTC (permalink / raw)
To: jrnieder; +Cc: jonathantanmy, git
In-Reply-To: <20191230220155.GF57251@google.com>
> Ooh, I think there's something subtle hiding in this paragraph.
>
> When I first read it, I thought you meant that the repositories are
> not self-contained --- that they contain references to the empty tree
> but do not fulfill "want"s for them. I don't believe that's what you
> mean, though.
>
> My second reading is the repository genuinely doesn't contain the
> empty tree but different Git server implementations handle that
> differently. I tried to reproduce this with
>
> empty_tree=$(git mktree </dev/null)
> git init --bare x
> git clone --filter=blob:none file://$(pwd)/x y
> cd y
> echo hi >README
> git add README
> git commit -m 'nonempty tree'
> GIT_TRACE=1 git diff-tree "$empty_tree" HEAD
>
> and indeed, it looks like Git serves the empty tree even from
> repositories that don't contain it. By comparison, JGit does not do
> the same trick. So we don't need to refer to a specific repository in
> the wild to reproduce this.
>
> All that said, having to fetch this object in the first place is
> unexpected. The question of the promisor remote serving it is only
> relevant from the point of view of "why didn't we discover this
> sooner?"
Yes, that's true. I'll reword it to emphasize the spurious fetching (and
mention some implementations like JGit not serving those, which
exacerbates the problem).
I think we didn't discover this sooner because not many people directly
enter the empty tree on the command line :-) (but this is a problem that
we should solve, most certainly).
> > There are 2 functions: repo_has_object_file() which does not consult
> > find_cached_object() (which, among other things, knows about the empty
> > tree); and repo_read_object_file() which does.
>
> Hm, where does this dichotomy come from? E.g. is the latter a
> lower-level function used by the former?
I don't know the reason for the dichotomy - perhaps it was an oversight.
The relevant commits are:
d66b37bb19 ("Add pretend_sha1_file() interface.", 2007-02-05)
Adds a way to pretend that some objects are present by including them in
a cache - empty-tree-pervasiveness is built on top of this later. Only
read_sha1_file() was updated to make use of this; has_sha1_file() and
sha1_object_info() were already present at this time, but were not
updated. (Maybe the latter 2 had no need for pretending?)
346245a1bb ("hard-code the empty tree object", 2008-02-13)
Empty tree pervasiveness built on top of the cache.
c4d9986f5f ("sha1_object_info: examine cached_object store too",
2011-02-07)
sha1_object_info() was updated to use the cache. has_sha1_file() is
never mentioned.
> > as an optimization to avoid reading blobs into memory,
> > parse_object() calls repo_has_object_file() before
> > repo_read_object_file(). In the case of a regular repository (that is,
> > not a partial clone), repo_has_object_file() will return false for the
> > empty tree (thus bypassing the optimization) and repo_read_object_file()
> > will nevertheless succeed, thus things coincidentally work.
>
> This might be easier to understand if phrased in terms of the
> intention behind the code instead of the specific call stacks used.
> See f06ab027 for an example of this kind of thing. For example:
>
> Applications within and outside of Git benefit from being able to
> assume that every repository contains the empty tree as an object
> (see, for example, commit 9abd46a347 "Make sure the empty tree
> exists when needed in merge-recursive", 2006-12-07). To this end,
> since 346245a1bb (hard-code the empty tree object, 2008-02-13), Git
> has made the empty tree available in all repositories via
> find_cached_object, which all object access paths can use.
>
> Object existence checks (has_object_file), however, do not use
> find_cached_object. <describe reason here>
>
> When I state it this way, it's not obvious why this particular caller
> of has_object_file is more relevant than others. Did I miss some
> subtlety?
This particular caller is what caused us to notice this problem.
> Indeed. Can we justify the change even more simply in those terms?
>
> Object existence checks (has_object_file), however, do not use
> find_cached_object. <describe reason here>
>
> This makes the API unnecessarily difficult to reason about.
> Instead, let's consistently view the empty tree as a genuine part of
> the repository, even in has_object_file. As a side effect, this
> allows us to simplify the common 'has_object_file ||
> find_cached_object' pattern to a more simple existence check.
OK, let me see if I can rewrite it to emphasize the reasoning-about-API
part. I still want to include the user-facing part that caused us to
observe this problem, but I can deemphasize it.
This might be a moot point, but what do you mean by the
"'has_object_file || find_cached_object' pattern"?
^ permalink raw reply
* Re: [PATCH v2 1/3] t: fix quotes tests for --pathspec-from-file
From: Jonathan Nieder @ 2019-12-31 0:26 UTC (permalink / raw)
To: Eric Sunshine
Cc: Alexandr Miloslavskiy via GitGitGadget, Git List,
Alexandr Miloslavskiy, Junio C Hamano
In-Reply-To: <CAPig+cSSqAxuHYg9DxuJzC7m2HAt8F2YPNxT0x5+SksCGic4MA@mail.gmail.com>
Eric Sunshine wrote:
> But is the "no newline" bit indeed intentional? If not, then a simple
> echo would be even easier (though with a bit more escaping):
>
> echo "\"file\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
For portability, that would be
printf "%s\n" "\"file\101.t\"" | ...
because some implementations of echo interpret escapes by default.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH v2] revision: allow missing promisor objects on CLI
From: Jonathan Nieder @ 2019-12-31 0:09 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20191230234453.255082-1-jonathantanmy@google.com>
Jonathan Tan wrote:
> Commit 4cf67869b2 ("list-objects.c: don't segfault for missing cmdline
> objects", 2018-12-06) prevented some segmentation faults from occurring
> by tightening handling of missing objects provided through the CLI: if
> --ignore-missing is set, then it is OK (and the missing object ignored,
> just like one would if encountered in traversal).
>
> However, in the case that --ignore-missing is not set but
> --exclude-promisor-objects is set, there is still no distinction between
> the case wherein the missing object is a promisor object and the case
> wherein it is not. This is unnecessarily restrictive, since if a missing
> promisor object is encountered in traversal, it is ignored; likewise it
> should be ignored if provided through the CLI. Therefore, distinguish
> between these 2 cases. (As a bonus, the code is now simpler.)
nit about tenses, not worth a reroll on its own: "As a bonus, this
makes the code simpler" (since commit messages describe the status quo
before the patch in the present tense).
[...]
> --- a/revision.c
> +++ b/revision.c
> @@ -370,8 +370,18 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
> if (!repo_parse_commit(revs->repo, c))
> object = (struct object *) c;
> else
> + /*
> + * There is something wrong with the commit.
> + * repo_parse_commit() will have already printed an
> + * error message. For our purposes, treat as missing.
> + */
> object = NULL;
> } else {
> + /*
> + * There is something wrong with the object. parse_object()
If we got here, we are in the !commit case, which is not inherently wrong,
right? Is the intent something like the following?
if (oid_object_info(...) == OBJ_COMMIT && !repo_parse_commit(...)) {
... good ...
} else if (parse_object(...)) {
... good ...
} else {
/*
* An error occured while parsing the object.
* parse_commit or parse_object has already printed an
* error message. For our purposes, it's safe to
* assume the object as missing.
*/
object = NULL;
}
This might be easiest to understand as a separate mini-function.
Something like
/*
* Like parse_object, but optimized by reading commits from the
* commit graph.
*
* If the repository has commit graphs, repo_parse_commit() avoids
* reading the object buffer, so use it whenever possible.
*/
static struct object *parse_object_probably_commit(
struct repository *r, const struct object_id *oid)
{
struct commit *c;
if (oid_object_info(r, oid, NULL) != OBJ_COMMIT)
return parse_object(r, oid);
c = lookup_commit(r, oid);
if (repo_parse_commit(r, c))
return NULL;
return (struct object *) c;
}
static struct object *get_reference(struct rev_info *revs, ...)
{
struct object *object = parse_object_probably_commit(revs->repo, oid);
if (!object)
/*
* An error occured while parsing the object.
* parse_object_probably_commit has already printed an
* error message. For our purposes, it's safe to
* assume the object as missing.
*/
;
By the way, why doesn't parse_object itself check the commit graph for
commit objects?
[...]
> @@ -1907,7 +1917,18 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
> verify_non_filename(revs->prefix, arg);
> object = get_reference(revs, arg, &oid, flags ^ local_flags);
> if (!object)
> - return revs->ignore_missing ? 0 : -1;
> + /*
> + * If this object is corrupt, get_reference() prints an error
> + * message and treats it as missing.
By "and treats it as missing" does this mean "and we should treat it
as missing"? (Forgive my ignorance.)
Why do we treat corrupt objects as missing? Is this for graceful
degredation when trying to recover data from a corrupt repository?
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH v2 1/1] commit: display advice hints when commit fails
From: Jonathan Tan @ 2019-12-31 0:04 UTC (permalink / raw)
To: gitster; +Cc: gitgitgadget, git, heba.waly, Jonathan Tan
In-Reply-To: <xmqqbls4aznl.fsf@gitster-ct.c.googlers.com>
> > This fix was about "we do not want to unconditionally drop the
> > advice messages when we reject the attempt to commit and show the
> > output like 'git status'", wasn't it? The earlier single-liner fix
> > in v1 that flips s->hints just before calling run_status() before
> > rejecting the attempt to commit was a lot easier to reason about, as
> > the fix was very focused and to the point. Why are we seeing this
> > many (seemingly unrelated) changes?
>
> In any case, here is what I tentatively have in my tree (with heavy
> rewrite to the proposed log message).
Junio, what are your plans over what you have in your tree? If you'd
like to hear Heba's opinion on it, then she can chime in; if you'd like
a review, then I think it's good to go in.
I think the main area of discussion is whether we should go with Heba's
attempt to address Emily's comment [1]:
> I think the intent of that commit was to not put hints into the editor,
> so does it make sense to instead wrap this guy:
>
> /*
> * Most hints are counter-productive when the commit has
> * already started.
> */
> s->hints = 0;
>
> in "if (use_editor)"?
>
> I didn't try it on my end. Maybe it won't help much, because we think
> we're going to use the editor right up until we realize it's not
> committable?
And I think the answer to that is "s" is used throughout the function in
various ways (in particular, used to print statuses both to stdout and
to the message template) so any wrapping or corralling of scope would
just make things more complicated. In particular, the way Heba did it in
v2 is more unclear - at the time of setting s->hints = 0, it's done
within a "if (use_editor && include_status)" block, but (as far as I can
tell) the commit message template might also be used when there is no
editor - for example, as input to a hook. And more importantly, when
s->hints is reset to the config, we don't know at that point that the
next status is going to stdout. So I think it's better just to use the
v1 way.
The second area of discussion I see is in the commit message. Commit
messages have to balance brevity and comprehensiveness, and this can be
a subjective matter, but I think Junio's strikes a good balance.
[1] https://lore.kernel.org/git/20191217224541.GA230678@google.com/
^ permalink raw reply
* [PATCH v2] revision: allow missing promisor objects on CLI
From: Jonathan Tan @ 2019-12-30 23:44 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster
In-Reply-To: <20191228003430.241283-1-jonathantanmy@google.com>
Commit 4cf67869b2 ("list-objects.c: don't segfault for missing cmdline
objects", 2018-12-06) prevented some segmentation faults from occurring
by tightening handling of missing objects provided through the CLI: if
--ignore-missing is set, then it is OK (and the missing object ignored,
just like one would if encountered in traversal).
However, in the case that --ignore-missing is not set but
--exclude-promisor-objects is set, there is still no distinction between
the case wherein the missing object is a promisor object and the case
wherein it is not. This is unnecessarily restrictive, since if a missing
promisor object is encountered in traversal, it is ignored; likewise it
should be ignored if provided through the CLI. Therefore, distinguish
between these 2 cases. (As a bonus, the code is now simpler.)
(Note that this only affects handling of missing promisor objects.
Handling of non-missing promisor objects is already done by setting all
of them to UNINTERESTING in prepare_revision_walk().)
Additionally, clarify in get_reference() that error messages are already
being printed by the functions called (parse_object(),
repo_parse_commit(), and parse_commit_buffer() - invoked by the latter).
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Changes from v1: Improved code comments and commit message
> This is the case where oid must be COMMIT from oid_object_info()'s
> point of view, but repo_parse_commit() finds it as a non-commit, and
> object becomes NULL. This is quite different from the normal lazy
> clone case where exclude-promisor-objects etc. wants to cover, that
> the object whose name is oid is truly missing because it can be
> fetched later from elsewhere. Instead, we have found that there is
> an inconsistency in the data we have about the object, iow, a
> possible corruption.
Thanks! I should have looked at the first half of get_reference() more
carefully.
If there is corruption in the form of hash mismatch, parse_object() will
print a message and then return NULL, leaving get_reference() to handle
it - and treat it as missing in this case. It seems reasonable to me to
handle the repo_parse_commit() failure in a similar way. I've added
comments to clarify that error messages are being printed.
---
revision.c | 23 ++++++++++++++++++++++-
t/t0410-partial-clone.sh | 10 ++--------
2 files changed, 24 insertions(+), 9 deletions(-)
diff --git a/revision.c b/revision.c
index 8136929e23..af1e31b4fc 100644
--- a/revision.c
+++ b/revision.c
@@ -370,8 +370,18 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
if (!repo_parse_commit(revs->repo, c))
object = (struct object *) c;
else
+ /*
+ * There is something wrong with the commit.
+ * repo_parse_commit() will have already printed an
+ * error message. For our purposes, treat as missing.
+ */
object = NULL;
} else {
+ /*
+ * There is something wrong with the object. parse_object()
+ * will have already printed an error message. For our
+ * purposes, treat as missing.
+ */
object = parse_object(revs->repo, oid);
}
@@ -1907,7 +1917,18 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
verify_non_filename(revs->prefix, arg);
object = get_reference(revs, arg, &oid, flags ^ local_flags);
if (!object)
- return revs->ignore_missing ? 0 : -1;
+ /*
+ * If this object is corrupt, get_reference() prints an error
+ * message and treats it as missing.
+ *
+ * get_reference() returns NULL only if this object is missing
+ * and ignore_missing is true, or this object is a (missing)
+ * promisor object and exclude_promisor_objects is true. In
+ * both these cases, we can safely ignore this object because
+ * this object will not appear in output and cannot be used as
+ * a source of UNINTERESTING ancestors (since it is missing).
+ */
+ return 0;
add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);
add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);
free(oc.path);
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index a3988bd4b8..fd28f5402a 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -416,15 +416,9 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '
git -C repo config extensions.partialclone "arbitrary string" &&
for OBJ in "$COMMIT" "$TREE" "$BLOB"; do
- test_must_fail git -C repo rev-list --objects \
+ git -C repo rev-list --objects \
--exclude-promisor-objects "$OBJ" &&
- test_must_fail git -C repo rev-list --objects-edge-aggressive \
- --exclude-promisor-objects "$OBJ" &&
-
- # Do not die or crash when --ignore-missing is passed.
- git -C repo rev-list --ignore-missing --objects \
- --exclude-promisor-objects "$OBJ" &&
- git -C repo rev-list --ignore-missing --objects-edge-aggressive \
+ git -C repo rev-list --objects-edge-aggressive \
--exclude-promisor-objects "$OBJ"
done
'
--
2.24.1.735.g03f4e72817-goog
^ permalink raw reply related
* Re: "gmane:" search undocumented on lore.kernel.org/git
From: Junio C Hamano @ 2019-12-30 23:33 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: Eric Wong, Philippe Blain, git
In-Reply-To: <20191230231620.lcydd5egk4ma2rph@chatter.i7.local>
Konstantin Ryabitsev <konstantin@linuxfoundation.org> writes:
>> gmane: doesn't seem configured on lore at all. Compare:
>>
>> https://lore.kernel.org/git/?q=gmane:1
>> https://public-inbox.org/git/?q=gmane:1
>
> I haven't configured them because I don't understand what benefit they
> serve. If someone can explain their benefit, I can consider adding this
> to the roadmap.
Just that many old messages you find in the lore and public-inbox
archive (and also a handful of commit log messages) use the syntax
like $gmane/290280 to refer to messages on the list with gmane plus
the article numbers.
^ permalink raw reply
* Re: [PATCH 16/16] t4124: let sed open its own files
From: Junio C Hamano @ 2019-12-30 23:27 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Denton Liu, Git Mailing List
In-Reply-To: <868smt2zqh.fsf@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> Denton Liu <liu.denton@gmail.com> writes:
>
>> In one case, we were using a redirection operator to feed input into
>> sed. However, since sed is capable of opening its own files, make sed
>> open its own files instead of redirecting input into it.
>
> Could you please write in the commit message what advantages does this
> change bring?
A fair question.
My version of short answer is "nothing---it is not wrong to write it
either way, and it is not worth the patch churn to rewrite it from
one form to the other, once the script is written".
If we were to extend these tests in such a way that the command
needs to read from more than one input file, though, dropping the
redirection like the patch does is a good first step toward that,
i.e. extending
sed -e "expression" patch
to
sed -e "expression" patch-1 patch-2 ...
would look more natural than starting from
sed -e "expression" <patch
to end at the same place, so as a preliminary clean-up, a change
like this one may have value, but because there is no such further
updates planned, so...
^ permalink raw reply
* Re: "gmane:" search undocumented on lore.kernel.org/git
From: Jonathan Nieder @ 2019-12-30 23:25 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: Eric Wong, Philippe Blain, git
In-Reply-To: <20191230231620.lcydd5egk4ma2rph@chatter.i7.local>
Konstantin Ryabitsev wrote:
> On Mon, Dec 30, 2019 at 11:13:50PM +0000, Eric Wong wrote:
>> gmane: doesn't seem configured on lore at all. Compare:
>>
>> https://lore.kernel.org/git/?q=gmane:1
>> https://public-inbox.org/git/?q=gmane:1
>
> I haven't configured them because I don't understand what benefit they
> serve. If someone can explain their benefit, I can consider adding this
> to the roadmap.
They allow finding the messages referred to by historic gmane.org
links. For example,
http://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435
refers to https://public-inbox.org/git/?q=gmane:151435, and
$gmane/127980
refers to https://public-inbox.org/git/?q=gmane:127980. There are
numerous examples of both kinds of references in the mailing list
archive.
Thanks,
Jonathan
^ permalink raw reply
* Re: "gmane:" search undocumented on lore.kernel.org/git
From: Konstantin Ryabitsev @ 2019-12-30 23:16 UTC (permalink / raw)
To: Eric Wong; +Cc: Philippe Blain, git
In-Reply-To: <20191230231350.GA16499@dcvr>
On Mon, Dec 30, 2019 at 11:13:50PM +0000, Eric Wong wrote:
> Philippe Blain <levraiphilippeblain@gmail.com> wrote:
> > Hello all,
> >
> > I just noticed that the help page on lore.kernel.org/git does not mention the possibility of searching for messages using Gmane id’s, as the public-inbox.org/git help page does.
> >
> > The search works though, it’s just undocumented, so I thought I’d share; I don’t know how much control we have on the lore.kernel.org public-inbox installation.
>
> +Cc Konstantin who runs lore.
>
> Unless you're getting load-balanced to differently configured
> instances, I think it's luck that the "gmane:" queries you tried
> work.
>
> gmane: doesn't seem configured on lore at all. Compare:
>
> https://lore.kernel.org/git/?q=gmane:1
> https://public-inbox.org/git/?q=gmane:1
I haven't configured them because I don't understand what benefit they
serve. If someone can explain their benefit, I can consider adding this
to the roadmap.
-K
^ permalink raw reply
* Re: "gmane:" search undocumented on lore.kernel.org/git
From: Eric Wong @ 2019-12-30 23:13 UTC (permalink / raw)
To: Philippe Blain; +Cc: git, Konstantin Ryabitsev
In-Reply-To: <03879A6A-2E8B-45D9-A06E-51926AC949F9@gmail.com>
Philippe Blain <levraiphilippeblain@gmail.com> wrote:
> Hello all,
>
> I just noticed that the help page on lore.kernel.org/git does not mention the possibility of searching for messages using Gmane id’s, as the public-inbox.org/git help page does.
>
> The search works though, it’s just undocumented, so I thought I’d share; I don’t know how much control we have on the lore.kernel.org public-inbox installation.
+Cc Konstantin who runs lore.
Unless you're getting load-balanced to differently configured
instances, I think it's luck that the "gmane:" queries you tried
work.
gmane: doesn't seem configured on lore at all. Compare:
https://lore.kernel.org/git/?q=gmane:1
https://public-inbox.org/git/?q=gmane:1
^ permalink raw reply
* "gmane:" search undocumented on lore.kernel.org/git
From: Philippe Blain @ 2019-12-30 23:06 UTC (permalink / raw)
To: git
Hello all,
I just noticed that the help page on lore.kernel.org/git does not mention the possibility of searching for messages using Gmane id’s, as the public-inbox.org/git help page does.
The search works though, it’s just undocumented, so I thought I’d share; I don’t know how much control we have on the lore.kernel.org public-inbox installation.
Cheers,
Philippe.
^ permalink raw reply
* Re: [PATCH 16/16] t4124: let sed open its own files
From: Jakub Narebski @ 2019-12-30 22:52 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
In-Reply-To: <54315fecfe373d8020f2172b9b43e02c0dae137d.1577454401.git.liu.denton@gmail.com>
Denton Liu <liu.denton@gmail.com> writes:
> In one case, we were using a redirection operator to feed input into
> sed. However, since sed is capable of opening its own files, make sed
> open its own files instead of redirecting input into it.
Could you please write in the commit message what advantages does this
change bring?
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> t/t4124-apply-ws-rule.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
> index 21a4adc73a..2b19ef9811 100755
> --- a/t/t4124-apply-ws-rule.sh
> +++ b/t/t4124-apply-ws-rule.sh
> @@ -42,7 +42,7 @@ apply_patch () {
> shift
> fi &&
> >target &&
> - sed -e "s|\([ab]\)/file|\1/target|" <patch |
> + sed -e "s|\([ab]\)/file|\1/target|" patch |
> $should_fail git apply "$@"
> }
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH] sha1-file: remove OBJECT_INFO_SKIP_CACHED
From: Jonathan Nieder @ 2019-12-30 22:01 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <20191230211027.37002-1-jonathantanmy@google.com>
Hi,
Jonathan Tan wrote:
> In a partial clone, if a user provides the hash of the empty tree ("git
> mktree </dev/null" - for SHA-1, this is 4b825d...) to a command which
> requires that that object be parsed, for example:
>
> git diff-tree 4b825d <a non-empty tree>
>
> then Git will lazily fetch the empty tree. This fetch would merely be
> inconvenient if the promisor remote could supply that tree, but at
> $DAYJOB we discovered that some repositories do not (e.g. [1]).
Ooh, I think there's something subtle hiding in this paragraph.
When I first read it, I thought you meant that the repositories are
not self-contained --- that they contain references to the empty tree
but do not fulfill "want"s for them. I don't believe that's what you
mean, though.
My second reading is the repository genuinely doesn't contain the
empty tree but different Git server implementations handle that
differently. I tried to reproduce this with
empty_tree=$(git mktree </dev/null)
git init --bare x
git clone --filter=blob:none file://$(pwd)/x y
cd y
echo hi >README
git add README
git commit -m 'nonempty tree'
GIT_TRACE=1 git diff-tree "$empty_tree" HEAD
and indeed, it looks like Git serves the empty tree even from
repositories that don't contain it. By comparison, JGit does not do
the same trick. So we don't need to refer to a specific repository in
the wild to reproduce this.
All that said, having to fetch this object in the first place is
unexpected. The question of the promisor remote serving it is only
relevant from the point of view of "why didn't we discover this
sooner?"
> There are 2 functions: repo_has_object_file() which does not consult
> find_cached_object() (which, among other things, knows about the empty
> tree); and repo_read_object_file() which does.
Hm, where does this dichotomy come from? E.g. is the latter a
lower-level function used by the former?
> This issue occurs
> because,
nit: on first reading I had trouble figuring out what "this issue"
refers to here.
[...]
> as an optimization to avoid reading blobs into memory,
> parse_object() calls repo_has_object_file() before
> repo_read_object_file(). In the case of a regular repository (that is,
> not a partial clone), repo_has_object_file() will return false for the
> empty tree (thus bypassing the optimization) and repo_read_object_file()
> will nevertheless succeed, thus things coincidentally work.
This might be easier to understand if phrased in terms of the
intention behind the code instead of the specific call stacks used.
See f06ab027 for an example of this kind of thing. For example:
Applications within and outside of Git benefit from being able to
assume that every repository contains the empty tree as an object
(see, for example, commit 9abd46a347 "Make sure the empty tree
exists when needed in merge-recursive", 2006-12-07). To this end,
since 346245a1bb (hard-code the empty tree object, 2008-02-13), Git
has made the empty tree available in all repositories via
find_cached_object, which all object access paths can use.
Object existence checks (has_object_file), however, do not use
find_cached_object. <describe reason here>
> But in a
> partial clone, repo_has_object_file() triggers a lazy fetch of the
> missing empty tree.
This particularly affects partial clones: has_object_file does not
only report false in this case but contacts the promisor remote in
order to obtain that answer. The cost of these repeated negative
lookups can add up.
For example, in an optimization introduced in 090ea12671
("parse_object: avoid putting whole blob in core", 2012-03-07),
object parsing uses has_object_file before read_object_file to check
for a fast-path, so this negative lookup is triggered whenever we
try to parse the absent empty tree.
When I state it this way, it's not obvious why this particular caller
of has_object_file is more relevant than others. Did I miss some
subtlety?
[...]
> This fetch would merely be
> inconvenient if the promisor remote could supply that tree, but at
> $DAYJOB we discovered that some repositories do not (e.g. [1]).
If the promisor remote is running standard Git then it *does* have a
copy of the empty tree, via the cached object itself. This
guarantee is not a documented part of the protocol, however, and
other Git servers do not implement it.
> The best solution to the problem introduced at the start of this commit
> message seems to be to eliminate this dichotomy.
Indeed. Can we justify the change even more simply in those terms?
Object existence checks (has_object_file), however, do not use
find_cached_object. <describe reason here>
This makes the API unnecessarily difficult to reason about.
Instead, let's consistently view the empty tree as a genuine part of
the repository, even in has_object_file. As a side effect, this
allows us to simplify the common 'has_object_file ||
find_cached_object' pattern to a more simple existence check.
[...]
> A cost is that repo_has_object_file() will
> now need to oideq upon each invocation, but that is trivial compared to
> the filesystem lookup or the pack index search required anyway. (And if
> find_cached_object() needs to do more because of previous invocations to
> pretend_object_file(), all the more reason to be consistent in whether
> we present cached objects.) Therefore, remove OBJECT_INFO_SKIP_CACHED.
Thanks for discussing the possible costs, and I agree that they're
trivial relative to the I/O that these functions already incur.
[...]
> object-store.h | 2 --
> sha1-file.c | 38 ++++++++++++++++++--------------------
> 2 files changed, 18 insertions(+), 22 deletions(-)
As hinted above, we should be able to simplify away has_sha1_file ||
find_cached_object checks in this change.
Thanks and hpoe that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries
From: Junio C Hamano @ 2019-12-30 21:57 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <nycvar.QRO.7.76.6.1912262209190.46@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Turns out that this inconsistency is only in Git for Windows v2.24.1(2)
> but not in current `master` of Git, so I simply struck that part from the
> commit message.
> ...
> I rephrased it to:
>
> So let's loosen the requirements: we now leave tree entries with
> backslashes in their file names alone, but we do require any entries
> that are added to the Git index to contain no backslashes on Windows.
> ...
We are in -rc so there is no real rush, but I take these to mean
that I should just leave this loose end hanging untied, and wait
for an updated version to replace it sometime early next year.
Thanks and happy new year.
^ permalink raw reply
* Re: [PATCH v2 1/3] t: fix quotes tests for --pathspec-from-file
From: Eric Sunshine @ 2019-12-30 21:55 UTC (permalink / raw)
To: Alexandr Miloslavskiy via GitGitGadget
Cc: Git List, Alexandr Miloslavskiy, Junio C Hamano
In-Reply-To: <6193dc7396b9cc6cb78f382c1b1679d6bb455fe4.1577733329.git.gitgitgadget@gmail.com>
On Mon, Dec 30, 2019 at 2:15 PM Alexandr Miloslavskiy via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.
>
> Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> ---
> diff --git a/t/t2026-checkout-pathspec-file.sh b/t/t2026-checkout-pathspec-file.sh
> @@ -109,7 +109,10 @@ test_expect_success 'CRLF delimiters' '
> - 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 &&
So, you want git-checkout to receive the following, quotes, backslash,
and no newline, on its standard input?
"file\101.t"
If so, another way to achieve the same without taxing the brain of the
reader or the next person who works on this code would be:
tr -d "\012" | git checkout --pathspec-from-file=- HEAD^1 <<-\EOF &&
"file\101.t"
EOF
Although it's three lines long, the body of the here-doc is the
literal text you want sent to the Git command, so no counting
backslashes, and no need for a lengthy in-code comment.
But is the "no newline" bit indeed intentional? If not, then a simple
echo would be even easier (though with a bit more escaping):
echo "\"file\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox