Re: [PATCH 05/10] p5303: measure time to repack with keep

Taylor Blau [off-list ref] writes:

From: Jeff King <redacted>

Not a fault of this series at all, but before the precontext of the
first hunk, there is  

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.

On Thu, Jan 28, 2021 at 07:40:40PM -0800, Junio C Hamano wrote:

quoted

diff --git a/t/perf/p5303-many-packs.sh b/t/perf/p5303-many-packs.sh
index 277d22ec4b..85b077b72b 100755
--- a/t/perf/p5303-many-packs.sh
+++ b/t/perf/p5303-many-packs.sh

@@ -27,8 +27,11 @@ repack_into_n () {

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.

-Peff

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

quoted

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 <redacted>
---
 t/perf/p5303-many-packs.sh | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

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 |

-- 
2.30.0.759.g69d54d14a7

On Fri, Jan 29, 2021 at 3:07 PM Jeff King [off-list ref] 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.

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

On Fri, Jan 29, 2021 at 3:07 PM Jeff King [off-list ref] wrote:

quoted

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

On Fri, Jan 29, 2021 at 3:28 PM Jeff King [off-list ref] wrote:

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

quoted

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.

On Fri, Jan 29, 2021 at 03:36:01PM -0500, Eric Sunshine wrote:

On Fri, Jan 29, 2021 at 3:28 PM Jeff King [off-list ref] wrote:

quoted

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

quoted

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