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

[Brandon Williams <bmwill@google.com>]

On 11/15, Stefan Beller wrote:
> > +       /*
> > +        * Limit number of threads for child process to use.
> > +        * This is to prevent potential fork-bomb behavior of git-grep as each
> > +        * submodule process has its own thread pool.
> > +        */
> > +       if (num_threads)
> > +               argv_array_pushf(&submodule_options, "--threads=%d",
> > +                                (num_threads + 1) / 2);
> 
> I think you would want to pass --threads=%d unconditionally,
> as it also serves as a weak defusal for fork bombs. Is it possible to come here
> with num_threads=0? (i.e. what happens if the user doesn't specify the number
> of threads or such, do we fall back to some default or is it just 0?)
> 
> I have seen some other places that check for num_threads unequal to 0,
> as e.g. no mutex needs to be locked then (assuming we don't have any
> thread but grep within the main process), but as you intend to use this also
> as a helper to not blow up the number of threads recursively, we'd need to
> pass at a number != 0 here?

The option parsing logic in cmd_grep handles the cases where num_threads
is some odd value (and fails if <0).  In the case where it is 0, it will
default to 8 under certain circumstances.  I figured I would just let
that logic handle the cases where num_theads ends up being 0 instead of
explicitly passing threads=1.  You can't pass threads=0 in some cases
due to the default "oh look threads==0, looks like we should use 8!"
case.

> 
> > +
> > +       git grep -e "bar" --and -e "foo" --recurse-submodules > actual &&
> 
> nit here and in the tests below:
> We prefer to have no white space between > and the file piped to.

I'll fix that up everywhere.

-- 
Brandon Williams