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

From: John Keeping <john@keeping.me.uk>
Date: Oct 27, 2015, 11:52 UTC
Message-ID: <20151027115256.GQ19802@serenity.lan>
In-Reply-To: <6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9FC@mail.accesssoftek.com>

On Mon, Oct 26, 2015 at 10:25:41PM -0700, Victor Leschuk wrote:

Show 27 quoted lines

> >> @@ -22,6 +22,7 @@ SYNOPSIS
> >>          [--color[=<when>] | --no-color]
> >>          [--break] [--heading] [-p | --show-function]
> >>          [-A <post-context>] [-B <pre-context>] [-C <context>]
> >> +        [--threads <num>]
> 
> > Is this the best place for this option?  I know the current list isn't
> > sorted in any particular way, but here you're splitting up the set of
> > context options (`-A`, `-B`, `-C` and `-W`).
> 
> Agree, I'll move the option both here and in documentation.
> 
> >> -static int wait_all(void)
> >> +static int wait_all(struct grep_opt *opt)
> 
> > I'm not sure passing a grep_opt in here is the cleanest way to do this.
> > Options are a UI concept and all we care about here is the number of
> > threads.
> 
> > Since `threads` is a global, shouldn't the number of threads be a global
> > as well?  Could we reuse `use_threads` here (possibly renaming it
> > `num_threads`)?
> 
> 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?