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

[Brandon Williams <bmwill@google.com>]

On 05/11, Ævar Arnfjörð Bjarmason wrote:
> 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.
> 

Ah got it, I wasn't aware of that.

> >> +                     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.

Fair enough, and I didn't notice the C_LOCALE_OUTPUT.

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

-- 
Brandon Williams