threads / patch / 56630

patchp3400: stop using tac(1)

Subject: [PATCH] p3400: stop using tac(1)

## tl;dr

7 messages between Oct 2, 2021 and Oct 7, 2021. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

René Scharfe· Oct 2, 2021, 17:44 UTC · lore

b3dfeebb92 (rebase: avoid computing unnecessary patch IDs, 2016-07-29) added a perf test that calls tac(1) from GNU core utilities. Support systems without it by reversing the generated list using sort -nr instead. sort(1) with options -n and -r is already used in other tests.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/perf/p3400-rebase.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/perf/p3400-rebase.sh +1 −2
diff --git a/t/perf/p3400-rebase.sh b/t/perf/p3400-rebase.sh
index 7a0bb29448..43d5a34e8c 100755
--- a/t/perf/p3400-rebase.sh
+++ b/t/perf/p3400-rebase.sh
@@ -18,7 +18,7 @@ test_expect_success 'setup rebasing on top of a lot of changes' '
 		test_tick &&
 		git commit -m commit$i unrelated-file$i &&
 		echo change$i >unrelated-file$i &&
-		test_seq 1000 | tac >>unrelated-file$i &&
+		test_seq 1000 | sort -nr >>unrelated-file$i &&
 		git add unrelated-file$i &&
 		test_tick &&
 		git commit -m commit$i-reverse unrelated-file$i ||
--
2.33.0
Jeff King· Oct 4, 2021, 08:31 UTC · re: René Scharfe · lore

Re: [PATCH] p3400: stop using tac(1)

On Sat, Oct 02, 2021 at 07:44:14PM +0200, René Scharfe wrote:
> b3dfeebb92 (rebase: avoid computing unnecessary patch IDs, 2016-07-29)
> added a perf test that calls tac(1) from GNU core utilities.  Support
> systems without it by reversing the generated list using sort -nr
> instead.  sort(1) with options -n and -r is already used in other tests.
Cute fix. With regular seq(1), this whole thing can become:
  seq 1000 -1 1

without the extra process, but our test_seq doesn't understand non-1 increments (nor comparisons besides -le). It wouldn't be that hard to teach it, but given that this is the first time we've wanted it, it may not be worth the effort.

-Peff
Show changes to t/test-lib-functions.sh +12 −5
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6348e8d733..76c8c0f2f6 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1149,15 +1149,22 @@ test_cmp_fspath () {
 
 test_seq () {
 	case $# in
-	1)	set 1 "$@" ;;
-	2)	;;
-	*)	BUG "not 1 or 2 parameters to test_seq" ;;
+	1)	set 1 1 "$1" ;;
+	2)	set "$1" 1 "$2" ;;
+	3)	;;
+	*)	BUG "not 1, 2, or 3 parameters to test_seq" ;;
 	esac
+	if test "$1" -lt "$3"
+	then
+		test_seq_cmp__=-le
+	else
+		test_seq_cmp__=-ge
+	fi
 	test_seq_counter__=$1
-	while test "$test_seq_counter__" -le "$2"
+	while test "$test_seq_counter__" $test_seq_cmp__ "$3"
 	do
 		echo "$test_seq_counter__"
-		test_seq_counter__=$(( $test_seq_counter__ + 1 ))
+		test_seq_counter__=$(( $test_seq_counter__ + $2 ))
 	done
 }
René Scharfe· Oct 5, 2021, 18:45 UTC · re: Jeff King · lore

Re: [PATCH] p3400: stop using tac(1)

Am 04.10.21 um 10:31 schrieb Jeff King:
Show 15 quoted lines
> On Sat, Oct 02, 2021 at 07:44:14PM +0200, René Scharfe wrote:
>
>> b3dfeebb92 (rebase: avoid computing unnecessary patch IDs, 2016-07-29)
>> added a perf test that calls tac(1) from GNU core utilities.  Support
>> systems without it by reversing the generated list using sort -nr
>> instead.  sort(1) with options -n and -r is already used in other tests.
>
> Cute fix. With regular seq(1), this whole thing can become:
>
>   seq 1000 -1 1
>
> without the extra process, but our test_seq doesn't understand non-1
> increments (nor comparisons besides -le). It wouldn't be that hard to
> teach it, but given that this is the first time we've wanted it, it may
> not be worth the effort.

Right. The original also allows "seq 1000 1", by the way. Not sure we need that either.

But when you say "without the extra process", I think:
test_seq () {
	set_step='END {step = first < last ? 1 : -1}'
	loop='END {for (; first <= last && step > 0 || first >= last && step < 0; first += step) print first}'
	case $# in
	1)      awk -v first=1    -v last="$1"    "$set_step $loop" ;;
	2)      awk -v first="$1" -v last="$2"    "$set_step $loop" ;;
	3)      awk -v first="$1" -v last="$3" -v step="$2" "$loop" ;;
	*)      BUG "not 1, 2, or 3 parameters to test_seq" ;;
	esac </dev/null
}
;-)
René
Jeff King· Oct 5, 2021, 19:38 UTC · re: René Scharfe · lore

Re: [PATCH] p3400: stop using tac(1)

