Re: [PATCH v3] Add git-grep threads param

[John Keeping <john@keeping.me.uk>]

On Tue, Oct 27, 2015 at 06:54:16AM -0700, Victor Leschuk wrote:
> Hello John,
> 
> >> This thought also crossed my mind, however we already pass grep_opt to
> >> start_threads() function, so I think passing it to wait_all() is not
> >> that ugly, and kind of symmetric. And I do not like the idea of
> >> duplicating same information in different places. What do you think?
> 
> > The grep_opt in start_threads() is being passed through to run(), so it
> > seems slightly different to me.  If the threads were being setup in
> > grep.c (as opposed to builtin/grep.c) then I'd agree that it belongs in
> > grep_opt, but since this is local to this particular user of the grep
> > infrastructure adding num_threads to the grep_opt structure at all feels
> > wrong to me.
> 
> > Note that I wasn't suggesting passing num_threads as a parameter to
> > wait_all(), but rather having it as global state that is accessed by
> > wait_all() in the same way as the `threads` array.
> 
> > If we rename use_threads to num_threads and just use that, then we only
> > have the information in one place don't we?
> 
> Yeah, I understood your idea. So we parse config_value directly to 
> 
> static int num_threads; /* old use_threads */

Presumably this is:

	static int num_threads = -1;

so that the default behaviour continues to work correctly.

> And use it internally in builtin/grep.c. I think you are right.
> 
> Looks like grep_cmd_config() is the right place to parse it. Something like:
> 
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -267,6 +267,8 @@ static int wait_all(struct grep_opt *opt)
>  static int grep_cmd_config(const char *var, const char *value, void *cb)
>  {
>         int st = grep_config(var, value, cb);
> +       if (thread_config(var, value, cb) < 0)
> +               st = -1;
>         if (git_color_default_config(var, value, cb) < 0)
>                 st = -1;
>         return st;
> 
> What do you think?

I'd be tempted to open code the "grep.threads" case in this function
rather than introducing a helper for a single variable, but I don't
think it matters either way.  This looks good.