diff mbox series

[1/7] Redirect grep's stderr top null too.

Message ID 20200518100356.29292-1-dtucker@dtucker.net (mailing list archive)
State New, archived
Headers show
Series [1/7] Redirect grep's stderr top null too. | expand

Commit Message

Darren Tucker May 18, 2020, 10:03 a.m. UTC
Prevents pollution of configure output on platforms that don't have
grep -a.

Signed-off-by: Darren Tucker <dtucker@dtucker.net>
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Đoàn Trần Công Danh May 18, 2020, 2:13 p.m. UTC | #1
Hi Darren,

On 2020-05-18 20:03:50+1000, Darren Tucker <dtucker@dtucker.net> wrote:
> Prevents pollution of configure output on platforms that don't have
> grep -a.

From your other's patch, I think you're in HP-UX,
would you mind also run the test.

Since t5703 also uses "grep -a"

> 
> Signed-off-by: Darren Tucker <dtucker@dtucker.net>
> ---
>  configure.ac | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 66aedb9288..4effc82b76 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -526,7 +526,7 @@ if test -n "$ASCIIDOC"; then
>  	esac
>  fi
>  
> -if grep -a ascii configure.ac >/dev/null; then
> +if grep -a ascii configure.ac >/dev/null 2>&1; then
>    AC_MSG_RESULT([Using 'grep -a' for sane_grep])
>    SANE_TEXT_GREP=-a
>  else
> -- 
> 2.21.3
>
Darren Tucker May 18, 2020, 2:29 p.m. UTC | #2
On Tue, 19 May 2020 at 00:13, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
>
> Hi Darren,
>
> On 2020-05-18 20:03:50+1000, Darren Tucker <dtucker@dtucker.net> wrote:
> > Prevents pollution of configure output on platforms that don't have
> > grep -a.
>
> From your other's patch, I think you're in HP-UX,

Yes (it's not my usual platform but I had occasion to test something
on it so revived an old system).

> would you mind also run the test.
>
> Since t5703 also uses "grep -a"

It fails with:
$ ./t5703-upload-pack-ref-in-want.sh
sed: There are too many commands for the s/\n// function.
ok 1 - setup repository
sed: There are too many commands for the s/\n// function.
not ok 2 - config controls ref-in-want advertisement
#
#               test-tool serve-v2 --advertise-capabilities >out &&
#               ! grep -a ref-in-want out &&
#
#               git config uploadpack.allowRefInWant false &&
#               test-tool serve-v2 --advertise-capabilities >out &&
#               ! grep -a ref-in-want out &&
#
#               git config uploadpack.allowRefInWant true &&
#               test-tool serve-v2 --advertise-capabilities >out &&
#               grep -a ref-in-want out
#
sed: There are too many commands for the s/\n// function.
ok 3 - invalid want-ref line
sed: There are too many commands for the s/\n// function.
ok 4 - basic want-ref
sed: There are too many commands for the s/\n// function.
ok 5 - multiple want-ref lines
sed: There are too many commands for the s/\n// function.
ok 6 - mix want and want-ref
sed: There are too many commands for the s/\n// function.
ok 7 - want-ref with ref we already have commit for
sed: There are too many commands for the s/\n// function.
FATAL: Unexpected exit with code 0

If I use gnu sed and native grep, only #2 fails as above.  If I use
gnu grep and gnu sed it passes with a warning from (gnu) printf:
$ ./t5703-upload-pack-ref-in-want.sh
printf: \3: invalid escape
ok 1 - setup repository
ok 2 - config controls ref-in-want advertisement
ok 3 - invalid want-ref line
ok 4 - basic want-ref
ok 5 - multiple want-ref lines
ok 6 - mix want and want-ref
ok 7 - want-ref with ref we already have commit for
ok 8 - setup repos for fetching with ref-in-want tests
ok 9 - fetching with exact OID
ok 10 - fetching multiple refs
ok 11 - fetching ref and exact OID
ok 12 - fetching with wildcard that does not match any refs
ok 13 - fetching with wildcard that matches multiple refs
# passed all 13 test(s)
# SKIP skipping test, git built without http support
1..13

