RE: [PATCH v3] Add git-grep threads param
>> 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?
Show 6 quoted lines
> 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 */
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?
--
Best Regards,
Victor