Thanks, the patch looks solid to me, just a few code style rants :)
On 13.12.2019 5:12, Emily Shaffer wrote:
+ if (patterns_from_file && pathspec_from_file &&
+ !strcmp(pathspec_from_file, "-") &&
+ !strcmp(patterns_from_file, "-"))
+ die(_("cannot specify both patterns and pathspec via stdin"));
+
+ if (patterns_from_file)
+ read_pattern_file(&opt, patterns_from_file);
+
+
That looks a lot more solid now, thanks!
@@ -549,6 +549,10 @@ test_expect_success 'grep -f, non-existent file' '
test_must_fail git grep -f patterns
'
Could also benefit from testing for specific error here.
+test_expect_success 'setup pathspecs-file tests' '
+cat >excluded-file <<EOF &&
+bar
+EOF
+cat >pathspec-file <<EOF &&
+foo
+bar
+baz
+EOF
+cat >unrelated-file <<EOF &&
+xyz
+EOF
+git add excluded-file pathspec-file unrelated-file
+'
Please use <<-EOF and tabulate the code properly. I suggest that you
have a look at `t/t7107-reset-pathspec-file.sh`, last test.
+
+cat >pathspecs <<EOF
+pathspec-file
+unrelated-file
+EOF
+
+cat >expected <<EOF
+pathspec-file:bar
+EOF
The general approach nowadays is to write `expect` file in every test.
Note that the standard name is `expect`, so please remove `ed` from your
name. I think that this is also reasonable for `pathspecs` file, it's
only used in two places. Again, please refer to
`t/t7107-reset-pathspec-file.sh`.
