Re: [PATCH v3 3/5] grep: fix bug when recursing with relative pathspec

[Brandon Williams <bmwill@google.com>]

On 03/21, Duy Nguyen wrote:
> On Sat, Mar 18, 2017 at 12:22 AM, Brandon Williams <bmwill@google.com> wrote:
> > With these two pieces of information a child process can correctly
> > interpret the pathspecs provided by the user as well as being able to
> > properly format its output relative to the directory the user invoked
> > the original command from.
> 
> This part can stand alone as a separate patch right? It would help
> focus on the pathspec thingy first.

I guess it could probably be factored out, though it is necessary for it
to work.  The issue I was running into was that when no pathspecs were
given it would create a 'prefix' pathspec.  So if we were in directory
'dir/' the pathspec that would get created would be:

ps.match = "dir/"
ps.original = "dir/"

Since it also set the original field it messed up when the child tried
to interpret the pathspecs again.  I solved this by just passing the raw
pathspec through to the child.

> 
> > @@ -399,13 +405,12 @@ static void run_pager(struct grep_opt *opt, const char *prefix)
> >  }
> >
> >  static void compile_submodule_options(const struct grep_opt *opt,
> > -                                     const struct pathspec *pathspec,
> > +                                     const char **argv,
> >                                       int cached, int untracked,
> >                                       int opt_exclude, int use_index,
> >                                       int pattern_type_arg)
> >  {
> >         struct grep_pat *pattern;
> > -       int i;
> >
> >         if (recurse_submodules)
> >                 argv_array_push(&submodule_options, "--recurse-submodules");
> 
> Side note. It would be awesome if you could make parse_options() (or a
> new function) do the reverse process: given a 'struct option' with
> valid data, spit out argv_array. Less worrying about git-grep having
> new option but not passed to subgrep by accident. You can have a new
> flag to tell it to ignore certain options if you don't want to pass
> all.

I thought about this for a second but didn't pursue it very far.  Mostly
because of how you would handle options with callback routines.  Maybe
if you want the option to be reversible you need to have an additional
callback routine to do the conversion?  I agree that having this sort of
functionality would be nice as it does save you from forgetting about
passing on options to a child process.

-- 
Brandon Williams