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

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
To: git@vger.kernel.org
Cc: Denton Liu <liu.denton@gmail.com>, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2] grep: support the --pathspec-from-file option
Date: Thu, 5 Dec 2019 12:58:19 +0100	[thread overview]
Message-ID: <6af006eb-6d67-4eb5-c4f5-1677d6cbd120@syntevo.com> (raw)
In-Reply-To: <20191204203911.237056-1-emilyshaffer@google.com>

I'm excited to see someone else join my effort, thanks for continuing my 
effort! Also, less work for me :)

On 04.12.2019 21:39, Emily Shaffer wrote:

>   static int file_callback(const struct option *opt, const char *arg, int unset)
>   {
>   	struct grep_opt *grep_opt = opt->value;
> -	int from_stdin;
>   	FILE *patterns;
>   	int lno = 0;
>   	struct strbuf sb = STRBUF_INIT;
>   
>   	BUG_ON_OPT_NEG(unset);
>   
> -	from_stdin = !strcmp(arg, "-");
> -	patterns = from_stdin ? stdin : fopen(arg, "r");
> +	patterns_from_stdin = !strcmp(arg, "-");
> +
> +	if (patterns_from_stdin && pathspec_from_stdin)

To my understanding, this check will not work as expected. 
`file_callback` will be called at the moment of parsing args. 
`pathspec_from_stdin` is only initialized later.

Maybe it would be better to convert `file_callback` into a regular 
function and call it after the options were parsed, similar to how 
`pathspec_from_file` is parsed later?

This will also allow to move global variables into local scope and 
resolve other small issues raised by other reviewers.

> +test_expect_success 'grep with two stdin inputs fails' '
> +	test_must_fail git grep --pathspec-from-file - --patterns-from-file - <pathspecs
> +'
> +

It is usually a good idea to test for specific error, like this:

   test_must_fail git grep --pathspec-from-file - --patterns-from-file - 
<pathspecs 2>err &&
   test_i18ngrep "cannot specify both patterns and pathspec via stdin" 
err &&