Re: [PATCH v2 4/6] grep: optionally recurse into submodules

[Brandon Williams <bmwill@google.com>]

On 11/01, Stefan Beller wrote:
> On Mon, Oct 31, 2016 at 3:38 PM, Brandon Williams <bmwill@google.com> wrote:
> 
> >
> > +--recurse-submodules::
> > +       Recursively search in each submodule that has been initialized and
> > +       checked out in the repository.
> > +
> 
> and warn otherwise.

I've been going back and forth on whether to warn the user...maybe
`grep` isn't really the right place for the warning?

> > +
> > +       /*
> > +        * Capture output to output buffer and check the return code from the
> > +        * child process.  A '0' indicates a hit, a '1' indicates no hit and
> > +        * anything else is an error.
> > +        */
> > +       status = capture_command(&cp, &w->out, 0);
> > +       if (status && (status != 1))
> 
> Does the user have enough information what went wrong?
> Is the child verbose enough, such that we do not need to give a
> die[_errno]("submodule processs failed") ?
good point...the output from the child is stored in a buffer and won't
actually get printed if this fails out.  Perhaps we should flush the
buffer and then die?

> > +               if (S_ISREG(ce->ce_mode) &&
> > +                   match_pathspec(pathspec, name.buf, name.len, 0, NULL,
> > +                                  S_ISDIR(ce->ce_mode) ||
> > +                                  S_ISGITLINK(ce->ce_mode))) {
> 
> Why do we have to pass the ISDIR and ISGITLINK here for the regular file
> case? ce_path_match and match_pathspec are doing the same thing?

I was simply doing what ce_path_match was doing.  And I needed to switch
to using match_pathspec instead because ce_path_match doesn't allow for
checking the super_prefix as part of the pathspec logic...Perhaps a
refactor (in the future) in the pathspec logic could do that via a flag?

> > +                          submodule_path_match(pathspec, name.buf, NULL)) {
> > +                       hit |= grep_submodule(opt, NULL, ce->name, ce->name);
> 
> What is the difference between the last two parameters?

Path and file name, in the cached case they are the same.

> > + * filename: name of the submodule including tree name of parent
> > + * path: location of the submodule
> 
> That sounds the same to me.
So they are similar.  path should be used as the directory to
chdir for the child process and it doesn't have the tree name prefixed
to it.

-- 
Brandon Williams