{"thread":{"id":"56630","subject":"[PATCH] p3400: stop using tac(1)","startedAt":"2021-10-02T17:44:29Z","lastAt":"2021-10-07T23:44:10Z","messageCount":7,"participants":["René Scharfe","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"437772","messageId":"85831cc2-307f-1be8-9bb3-c44028ad07fa@web.de","threadId":"56630","inReplyTo":null,"subject":"[PATCH] p3400: stop using tac(1)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-02T17:44:14Z","receivedAt":"2021-10-02T17:44:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"b3dfeebb92 (rebase: avoid computing unnecessary patch IDs, 2016-07-29)\nadded a perf test that calls tac(1) from GNU core utilities. Support\nsystems without it by reversing the generated list using sort -nr\ninstead. sort(1) with options -n and -r is already used in other tests.\n\nSigned-off-by: René Scharfe \n---\n t/perf/p3400-rebase.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/perf/p3400-rebase.sh b/t/perf/p3400-rebase.sh\nindex 7a0bb29448..43d5a34e8c 100755\n--- a/t/perf/p3400-rebase.sh\n+++ b/t/perf/p3400-rebase.sh\n@@ -18,7 +18,7 @@ test_expect_success 'setup rebasing on top of a lot of changes' '\n \t\ttest_tick &&\n \t\tgit commit -m commit$i unrelated-file$i &&\n \t\techo change$i >unrelated-file$i &&\n-\t\ttest_seq 1000 | tac >>unrelated-file$i &&\n+\t\ttest_seq 1000 | sort -nr >>unrelated-file$i &&\n \t\tgit add unrelated-file$i &&\n \t\ttest_tick &&\n \t\tgit commit -m commit$i-reverse unrelated-file$i ||\n--\n2.33.0\n"},{"id":"437861","messageId":"YVq752xjzYz+LTq6@coredump.intra.peff.net","threadId":"56630","inReplyTo":"85831cc2-307f-1be8-9bb3-c44028ad07fa@web.de","subject":"Re: [PATCH] p3400: stop using tac(1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-04T08:31:35Z","receivedAt":"2021-10-04T08:31:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 02, 2021 at 07:44:14PM +0200, René Scharfe wrote:\n\n> b3dfeebb92 (rebase: avoid computing unnecessary patch IDs, 2016-07-29)\n> added a perf test that calls tac(1) from GNU core utilities. Support\n> systems without it by reversing the generated list using sort -nr\n> instead. sort(1) with options -n and -r is already used in other tests.\n\nCute fix. With regular seq(1), this whole thing can become:\n\n seq 1000 -1 1\n\nwithout the extra process, but our test_seq doesn't understand non-1\nincrements (nor comparisons besides -le). It wouldn't be that hard to\nteach it, but given that this is the first time we've wanted it, it may\nnot be worth the effort.\n\n-Peff\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 6348e8d733..76c8c0f2f6 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1149,15 +1149,22 @@ test_cmp_fspath () {\n \n test_seq () {\n \tcase $# in\n-\t1)\tset 1 \"$@\" ;;\n-\t2)\t;;\n-\t*)\tBUG \"not 1 or 2 parameters to test_seq\" ;;\n+\t1)\tset 1 1 \"$1\" ;;\n+\t2)\tset \"$1\" 1 \"$2\" ;;\n+\t3)\t;;\n+\t*)\tBUG \"not 1, 2, or 3 parameters to test_seq\" ;;\n \tesac\n+\tif test \"$1\" -lt \"$3\"\n+\tthen\n+\t\ttest_seq_cmp__=-le\n+\telse\n+\t\ttest_seq_cmp__=-ge\n+\tfi\n \ttest_seq_counter__=$1\n-\twhile test \"$test_seq_counter__\" -le \"$2\"\n+\twhile test \"$test_seq_counter__\" $test_seq_cmp__ \"$3\"\n \tdo\n \t\techo \"$test_seq_counter__\"\n-\t\ttest_seq_counter__=$(( $test_seq_counter__ + 1 ))\n+\t\ttest_seq_counter__=$(( $test_seq_counter__ + $2 ))\n \tdone\n }\n \n"},{"id":"437993","messageId":"f6a1296f-f524-9042-7f88-9591522971af@web.de","threadId":"56630","inReplyTo":"YVq752xjzYz+LTq6@coredump.intra.peff.net","subject":"Re: [PATCH] p3400: stop using tac(1)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-05T18:45:38Z","receivedAt":"2021-10-05T18:45:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.10.21 um 10:31 schrieb Jeff King:\n> On Sat, Oct 02, 2021 at 07:44:14PM +0200, René Scharfe wrote:\n>\n>> b3dfeebb92 (rebase: avoid computing unnecessary patch IDs, 2016-07-29)\n>> added a perf test that calls tac(1) from GNU core utilities. Support\n>> systems without it by reversing the generated list using sort -nr\n>> instead. sort(1) with options -n and -r is already used in other tests.\n>\n> Cute fix. With regular seq(1), this whole thing can become:\n>\n> seq 1000 -1 1\n>\n> without the extra process, but our test_seq doesn't understand non-1\n> increments (nor comparisons besides -le). It wouldn't be that hard to\n> teach it, but given that this is the first time we've wanted it, it may\n> not be worth the effort.\n\nRight. The original also allows \"seq 1000 1\", by the way. Not sure we\nneed that either.\n\nBut when you say \"without the extra process\", I think:\n\ntest_seq () {\n\tset_step='END {step = first < last ? 1 : -1}'\n\tloop='END {for (; first <= last && step > 0 || first >= last && step < 0; first += step) print first}'\n\tcase $# in\n\t1) awk -v first=1 -v last=\"$1\" \"$set_step $loop\" ;;\n\t2) awk -v first=\"$1\" -v last=\"$2\" \"$set_step $loop\" ;;\n\t3) awk -v first=\"$1\" -v last=\"$3\" -v step=\"$2\" \"$loop\" ;;\n\t*) BUG \"not 1, 2, or 3 parameters to test_seq\" ;;\n\tesac Am 04.10.21 um 10:31 schrieb Jeff King:\n> > On Sat, Oct 02, 2021 at 07:44:14PM +0200, René Scharfe wrote:\n> >\n> >> b3dfeebb92 (rebase: avoid computing unnecessary patch IDs, 2016-07-29)\n> >> added a perf test that calls tac(1) from GNU core utilities. Support\n> >> systems without it by reversing the generated list using sort -nr\n> >> instead. sort(1) with options -n and -r is already used in other tests.\n> >\n> > Cute fix. With regular seq(1), this whole thing can become:\n> >\n> > seq 1000 -1 1\n> >\n> > without the extra process, but our test_seq doesn't understand non-1\n> > increments (nor comparisons besides -le). It wouldn't be that hard to\n> > teach it, but given that this is the first time we've wanted it, it may\n> > not be worth the effort.\n> \n> Right. The original also allows \"seq 1000 1\", by the way. Not sure we\n> need that either.\n\nI'm not sure what you mean by \"original\" here. \"seq 1000 1\" produces no\noutput for me (nor does it work with test_seq).\n\n> But when you say \"without the extra process\", I think:\n\nI meant without the extra tac/sort process.\n\n> test_seq () {\n> \tset_step='END {step = first < last ? 1 : -1}'\n> \tloop='END {for (; first <= last && step > 0 || first >= last && step < 0; first += step) print first}'\n> \tcase $# in\n> \t1) awk -v first=1 -v last=\"$1\" \"$set_step $loop\" ;;\n> \t2) awk -v first=\"$1\" -v last=\"$2\" \"$set_step $loop\" ;;\n> \t3) awk -v first=\"$1\" -v last=\"$3\" -v step=\"$2\" \"$loop\" ;;\n> \t*) BUG \"not 1, 2, or 3 parameters to test_seq\" ;;\n> \tesac }\n\nRight, that works. It does introduce an extra awk process per\ninvocation, though I doubt that really matters all that much. The diff I\nshowed in my earlier response works totally in shell, like the current\ntest_seq().\n\n-Peff\n"},{"id":"437999","messageId":"6c817b65-1699-72d2-892a-3610dcbdf6a5@web.de","threadId":"56630","inReplyTo":"YVyppmEGkNCxZ+5L@coredump.intra.peff.net","subject":"Re: [PATCH] p3400: stop using tac(1)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-05T19:54:00Z","receivedAt":"2021-10-05T19:54:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.10.21 um 21:38 schrieb Jeff King:\n> On Tue, Oct 05, 2021 at 08:45:38PM +0200, René Scharfe wrote:\n>\n>> Am 04.10.21 um 10:31 schrieb Jeff King:\n>>> On Sat, Oct 02, 2021 at 07:44:14PM +0200, René Scharfe wrote:\n>>>\n>>>> b3dfeebb92 (rebase: avoid computing unnecessary patch IDs, 2016-07-29)\n>>>> added a perf test that calls tac(1) from GNU core utilities. Support\n>>>> systems without it by reversing the generated list using sort -nr\n>>>> instead. sort(1) with options -n and -r is already used in other tests.\n>>>\n>>> Cute fix. With regular seq(1), this whole thing can become:\n>>>\n>>> seq 1000 -1 1\n>>>\n>>> without the extra process, but our test_seq doesn't understand non-1\n>>> increments (nor comparisons besides -le). It wouldn't be that hard to\n>>> teach it, but given that this is the first time we've wanted it, it may\n>>> not be worth the effort.\n>>\n>> Right. The original also allows \"seq 1000 1\", by the way. Not sure we\n>> need that either.\n>\n> I'm not sure what you mean by \"original\" here. \"seq 1000 1\" produces no\n> output for me (nor does it work with test_seq).\n\nI meant the non-shell one, i.e. seq(1). I somehow expected everyone to use\nthe same, but of course there is GNU seq, which has \"an omitted INCREMENT\ndefaults to 1 even when LAST is smaller than FIRST\" [1] and BSD seq with\n\"When first is larger than last the default incr is -1\" [2].\n\n[1] https://www.man7.org/linux/man-pages/man1/seq.1.html\n[2] https://man.netbsd.org/seq.1\n\nRené\n"},{"id":"438226","messageId":"a3377973-4f4a-376a-f02e-94a3b20dd9ad@web.de","threadId":"56630","inReplyTo":"YVyppmEGkNCxZ+5L@coredump.intra.peff.net","subject":"Re: [PATCH] p3400: stop using tac(1)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-07T20:42:08Z","receivedAt":"2021-10-07T20:42:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.10.21 um 21:38 schrieb Jeff King:\n> Right, that works. It does introduce an extra awk process per\n> invocation, though I doubt that really matters all that much. The diff I\n> showed in my earlier response works totally in shell, like the current\n> test_seq().\n\nIn my mind, test is an external command. Wikipedia [1] says it has been\na builtin since the early 80ies, and I couldn't find a shell without it.\nI wonder where I picked up that outdated assumption -- I'm not actually\n_that_ old. Time for an update..\n\nRené\n\n\n[1] https://en.wikipedia.org/wiki/Test_(Unix)\n"},{"id":"438249","messageId":"xmqqzgrkmmd7.fsf@gitster.g","threadId":"56630","inReplyTo":"a3377973-4f4a-376a-f02e-94a3b20dd9ad@web.de","subject":"Re: [PATCH] p3400: stop using tac(1)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-07T23:44:04Z","receivedAt":"2021-10-07T23:44:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> In my mind, test is an external command. Wikipedia [1] says it has been\n> a builtin since the early 80ies, and I couldn't find a shell without it.\n> I wonder where I picked up that outdated assumption -- I'm not actually\n> _that_ old. Time for an update..\n\nFunny, as I was wondering about the same thing the other day in a\ntotally different context.\n\nOur older shell script (like tests) tends to strongly prefer \"case\n... esac\" over \"test\" for this exact reason, e.g option parsing loop\noften uses\n\n while case $#,$1 in 0,*) break ;; *,-*) ;; *) break ;; esac\n do\n\t...\n\ninstead of\n\n while test $# -gt 0 && ...\n\nto avoid \"test\". I'm not actually _that_ old, either, but my\ncurrent suspicion is that those who traind us are old enough ;-)\n\n\n"}]}