Re: [PATCH v2] grep: support the --pathspec-from-file option
From: Alexandr Miloslavskiy <hidden>
Date: 2019-12-05 11:58:34
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 &&