p5303: avoid sed GNU-ism

Commit Message

Jeff King Jan. 29, 2021, 8:04 p.m. UTC

On Fri, Jan 29, 2021 at 02:32:50PM -0500, Jeff King wrote:

> > this construct:
> > 
> > 	... |
> > 	sed -n '1~5p' |
> > 	head -n "$1" |
> >         ...
> > 
> > which is a GNUism.  Peff often says that very small population
> > actually run our perf suite, and this seems to corroborate the
> > conjecture.
> 
> Oops. Looks like I was the one who introduced that. Nobody seems to have
> complained, so I'm somewhat tempted to leave it. But it would not be too
> hard to replace with perl, I think.

Maybe worth doing this?

-- >8 --
Subject: [PATCH] p5303: avoid sed GNU-ism

Using "1~5" isn't portable. Nobody seems to have noticed, since perhaps
people don't tend to run the perf suite on more exotic platforms. Still,
it's better to set a good example.

We can use:

  perl -ne 'print if $. % 5 == 1'

instead. But we can further observe that perl does a good job of the
other parts of this pipeline, and fold the whole thing together.

Signed-off-by: Jeff King <[e-mail]>
---
 t/perf/p5303-many-packs.sh | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Eric Sunshine Jan. 29, 2021, 8:19 p.m. UTC | #1

On Fri, Jan 29, 2021 at 3:07 PM Jeff King <[e-mail]> wrote:
> Subject: [PATCH] p5303: avoid sed GNU-ism
>
> Using "1~5" isn't portable. Nobody seems to have noticed, since perhaps
> people don't tend to run the perf suite on more exotic platforms. Still,
> it's better to set a good example.

It's not just exotic platforms on which this can be a problem. BSD
lineage `sed`, such as stock `sed` on macOS, doesn't understand this
notation.

Thanks for eliminating this particular GNU-ism.

Jeff King Jan. 29, 2021, 8:27 p.m. UTC | #2

On Fri, Jan 29, 2021 at 03:19:31PM -0500, Eric Sunshine wrote:

> On Fri, Jan 29, 2021 at 3:07 PM Jeff King <[e-mail]> wrote:
> > Subject: [PATCH] p5303: avoid sed GNU-ism
> >
> > Using "1~5" isn't portable. Nobody seems to have noticed, since perhaps
> > people don't tend to run the perf suite on more exotic platforms. Still,
> > it's better to set a good example.
> 
> It's not just exotic platforms on which this can be a problem. BSD
> lineage `sed`, such as stock `sed` on macOS, doesn't understand this
> notation.
> 
> Thanks for eliminating this particular GNU-ism.

OK, then I'm doubly surprised nobody has noticed and complained about
this. :)

-Peff

Eric Sunshine Jan. 29, 2021, 8:36 p.m. UTC | #3

On Fri, Jan 29, 2021 at 3:28 PM Jeff King <[e-mail]> wrote:
> On Fri, Jan 29, 2021 at 03:19:31PM -0500, Eric Sunshine wrote:
> > It's not just exotic platforms on which this can be a problem. BSD
> > lineage `sed`, such as stock `sed` on macOS, doesn't understand this
> > notation.
>
> OK, then I'm doubly surprised nobody has noticed and complained about
> this. :)

Aside from there possibly being relatively few regular Git developers
using macOS, it could also be because it's difficult to run the perf
tests on macOS in the first place due to the GNU prerequisites. For
instance, the perf tests have an unconditional dependency on GNU
`time` which is not installed on macOS by default, and it's not always
easy to figure out how to obtain it.

Taylor Blau Jan. 29, 2021, 10:11 p.m. UTC | #4

On Fri, Jan 29, 2021 at 03:36:01PM -0500, Eric Sunshine wrote:
> On Fri, Jan 29, 2021 at 3:28 PM Jeff King <[e-mail]> wrote:
> > On Fri, Jan 29, 2021 at 03:19:31PM -0500, Eric Sunshine wrote:
> > > It's not just exotic platforms on which this can be a problem. BSD
> > > lineage `sed`, such as stock `sed` on macOS, doesn't understand this
> > > notation.
> >
> > OK, then I'm doubly surprised nobody has noticed and complained about
> > this. :)
>
> Aside from there possibly being relatively few regular Git developers
> using macOS, it could also be because it's difficult to run the perf
> tests on macOS in the first place due to the GNU prerequisites. For
> instance, the perf tests have an unconditional dependency on GNU
> `time` which is not installed on macOS by default, and it's not always
> easy to figure out how to obtain it.

Yep, I agree completely. I was going to say that this would produce a
conflict (albeit, a trivial one) with the series that this came out of.

But I think that we're better off abandoning that series for now until I
send a different version, so I think we should just go ahead an apply
this.


Thanks,
Taylor

Patch

diff --git a/t/perf/p5303-many-packs.sh b/t/perf/p5303-many-packs.sh
index f4c2ab0584..ce0c42cc9f 100755
--- a/t/perf/p5303-many-packs.sh
+++ b/t/perf/p5303-many-packs.sh
@@ -21,10 +21,14 @@  repack_into_n () {
 	mkdir staging &&
 
 	git rev-list --first-parent HEAD |
-	sed -n '1~5p' |
-	head -n "$1" |
-	perl -e 'print reverse <>' \
-	>pushes
+	perl -e '
+		my $n = shift;
+		while (<>) {
+			last unless @commits < $n;
+			push @commits, $_ if $. % 5 == 1;
+		}
+		print reverse @commits;
+	' "$1" >pushes
 
 	# create base packfile
 	head -n 1 pushes |