Re: [PATCH 28/29] grep: given --threads with NO_PTHREADS=YesPlease, warn

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, May 11, 2017 at 10:21 PM, Brandon Williams <bmwill@google.com> wrote:
> On 05/11, Ævar Arnfjörð Bjarmason wrote:
>> Add a warning about missing thread support when grep.threads or
>> --threads is set to a non 0 (default) or 1 (no parallelism) value
>> under NO_PTHREADS=YesPlease.
>>
>> This is for consistency with the index-pack & pack-objects commands,
>> which also take a --threads option & are configurable via
>> pack.threads, and have long warned about the same under
>> NO_PTHREADS=YesPlease.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  builtin/grep.c  | 11 +++++++++++
>>  t/t7810-grep.sh | 18 ++++++++++++++++++
>>  2 files changed, 29 insertions(+)
>>
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index 1c0adb30f3..f4e08dd2b6 100644
>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -289,6 +289,15 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
>>               if (num_threads < 0)
>>                       die(_("invalid number of threads specified (%d) for %s"),
>>                           num_threads, var);
>> +#ifdef NO_PTHREADS
>> +             else if (num_threads && num_threads != 1) {
>> +                     /* TRANSLATORS: %s is the configuration
>> +                        variable for tweaking threads, currently
>> +                        grep.threads */
>
> nit: this comment isn't formatted properly:
>   /*
>    * ... comment ...
>    */

Comments for translators use a different style, see cbcfd4e3ea ("i18n:
mention "TRANSLATORS:" marker in Documentation/CodingGuidelines",
2014-04-18). Otherwise the "*" gets interpolated into the string the
translators see in their UI.

>> +                     warning(_("no threads support, ignoring %s"), var);
>> +                     num_threads = 0;
>> +             }
>> +#endif
>>       }
>>
>>       return st;
>> @@ -1233,6 +1242,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>       else if (num_threads < 0)
>>               die(_("invalid number of threads specified (%d)"), num_threads);
>>  #else
>> +     if (num_threads)
>> +             warning(_("no threads support, ignoring --threads"));
>>       num_threads = 0;
>>  #endif
>>
>> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
>> index 561709ef6a..f106387820 100755
>> --- a/t/t7810-grep.sh
>> +++ b/t/t7810-grep.sh
>> @@ -791,6 +791,24 @@ do
>>       "
>>  done
>>
>> +test_expect_success !PTHREADS,C_LOCALE_OUTPUT 'grep --threads=N or pack.threads=N warns when no pthreads' '
>> +     git grep --threads=2 Hello hello_world 2>err &&
>> +     grep ^warning: err >warnings &&
>> +     test_line_count = 1 warnings &&
>> +     grep -F "no threads support, ignoring --threads" err &&
>> +     git -c grep.threads=2 grep Hello hello_world 2>err &&
>> +     grep ^warning: err >warnings &&
>> +     test_line_count = 1 warnings &&
>> +     grep -F "no threads support, ignoring grep.threads" err &&
>> +     git -c grep.threads=2 grep --threads=4 Hello hello_world 2>err &&
>> +     grep ^warning: err >warnings &&
>> +     test_line_count = 2 warnings &&
>> +     grep -F "no threads support, ignoring --threads" err &&
>> +     grep -F "no threads support, ignoring grep.threads" err &&
>> +     git -c grep.threads=0 grep --threads=0 Hello hello_world 2>err &&
>> +     test_line_count = 0 err
>> +'
>> +
>
> Same bit about doing the correct checks on the error strings to account
> for translation.

Do you mean why not use test_i18ngrep? The test is guarded by
C_LOCALE_OUTPUT which does the same thing, the whole thing is testing
output so no point in doing just parts of it IMO, unlike some other
tests that just end in "let's compare the output" but actually test
other stuff.

I could e.g. test that there's something on stderr under poison, but
no point in doing so.

>>  test_expect_success 'grep from a subdirectory to search wider area (1)' '
>>       mkdir -p s &&
>>       (
>> --
>> 2.11.0
>>
>
> --
> Brandon Williams