On 05/11, Ævar Arnfjörð Bjarmason wrote:
> On Thu, May 11, 2017 at 10:21 PM, Brandon Williams <bmwill@xxxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >> 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