On Tue, Oct 05, 2021 at 08:45:38PM +0200, René Scharfe wrote:
Show 19 quoted lines
> Am 04.10.21 um 10:31 schrieb Jeff King:
> > On Sat, Oct 02, 2021 at 07:44:14PM +0200, René Scharfe wrote:
> >
> >> b3dfeebb92 (rebase: avoid computing unnecessary patch IDs, 2016-07-29)
> >> added a perf test that calls tac(1) from GNU core utilities.  Support
> >> systems without it by reversing the generated list using sort -nr
> >> instead.  sort(1) with options -n and -r is already used in other tests.
> >
> > Cute fix. With regular seq(1), this whole thing can become:
> >
> >   seq 1000 -1 1
> >
> > without the extra process, but our test_seq doesn't understand non-1
> > increments (nor comparisons besides -le). It wouldn't be that hard to
> > teach it, but given that this is the first time we've wanted it, it may
> > not be worth the effort.
> 
> Right.  The original also allows "seq 1000 1", by the way.  Not sure we
> need that either.

I'm not sure what you mean by "original" here. "seq 1000 1" produces no output for me (nor does it work with test_seq).

> But when you say "without the extra process", I think:
I meant without the extra tac/sort process.
Show 10 quoted lines
> test_seq () {
> 	set_step='END {step = first < last ? 1 : -1}'
> 	loop='END {for (; first <= last && step > 0 || first >= last && step < 0; first += step) print first}'
> 	case $# in
> 	1)      awk -v first=1    -v last="$1"    "$set_step $loop" ;;
> 	2)      awk -v first="$1" -v last="$2"    "$set_step $loop" ;;
> 	3)      awk -v first="$1" -v last="$3" -v step="$2" "$loop" ;;
> 	*)      BUG "not 1, 2, or 3 parameters to test_seq" ;;
> 	esac </dev/null
> }

Right, that works. It does introduce an extra awk process per invocation, though I doubt that really matters all that much. The diff I showed in my earlier response works totally in shell, like the current test_seq().

-Peff
René Scharfe· Oct 5, 2021, 19:54 UTC · re: Jeff King · lore

Re: [PATCH] p3400: stop using tac(1)

Am 05.10.21 um 21:38 schrieb Jeff King:
Show 24 quoted lines
> On Tue, Oct 05, 2021 at 08:45:38PM +0200, René Scharfe wrote:
>
>> Am 04.10.21 um 10:31 schrieb Jeff King:
>>> On Sat, Oct 02, 2021 at 07:44:14PM +0200, René Scharfe wrote:
>>>
>>>> b3dfeebb92 (rebase: avoid computing unnecessary patch IDs, 2016-07-29)
>>>> added a perf test that calls tac(1) from GNU core utilities.  Support
>>>> systems without it by reversing the generated list using sort -nr
>>>> instead.  sort(1) with options -n and -r is already used in other tests.
>>>
>>> Cute fix. With regular seq(1), this whole thing can become:
>>>
>>>   seq 1000 -1 1
>>>
>>> without the extra process, but our test_seq doesn't understand non-1
>>> increments (nor comparisons besides -le). It wouldn't be that hard to
>>> teach it, but given that this is the first time we've wanted it, it may
>>> not be worth the effort.
>>
>> Right.  The original also allows "seq 1000 1", by the way.  Not sure we
>> need that either.
>
> I'm not sure what you mean by "original" here. "seq 1000 1" produces no
> output for me (nor does it work with test_seq).

I meant the non-shell one, i.e. seq(1). I somehow expected everyone to use the same, but of course there is GNU seq, which has "an omitted INCREMENT defaults to 1 even when LAST is smaller than FIRST" [1] and BSD seq with "When first is larger than last the default incr is -1" [2].

[1] https://www.man7.org/linux/man-pages/man1/seq.1.html [2] https://man.netbsd.org/seq.1

René
René Scharfe· Oct 7, 2021, 20:42 UTC · re: Jeff King · lore

Re: [PATCH] p3400: stop using tac(1)

Am 05.10.21 um 21:38 schrieb Jeff King:
> Right, that works. It does introduce an extra awk process per
> invocation, though I doubt that really matters all that much. The diff I
> showed in my earlier response works totally in shell, like the current
> test_seq().

In my mind, test is an external command. Wikipedia [1] says it has been a builtin since the early 80ies, and I couldn't find a shell without it. I wonder where I picked up that outdated assumption -- I'm not actually _that_ old. Time for an update..

René
[1] https://en.wikipedia.org/wiki/Test_(Unix)
Junio C Hamano· Oct 7, 2021, 23:44 UTC · re: René Scharfe · lore

Re: [PATCH] p3400: stop using tac(1)

René Scharfe <l.s.r@web.de> writes:
> In my mind, test is an external command.  Wikipedia [1] says it has been
> a builtin since the early 80ies, and I couldn't find a shell without it.
> I wonder where I picked up that outdated assumption -- I'm not actually
> _that_ old.  Time for an update..

Funny, as I was wondering about the same thing the other day in a totally different context.

Our older shell script (like tests) tends to strongly prefer "case ... esac" over "test" for this exact reason, e.g option parsing loop often uses

    while case $#,$1 in 0,*) break ;; *,-*) ;; *) break ;; esac
    do
	...
instead of
    while test $# -gt 0 && ...

to avoid "test". I'm not actually _that_ old, either, but my current suspicion is that those who traind us are old enough ;-)

← back to recent threads