Re: [PATCH v3] Add git-grep threads param
From: John Keeping <hidden>
Date: 2016-06-15 23:07:04
On Tue, Oct 27, 2015 at 06:54:16AM -0700, Victor Leschuk wrote:
Hello John,quoted
quoted
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?quoted
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.quoted
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.quoted
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.
quoted hunk ↗ jump to hunk
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.