Re: [PATCH v2] grep: support the --pathspec-from-file option

From: Denton Liu <liu.denton@gmail.com>
Date: Dec 4, 2019, 21:05 UTC
Message-ID: <20191204210514.GA89300@generichostname>
In-Reply-To: <20191204203911.237056-1-emilyshaffer@google.com>

Hi Emily,

On Wed, Dec 04, 2019 at 12:39:11PM -0800, Emily Shaffer wrote:

Show 17 quoted lines

> @@ -289,6 +293,20 @@ In future versions we may learn to support patterns containing \0 for
>  more search backends, until then we'll die when the pattern type in
>  question doesn't support them.
>  
> +--pathspec-from-file <file>::
> +	Read pathspec from <file> instead of the command line. If `<file>` is
> +	exactly `-` then standard input is used; standard input cannot be used
> +	for both --patterns-from-file and --pathspec-from-file. Pathspec elements
> +	are separated by LF or CR/LF. Pathspec elements can be quoted as
> +	explained for the configuration variable `core.quotePath` (see
> +	linkgit:git-config[1]). See also `--pathspec-file-nul` and global
> +	`--literal-pathspecs`.
> +
> +--pathspec-file-nul::
> +	Only meaningful with `--pathspec-from-file`. Pathspec elements are
> +	separated with NUL character and all other characters are taken
> +	literally (including newlines and quotes).

Does it make sense to have a corresponding --patterns-file-nul option? As in, is it possible for patterns to contain inline newlines? If it's not possible, then that option probably isn't necessary.

> +
>  -e::
>  	The next parameter is the pattern. This option has to be
>  	used for patterns starting with `-` and should be used in

Show 18 quoted lines

> @@ -1125,6 +1129,44 @@ test_expect_success 'grep --no-index descends into repos, but not .git' '
>  	)
>  '
>  
> +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
> +'

Could you please change these here-docs to be <<-\EOF and then indent the test case?

Show 9 quoted lines

> +
> +cat >pathspecs <<EOF
> +pathspec-file
> +unrelated-file
> +EOF
> +
> +cat >expected <<EOF
> +pathspec-file:bar
> +EOF

In an earlier email, I was wondering aloud why these two blocks were outside of the test case above. I think the answer to that is that you're following the existing style of the test case.

In that case, could I pester you to do some test clean up while you're at it? I think it'd be nice to move the cats into their respective test cases (with <<-\EOF) and also rename the files from 'expected' to 'expect'. But otherwise, I think it's fine as is as well.

Thanks,

Denton

Show 21 quoted lines

> +
> +test_expect_success 'grep --pathspec-from-file with file' '
> +	git grep --pathspec-from-file pathspecs "bar" >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'grep --pathspec-file with stdin' '
> +	git grep --pathspec-from-file - "bar" <pathspecs >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'grep with two stdin inputs fails' '
> +	test_must_fail git grep --pathspec-from-file - --patterns-from-file - <pathspecs
> +'
> +
>  test_expect_success 'setup double-dash tests' '
>  cat >double-dash <<EOF &&
>  --
> -- 
> 2.24.0.393.g34dc348eaf-goog
>