Maybe you want AC_PROG_GREP?  That picks the gnu grep in /usr/local,
but plumbing it in is more involved.
Đoàn Trần Công Danh May 18, 2020, 3:30 p.m. UTC | #3
On 2020-05-19 00:29:47+1000, Darren Tucker <dtucker@dtucker.net> wrote:
> On Tue, 19 May 2020 at 00:13, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> >
> > Hi Darren,
> >
> > On 2020-05-18 20:03:50+1000, Darren Tucker <dtucker@dtucker.net> wrote:
> > > Prevents pollution of configure output on platforms that don't have
> > > grep -a.
> >
> > From your other's patch, I think you're in HP-UX,
> 
> Yes (it's not my usual platform but I had occasion to test something
> on it so revived an old system).
> 
> > would you mind also run the test.
> >
> > Since t5703 also uses "grep -a"
> 
> It fails with:
> $ ./t5703-upload-pack-ref-in-want.sh
> sed: There are too many commands for the s/\n// function.

I think this was introduced at 878f988350 (t/test-lib: teach
--chain-lint to detect broken &&-chains in subshells, 2018-07-11)

The chainlint.sed is too complicated for a mortal like me to
understand, I added Eric to Cc.

> ok 1 - setup repository
> sed: There are too many commands for the s/\n// function.
> not ok 2 - config controls ref-in-want advertisement
> #
> #               test-tool serve-v2 --advertise-capabilities >out &&
> #               ! grep -a ref-in-want out &&
> #
> #               git config uploadpack.allowRefInWant false &&
> #               test-tool serve-v2 --advertise-capabilities >out &&
> #               ! grep -a ref-in-want out &&
> #
> #               git config uploadpack.allowRefInWant true &&
> #               test-tool serve-v2 --advertise-capabilities >out &&
> #               grep -a ref-in-want out

I'm thinking about:
-----------8<------------
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index a34460f7d8..92ad5eeec0 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -49,15 +49,18 @@ test_expect_success 'setup repository' '
 
 test_expect_success 'config controls ref-in-want advertisement' '
 	test-tool serve-v2 --advertise-capabilities >out &&
-	! grep -a ref-in-want out &&
+	perl -ne "/ref-in-want/ and print" out >out.filter &&
+	test_must_be_empty out.filter &&
 
 	git config uploadpack.allowRefInWant false &&
 	test-tool serve-v2 --advertise-capabilities >out &&
-	! grep -a ref-in-want out &&
+	perl -ne "/ref-in-want/ and print" out >out.filter &&
+	test_must_be_empty out.filter &&
 
 	git config uploadpack.allowRefInWant true &&
 	test-tool serve-v2 --advertise-capabilities >out &&
-	grep -a ref-in-want out
+	perl -ne "/ref-in-want/ and print" out >out.filter &&
+	test_file_not_empty out.filter
 '
 
 test_expect_success 'invalid want-ref line' '
------------------->8-----------

> #
> sed: There are too many commands for the s/\n// function.
> ok 3 - invalid want-ref line
> sed: There are too many commands for the s/\n// function.
> ok 4 - basic want-ref
> sed: There are too many commands for the s/\n// function.
> ok 5 - multiple want-ref lines
> sed: There are too many commands for the s/\n// function.
> ok 6 - mix want and want-ref
> sed: There are too many commands for the s/\n// function.
> ok 7 - want-ref with ref we already have commit for
> sed: There are too many commands for the s/\n// function.
> FATAL: Unexpected exit with code 0
> 
> If I use gnu sed and native grep, only #2 fails as above.  If I use
> gnu grep and gnu sed it passes with a warning from (gnu) printf:
> $ ./t5703-upload-pack-ref-in-want.sh
> printf: \3: invalid escape

Look like HP-UX's printf doesn't understand octal escape.
This may be a problem, since we've a patch to fix git-bisect's replace
for Windows, which is using "printf \015"
Cc-ing Junio for checking.
Junio C Hamano May 18, 2020, 6:20 p.m. UTC | #4
Darren Tucker <dtucker@dtucker.net> writes:

