Re: [PATCH] fixup! use perl instead of sed
On Wed, May 10, 2017 at 12:38 AM, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > >> On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan >> wrote: >> ... >>> # Tweak the push output to make the push option outside the cert >>> # different, then replay it on a fresh dst, checking that ff is not >>> # deleted. >>> - sed -i "s/\\([^ ]\\)bar/\\1baz/" push && >>> + perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak && >>> prepare_dst && >>> git -C dst config receive.certnonceseed sekrit && >>> git -C dst config receive.advertisepushoptions 1 && >>> - git receive-pack dst out && >>> + git receive-pack dst out && >> >> The test should have a PERL prerequisite now, that's missing. > > For a single-liner like this, our stance has always been that t/ > scripts can assume _some_ version of Perl interpreter available for > use, cf. t/README where it lists prerequisites and explains them: > > - PERL > >Git wasn't compiled with NO_PERL=YesPlease. > >Even without the PERL prerequisite, tests can assume there is a >usable perl interpreter at $PERL_PATH, though it need not be >particularly modern. > > So unless "receive-pack" that is being tested here requires Perl at > runtime, we do not want PERL prerequisite for this test. Oops, sorry about that. >> Also using \1 will likely be deprecated in future versions of perl at >> some point: >> >> $ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/" >> \1 better written as $1 at -e line 1. >> hifoo > > Very good advice from a Perl expert; thanks. No problem. Hopefully my noisy advice will at least lead to fixing a bug in perl if there is one :)
Re: [PATCH] fixup! use perl instead of sed
Ævar Arnfjörð Bjarmason writes: > On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan wrote: > ... >> # Tweak the push output to make the push option outside the cert >> # different, then replay it on a fresh dst, checking that ff is not >> # deleted. >> - sed -i "s/\\([^ ]\\)bar/\\1baz/" push && >> + perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak && >> prepare_dst && >> git -C dst config receive.certnonceseed sekrit && >> git -C dst config receive.advertisepushoptions 1 && >> - git receive-pack dst out && >> + git receive-pack dst out && > > The test should have a PERL prerequisite now, that's missing. For a single-liner like this, our stance has always been that t/ scripts can assume _some_ version of Perl interpreter available for use, cf. t/README where it lists prerequisites and explains them: - PERL Git wasn't compiled with NO_PERL=YesPlease. Even without the PERL prerequisite, tests can assume there is a usable perl interpreter at $PERL_PATH, though it need not be particularly modern. So unless "receive-pack" that is being tested here requires Perl at runtime, we do not want PERL prerequisite for this test. > Also using \1 will likely be deprecated in future versions of perl at > some point: > > $ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/" > \1 better written as $1 at -e line 1. > hifoo Very good advice from a Perl expert; thanks.
Re: [PATCH] fixup! use perl instead of sed
On Tue, May 9, 2017 at 10:43 PM, Johannes Sixt wrote: > Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason: >> >> Finally, you can just use -i like you did with sed, no need for the >> tempfile: > > > Nope. Some implementations of perl attempt to remove the file that it has > just opened. That doesn't work on Windows. You have to supply a backup file > name as in `perl -i.bak ...` :-( This should have been fixed in 2002, and is in 5.8, the oldest perl release we support: https://github.com/Perl/perl5/commit/c030f24b81 & http://www.nntp.perl.org/group/perl.perl5.porters/2002/05/msg60275.html But maybe __CYGWIN__ isn't always defined on Windows, maybe this was a mingw build or something and perl was missing a test for this when support for that was added? This is obviously a trivial issue for git, but if it's a bug in perl I'd like to fix that. >> >> $ echo hibar >push >> $ perl -pi -e 's/([^ ])bar/$1baz/' push >> $ cat push >> hibaz > > > -- Hannes
Re: [PATCH] fixup! use perl instead of sed
On 05/09/2017 01:43 PM, Johannes Sixt wrote: Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason: Finally, you can just use -i like you did with sed, no need for the tempfile: Nope. Some implementations of perl attempt to remove the file that it has just opened. That doesn't work on Windows. You have to supply a backup file name as in `perl -i.bak ...` :-( $ echo hibar >push $ perl -pi -e 's/([^ ])bar/$1baz/' push $ cat push hibaz -- Hannes Thanks - sent a fixup [1] in reply to my PATCH v3 cover letter (but forgot to CC you). [1] <20170509210158.17898-1-jonathanta...@google.com>
Re: [PATCH] fixup! use perl instead of sed
Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason: Finally, you can just use -i like you did with sed, no need for the tempfile: Nope. Some implementations of perl attempt to remove the file that it has just opened. That doesn't work on Windows. You have to supply a backup file name as in `perl -i.bak ...` :-( $ echo hibar >push $ perl -pi -e 's/([^ ])bar/$1baz/' push $ cat push hibaz -- Hannes
Re: [PATCH] fixup! use perl instead of sed
On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan wrote: > Signed-off-by: Jonathan Tan > --- > > Thanks - I didn't realize the existence of the test lint. Here's a > fixup. Let me know if you prefer a full reroll. > > I had to use perl because "push" is a binary file (the first line > contains a NUL). > > > t/t5534-push-signed.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh > index 0ef6f66b5..3ee58dafb 100755 > --- a/t/t5534-push-signed.sh > +++ b/t/t5534-push-signed.sh > @@ -152,11 +152,11 @@ test_expect_success GPG 'inconsistent push options in > signed push not allowed' ' > # Tweak the push output to make the push option outside the cert > # different, then replay it on a fresh dst, checking that ff is not > # deleted. > - sed -i "s/\\([^ ]\\)bar/\\1baz/" push && > + perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak && > prepare_dst && > git -C dst config receive.certnonceseed sekrit && > git -C dst config receive.advertisepushoptions 1 && > - git receive-pack dst out && > + git receive-pack dst out && The test should have a PERL prerequisite now, that's missing. Also using \1 will likely be deprecated in future versions of perl at some point: $ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/" \1 better written as $1 at -e line 1. hifoo Finally, you can just use -i like you did with sed, no need for the tempfile: $ echo hibar >push $ perl -pi -e 's/([^ ])bar/$1baz/' push $ cat push hibaz > git -C dst rev-parse ff && > grep "inconsistent push options" out > ' > -- > 2.13.0.rc2.291.g57267f2277-goog >
