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

From: Brandon Williams <bmwill@google.com>
Date: Mar 22, 2017, 21:46 UTC
Message-ID: <20170322214654.GC11254@google.com>
In-Reply-To: <CACsJy8Cu8cgtJzDDM09GoJr5Ny+G+nP17GjZjXng6ZPQv9eXXg@mail.gmail.com>

On 03/21, Duy Nguyen wrote:

Show 8 quoted lines

> 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.

Show 23 quoted lines

> 
> > @@ -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