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

From: Emily Shaffer <emilyshaffer@google.com>
To: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Cc: git@vger.kernel.org, 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, 12 Dec 2019 20:00:42 -0800	[thread overview]
Message-ID: <20191213040042.GB135450@google.com> (raw)
In-Reply-To: <6af006eb-6d67-4eb5-c4f5-1677d6cbd120@syntevo.com>

On Thu, Dec 05, 2019 at 12:58:19PM +0100, Alexandr Miloslavskiy wrote:
> 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.

Yeah, I think this is a good idea for the reasons you stated, so I'll do
so.

> 
> > +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 &&

Sure. Thanks.