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

From: Victor Leschuk <vleschuk@accesssoftek.com>
Date: Oct 27, 2015, 05:25 UTC
Message-ID: <6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9FC@mail.accesssoftek.com>
In-Reply-To: <20151026193241.GO19802@serenity.lan>

Hello John,

see comments inline.

Show 5 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?

-- Best Regards, Victor