From: Junio C Hamano <hidden> Date: 2016-06-15 23:08:19
Jeff King [off-list ref] writes:
Yeah, I'd worry that "-a" is not portable. OTOH, BSD grep seems to have
it, so between that and GNU, I think most systems are covered. We could
do:
test_lazy_prereq GREP_A '
echo foo | grep -a foo
'
and mark these tests with it. I'd also be happy to skip that step and
just do it if and when somebody actually complains about a system
without it (I wouldn't be surprised if most people on antique systems
end up installing GNU grep anyway).
Another option might be using "sed -ne '/^author/p'" or similar. But
that may very well just be trading one portability problem for another.
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
On Fri, Feb 19, 2016 at 09:38:17AM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
Yeah, I'd worry that "-a" is not portable. OTOH, BSD grep seems to have
it, so between that and GNU, I think most systems are covered. We could
do:
test_lazy_prereq GREP_A '
echo foo | grep -a foo
'
and mark these tests with it. I'd also be happy to skip that step and
just do it if and when somebody actually complains about a system
without it (I wouldn't be surprised if most people on antique systems
end up installing GNU grep anyway).
Another option might be using "sed -ne '/^author/p'" or similar. But
that may very well just be trading one portability problem for another.
Would $PERL help, I wonder?
It would, though I think you would need to call `binmode` to make it
reliable. I was hesitant to suggest it, because I seem to recall some
resistance to more perl dependencies in the test suite, but I think we
may be past the point of no return there, anyway.
-Peff
From: John Keeping <hidden> Date: 2016-06-15 23:08:19
On Fri, Feb 19, 2016 at 09:38:17AM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
Yeah, I'd worry that "-a" is not portable. OTOH, BSD grep seems to have
it, so between that and GNU, I think most systems are covered. We could
do:
test_lazy_prereq GREP_A '
echo foo | grep -a foo
'
and mark these tests with it. I'd also be happy to skip that step and
just do it if and when somebody actually complains about a system
without it (I wouldn't be surprised if most people on antique systems
end up installing GNU grep anyway).
Another option might be using "sed -ne '/^author/p'" or similar. But
that may very well just be trading one portability problem for another.
Would $PERL help, I wonder?
I suspect that any grep that lacks "-a" also lacks binary file handling
that will break these tests. I found a Solaris grep that doesn't
support "-a" and it treats these files as text.
From that perspective, it would be better to have a central place that
deals with figuring out how to get grep to work for us. Perhaps we need
test_grep to get this right. We already have test_cmp_bin() as a thin
wrapper around cmp so I don't think this is completely unprecedented.
From: Jeff King <hidden> Date: 2016-06-15 23:08:19
On Fri, Feb 19, 2016 at 07:23:11PM +0000, John Keeping wrote:
I suspect that any grep that lacks "-a" also lacks binary file handling
that will break these tests. I found a Solaris grep that doesn't
support "-a" and it treats these files as text.
From that perspective, it would be better to have a central place that
deals with figuring out how to get grep to work for us. Perhaps we need
test_grep to get this right. We already have test_cmp_bin() as a thin
wrapper around cmp so I don't think this is completely unprecedented.
I think 99% of the time we are using grep for ascii text. As evidenced
by the number of test failures we see with the new grep, it is a small
minority that feed binary gibberish. I'd prefer if "-a" handling didn't
need to pollute anything outside of this narrow range of tests (and as
with my prereq suggestion, I am even find just skipping this narrow
range of tests on platforms with no "-a", though falling back to running
without "-a" is fine if it works).
-Peff
From: John Keeping <hidden> Date: 2016-06-15 23:08:20
On Fri, Feb 19, 2016 at 02:33:10PM -0500, Jeff King wrote:
On Fri, Feb 19, 2016 at 07:23:11PM +0000, John Keeping wrote:
quoted
I suspect that any grep that lacks "-a" also lacks binary file handling
that will break these tests. I found a Solaris grep that doesn't
support "-a" and it treats these files as text.
From that perspective, it would be better to have a central place that
deals with figuring out how to get grep to work for us. Perhaps we need
test_grep to get this right. We already have test_cmp_bin() as a thin
wrapper around cmp so I don't think this is completely unprecedented.
I think 99% of the time we are using grep for ascii text. As evidenced
by the number of test failures we see with the new grep, it is a small
minority that feed binary gibberish. I'd prefer if "-a" handling didn't
need to pollute anything outside of this narrow range of tests (and as
with my prereq suggestion, I am even find just skipping this narrow
range of tests on platforms with no "-a", though falling back to running
without "-a" is fine if it works).
I went with using sed in this series because it seems to be the simplest
and most compatible way to extract lines from the input. We don't need
any special casing to figure out if an implementation needs "-a" or if
it doesn't support that option and all the implementation I tested
support the constructs used here.
John Keeping (2):
t8005: avoid grep on non-ASCII data
t9200: avoid grep on non-ASCII data
t/t8005-blame-i18n.sh | 16 ++++++++--------
t/t9200-git-cvsexportcommit.sh | 2 +-
2 files changed, 9 insertions(+), 9 deletions(-)
--
2.7.1.503.g3cfa3ac
From: John Keeping <hidden> Date: 2016-06-15 23:08:20
GNU grep 2.23 detects the input used in this test as binary data so it
does not work for extracting lines from a file. We could add the "-a"
option to force grep to treat the input as text, but not all
implementations support that. Instead, use sed to extract the desired
lines since it will always treat its input as text.
While touching these lines, modernize the test style to avoid hiding the
exit status of "git blame" and remove a space following a redirection
operator.
Signed-off-by: John Keeping <redacted>
---
t/t8005-blame-i18n.sh | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
From: John Keeping <hidden> Date: 2016-06-15 23:08:20
GNU grep 2.23 detects the input used in this test as binary data so it
does not work for extracting lines from a file. We could add the "-a"
option to force grep to treat the input as text, but not all
implementations support that. Instead, use sed to extract the desired
lines since it will always treat its input as text.
Signed-off-by: John Keeping <redacted>
---
t/t9200-git-cvsexportcommit.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:20
On Sun, Feb 21, 2016 at 12:32 PM, John Keeping [off-list ref] wrote:
quoted hunk
GNU grep 2.23 detects the input used in this test as binary data so it
does not work for extracting lines from a file. We could add the "-a"
option to force grep to treat the input as text, but not all
implementations support that. Instead, use sed to extract the desired
lines since it will always treat its input as text.
While touching these lines, modernize the test style to avoid hiding the
exit status of "git blame" and remove a space following a redirection
operator.
Signed-off-by: John Keeping <redacted>
---
These tests all crash and burn with BSD sed (including Mac OS X) since
you're not restricting yourself to BRE (basic regular expressions).
You _could_ request extended regular expressions, which do work on
those platforms, as well as with GNU sed:
sed -nEe "/^(author|summary) /p" ...
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:20
On Sun, Feb 21, 2016 at 12:32 PM, John Keeping [off-list ref] wrote:
quoted hunk
GNU grep 2.23 detects the input used in this test as binary data so it
does not work for extracting lines from a file. We could add the "-a"
option to force grep to treat the input as text, but not all
implementations support that. Instead, use sed to extract the desired
lines since it will always treat its input as text.
Signed-off-by: John Keeping <redacted>
---
From: Jeff King <hidden> Date: 2016-06-15 23:08:20
On Sun, Feb 21, 2016 at 04:01:27PM -0500, Eric Sunshine wrote:
On Sun, Feb 21, 2016 at 12:32 PM, John Keeping [off-list ref] wrote:
quoted
GNU grep 2.23 detects the input used in this test as binary data so it
does not work for extracting lines from a file. We could add the "-a"
option to force grep to treat the input as text, but not all
implementations support that. Instead, use sed to extract the desired
lines since it will always treat its input as text.
While touching these lines, modernize the test style to avoid hiding the
exit status of "git blame" and remove a space following a redirection
operator.
Signed-off-by: John Keeping <redacted>
---
These tests all crash and burn with BSD sed (including Mac OS X) since
you're not restricting yourself to BRE (basic regular expressions).
You _could_ request extended regular expressions, which do work on
those platforms, as well as with GNU sed:
sed -nEe "/^(author|summary) /p" ...
At that point, I think we may as well use grep, because obscure
platforms are probably broken either way.
I'm tempted to just go the perl route. We already depend on at least a
baisc version of perl5 being installed for many of the other tests, so
it's not really introducing a new dependency.
Something like the patch below works for me. I think we could make it
shorter by using $PERLIO to get the raw behavior, but using binmode will
work even on ancient versions of perl.
John, if you agree on the direction, feel free to combine it with your
patch.
These tests all crash and burn with BSD sed (including Mac OS X) since
you're not restricting yourself to BRE (basic regular expressions).
You _could_ request extended regular expressions, which do work on
those platforms, as well as with GNU sed:
sed -nEe "/^(author|summary) /p" ...
At that point, I think we may as well use grep, because obscure
platforms are probably broken either way.
I came to the same conclusion but forgot to say so at the end of my message.
I'm tempted to just go the perl route. We already depend on at least a
baisc version of perl5 being installed for many of the other tests, so
it's not really introducing a new dependency.
Something like the patch below works for me. I think we could make it
shorter by using $PERLIO to get the raw behavior, but using binmode will
work even on ancient versions of perl.
+filter_blame () {
+ perl -e '
+ binmode STDIN;
+ binmode STDOUT;
I was worried about binmode() due to some vague recollection from
years and years ago of it being problematic on Windows, but I see
these tests are all protected by !MINGW anyhow...
From: Jeff King <hidden> Date: 2016-06-15 23:08:20
On Sun, Feb 21, 2016 at 06:31:08PM -0500, Eric Sunshine wrote:
quoted
Something like the patch below works for me. I think we could make it
shorter by using $PERLIO to get the raw behavior, but using binmode will
work even on ancient versions of perl.
+filter_blame () {
+ perl -e '
+ binmode STDIN;
+ binmode STDOUT;
I was worried about binmode() due to some vague recollection from
years and years ago of it being problematic on Windows, but I see
these tests are all protected by !MINGW anyhow...
Thanks for mentioning that. I meant to put a note on that at the end of
_my_ message, but forgot. :)
It does mean we won't do CRLF processing. We could get around that with
some explicit `chomp`-ing, I think. Or just leave it as-is and assume
these will lose the !MINGW prereq.
I see Junio just mentioned elsewhere that we can simply avoid the
extended regular expressions by using two sed commands. That would be
fine with me, too.
-Peff
From: John Keeping <hidden> Date: 2016-06-15 23:08:20
On Sun, Feb 21, 2016 at 06:19:14PM -0500, Jeff King wrote:
On Sun, Feb 21, 2016 at 04:01:27PM -0500, Eric Sunshine wrote:
quoted
On Sun, Feb 21, 2016 at 12:32 PM, John Keeping [off-list ref] wrote:
quoted
GNU grep 2.23 detects the input used in this test as binary data so it
does not work for extracting lines from a file. We could add the "-a"
option to force grep to treat the input as text, but not all
implementations support that. Instead, use sed to extract the desired
lines since it will always treat its input as text.
While touching these lines, modernize the test style to avoid hiding the
exit status of "git blame" and remove a space following a redirection
operator.
Signed-off-by: John Keeping <redacted>
---
These tests all crash and burn with BSD sed (including Mac OS X) since
you're not restricting yourself to BRE (basic regular expressions).
You _could_ request extended regular expressions, which do work on
those platforms, as well as with GNU sed:
sed -nEe "/^(author|summary) /p" ...
At that point, I think we may as well use grep, because obscure
platforms are probably broken either way.
Also GNU sed doesn't understand "-E", it uses "-r" for --regexp-extended.
I'm tempted to just go the perl route. We already depend on at least a
baisc version of perl5 being installed for many of the other tests, so
it's not really introducing a new dependency.
Something like the patch below works for me. I think we could make it
shorter by using $PERLIO to get the raw behavior, but using binmode will
work even on ancient versions of perl.
John, if you agree on the direction, feel free to combine it with your
patch.
My original sed version was:
sed -ne "/^author /p" -e "/^summary /p"
which I think will work on all platforms (we already use it in
t0000-basic.sh) but then I decided to be too clever :-(
I still think sed is simpler than introducing a new function to wrap a
perl script.
From: John Keeping <hidden> Date: 2016-06-15 23:08:20
On Sun, Feb 21, 2016 at 04:15:31PM -0500, Eric Sunshine wrote:
On Sun, Feb 21, 2016 at 12:32 PM, John Keeping [off-list ref] wrote:
quoted
GNU grep 2.23 detects the input used in this test as binary data so it
does not work for extracting lines from a file. We could add the "-a"
option to force grep to treat the input as text, but not all
implementations support that. Instead, use sed to extract the desired
lines since it will always treat its input as text.
Signed-off-by: John Keeping <redacted>
---
This works with BSD sed, but double negatives are confusing. Have you
considered this instead?
sed -ne '/^\//p' ...
What do you mean double negatives? Do you mean using "!" as an
alternative delimiter? I find changing delimters is normally simpler
than following multiple levels of quoting for escaping slashes, although
in this case it's simple enough that it doesn't make much difference.
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:20
On Sun, Feb 21, 2016 at 6:41 PM, John Keeping [off-list ref] wrote:
On Sun, Feb 21, 2016 at 06:19:14PM -0500, Jeff King wrote:
quoted
On Sun, Feb 21, 2016 at 04:01:27PM -0500, Eric Sunshine wrote:
quoted
These tests all crash and burn with BSD sed (including Mac OS X) since
you're not restricting yourself to BRE (basic regular expressions).
You _could_ request extended regular expressions, which do work on
those platforms, as well as with GNU sed:
sed -nEe "/^(author|summary) /p" ...
At that point, I think we may as well use grep, because obscure
platforms are probably broken either way.
Also GNU sed doesn't understand "-E", it uses "-r" for --regexp-extended.
It actually does recognize -E in all the versions I've tested,
however, apparently it's undocumented (thus probably should be
avoided).
My original sed version was:
sed -ne "/^author /p" -e "/^summary /p"
which I think will work on all platforms (we already use it in
t0000-basic.sh) but then I decided to be too clever :-(
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:20
On Sun, Feb 21, 2016 at 6:43 PM, John Keeping [off-list ref] wrote:
On Sun, Feb 21, 2016 at 04:15:31PM -0500, Eric Sunshine wrote:
quoted
On Sun, Feb 21, 2016 at 12:32 PM, John Keeping [off-list ref] wrote:
quoted
GNU grep 2.23 detects the input used in this test as binary data so it
does not work for extracting lines from a file. We could add the "-a"
option to force grep to treat the input as text, but not all
implementations support that. Instead, use sed to extract the desired
lines since it will always treat its input as text.
Signed-off-by: John Keeping <redacted>
---
This works with BSD sed, but double negatives are confusing. Have you
considered this instead?
sed -ne '/^\//p' ...
What do you mean double negatives? Do you mean using "!" as an
alternative delimiter? I find changing delimters is normally simpler
than following multiple levels of quoting for escaping slashes, although
in this case it's simple enough that it doesn't make much difference.
Nice, I learned something new today. If I recall correctly, historic
sed did not allow the delimiter to be changed (or it wasn't documented
or I simply forgot about the capability). So, feel free to ignore me.
From: Jeff King <hidden> Date: 2016-06-15 23:08:21
On Sun, Feb 21, 2016 at 11:41:35PM +0000, John Keeping wrote:
My original sed version was:
sed -ne "/^author /p" -e "/^summary /p"
which I think will work on all platforms (we already use it in
t0000-basic.sh) but then I decided to be too clever :-(
I still think sed is simpler than introducing a new function to wrap a
perl script.
Yeah, I think that is good (personally I'd use a function anyway, but I
think it is short enough that we could go either way).
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:08:21
On Sun, Feb 21, 2016 at 11:43:45PM +0000, John Keeping wrote:
On Sun, Feb 21, 2016 at 04:15:31PM -0500, Eric Sunshine wrote:
quoted
On Sun, Feb 21, 2016 at 12:32 PM, John Keeping [off-list ref] wrote:
quoted
GNU grep 2.23 detects the input used in this test as binary data so it
does not work for extracting lines from a file. We could add the "-a"
option to force grep to treat the input as text, but not all
implementations support that. Instead, use sed to extract the desired
lines since it will always treat its input as text.
Signed-off-by: John Keeping <redacted>
---
This works with BSD sed, but double negatives are confusing. Have you
considered this instead?
sed -ne '/^\//p' ...
What do you mean double negatives? Do you mean using "!" as an
alternative delimiter? I find changing delimters is normally simpler
than following multiple levels of quoting for escaping slashes, although
in this case it's simple enough that it doesn't make much difference.
I agree that changing delimiters is much nicer than backslashes. But I
wonder if using "!" is more confusing than it needs to be, given its
other meanings.
I dunno. I admit that the backslash threw me off, too (since it needs
escaped in interactive shells, I first assumed that's what was going
on). Using backslash to select the delimiter was new to me. I've usually
seen:
s!/foo/!/bar/!
which is arguably a little more clear. Too bad we cannot do:
m!/foo!
which I think reads better. Oh well. Maybe:
sed -ne '\#^/#p'
would be more readable, but I'm just bikeshedding at this point. The
grep invocation really was the most clear. :-/
-Peff