> Subject: Re: [PATCH 1/7] Redirect grep's stderr top null too.

Our patch title typically looks like

	area: title without initial Capitalization and full stop

Perhaps

	Subject: autoconf: redirect grep's stderr to null

> Prevents pollution of configure output on platforms that don't have
> grep -a.

Our log message typically describes the state without this patch in
the current tense to highlight the problem being solved, and then
orders the person who is updating the system to "make the codebase
like so", e.g.

	When the tested platform's 'grep' does not support the '-a'
	option, an error message would be given to its standard
	error stream, polluting the output.

	Redirect the error message to /dev/null, in addition to the
	standard output, to squelch it.

or something along that style.

Thanks.

>
> Signed-off-by: Darren Tucker <dtucker@dtucker.net>
> ---
>  configure.ac | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index 66aedb9288..4effc82b76 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -526,7 +526,7 @@ if test -n "$ASCIIDOC"; then
>  	esac
>  fi
>  
> -if grep -a ascii configure.ac >/dev/null; then
> +if grep -a ascii configure.ac >/dev/null 2>&1; then
>    AC_MSG_RESULT([Using 'grep -a' for sane_grep])
>    SANE_TEXT_GREP=-a
>  else
Darren Tucker May 18, 2020, 11:22 p.m. UTC | #5
On Tue, 19 May 2020 at 01:30, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
[...]
> > printf: \3: invalid escape
>
> Look like HP-UX's printf doesn't understand octal escape.

The HP-UX one is actually OK with that.  The error is from an old gnu
coreutils (2.0), and it's complaining because there no leading zero,
which POSIX says octal escapes have:
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/printf.html

"""
The following <backslash>-escape sequences shall be supported:
[...]
"\0ddd", where ddd is a zero, one, two, or three-digit octal number
that shall be converted to a byte with the numeric value specified by
the octal number
"""

$ /usr/local/bin/printf '\03' | od -x
0000000 0300
0000001
$ /usr/local/bin/printf '\3' | od -x
/usr/local/bin/printf: \3: invalid escape
0000000
Eric Sunshine May 19, 2020, 4:09 p.m. UTC | #6
On Mon, May 18, 2020 at 11:30 AM Đoàn Trần Công Danh
<congdanhqx@gmail.com> wrote:
> On 2020-05-19 00:29:47+1000, Darren Tucker <dtucker@dtucker.net> wrote:
> > $ ./t5703-upload-pack-ref-in-want.sh
> > sed: There are too many commands for the s/\n// function.
>
> I think this was introduced at 878f988350 (t/test-lib: teach
> --chain-lint to detect broken &&-chains in subshells, 2018-07-11)
>
> The chainlint.sed is too complicated for a mortal like me to
> understand, I added Eric to Cc.

That's a rather weird error message; seems like that 'sed' is somewhat broken.

Back when Ævar was trying to get chain-lint to work on some really old
and broken platforms, it was ultimately decided (if I recall
correctly) that it wasn't worth the effort, and that chain-lint should
simply be disabled via GIT_TEST_CHAIN_LINT=0 or --no-chain-lint on
those platforms.

After all, chain-lint exists only to ferret out a specific problem in
_newly-written_ tests (it's testing the tests), not to ferret out
problems in Git functionality (that's what the test suite itself is
for). So it's not a great loss to disable chain-lint on an old or
broken platform on which it is unlikely someone will be developing
_new_ tests. (And, even if someone does write a test on such a
platform, &&-chain breakage will be discovered soon enough once the
patch is posted to the mailing list and someone runs it on a
non-broken platform.)
diff mbox series

Patch

diff --git a/configure.ac b/configure.ac
index 66aedb9288..4effc82b76 100644
--- a/configure.ac
+++ b/configure.ac
@@ -526,7 +526,7 @@  if test -n "$ASCIIDOC"; then
 	esac
 fi
 
-if grep -a ascii configure.ac >/dev/null; then
+if grep -a ascii configure.ac >/dev/null 2>&1; then
   AC_MSG_RESULT([Using 'grep -a' for sane_grep])
   SANE_TEXT_GREP=-a
 else