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

From: Duy Nguyen <pclouds@gmail.com>
Date: Nov 30, 2015, 19:31 UTC
Message-ID: <CACsJy8AdRnW88uy+U-Q0TKf05KvDQLf3bDcKmqoLTDT3sAzg+w@mail.gmail.com>
In-Reply-To: <1447242770-20753-1-git-send-email-vleschuk@accesssoftek.com>

On Wed, Nov 11, 2015 at 12:52 PM, Victor Leschuk <vleschuk@gmail.com> wrote:

Show 6 quoted lines

> "git grep" can now be configured (or told from the command line)
>  how many threads to use when searching in the working tree files.
>
>  Changes to default behavior: number of threads now doesn't depend
>  on online_cpus(), e.g. if specific number is not configured
>  GREP_NUM_THREADS_DEFAULT (8) threads will be used even on 1-core CPU.

Why? (I'm asking for an explanation in the commit message so that I will not have to ask again in future)

Show 6 quoted lines

> @@ -206,7 +206,8 @@ static void start_threads(struct grep_opt *opt)
>                 strbuf_init(&todo[i].out, 0);
>         }
>
> -       for (i = 0; i < ARRAY_SIZE(threads); i++) {
> +       threads = xcalloc(num_threads, sizeof(pthread_t));

I think we usually go with sizeof(*threads), but not sure if it's just a personal taste or the preferred style for git.

Show 9 quoted lines

>  static int grep_cmd_config(const char *var, const char *value, void *cb)
>  {
>         int st = grep_config(var, value, cb);
> -       if (git_color_default_config(var, value, cb) < 0)
> +       if (grep_threads_config(var, value, cb) < 0)
> +               st = -1;
> +       else if (git_color_default_config(var, value, cb) < 0)
>                 st = -1;
>         return st;

Hm... isn't it simpler to just return -1 instead of assigning to st first? I think you could just merge grep_threads_config() in this function because it's not that complex to stay separate..

>  }

-- 
Duy