From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Date: Fri, 13 Dec 2019 13:04:22 GMT
Subject: Re: [PATCH v3] grep: support the --pathspec-from-file option
Message-ID: <e4e81dba-d861-0e7c-c06c-4041e903f8ea@syntevo.com>
In-Reply-To: <20191213041254.13202-1-emilyshaffer@google.com>

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`.