Re: [PATCH v2 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

[Jeff King <peff@peff.net>]

On Mon, Apr 08, 2024 at 06:22:10PM +0200, Patrick Steinhardt wrote:

> On Mon, Apr 08, 2024 at 09:07:32AM -0700, Junio C Hamano wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > 
> > > I was going to suggest that you could accomplish this more easily
> > > directly in shell (without employing `awk`):
> > >
> > >     {
> > >         echo start &&
> > >         printf "create refs/heads/branch-%d HEAD\n" $(test_seq 0 9999) &&
> > >         echo commit
> > >     } >input &&
> > >
> > > but then I realized that that could potentially run afoul of
> > > command-line length limit on some platform due to the 0-9999 sequence.
> > 
> > As xargs is supposed to know the system limit, perhaps
> > 
> > 	test_seq 0 9999 | xargs printf "...%d...\n"
> > 
> > should work?
> 
> Is there a reason why we want to avoid using awk(1) in the first place?
> We use it in several other tests and I don't think that the resulting
> code is all that bad.

Using awk is fine, but I think in general if we can do something just as
easily without an extra process, that is preferable. Using xargs here
does not to me count as "just as easily". However, using a shell loop
might actually be more readable because you'd avoid the extra quoting.
I.e. either:

  for i in $(test_seq 10000)
  do
	echo "create refs/heads/branch-$i HEAD"
  done

or:

  i=0
  while test $i -lt 10000
  do
	echo "create refs/heads/branch-$i HEAD"
	i=$((i+1))
  done

I think the first one actually incurs an extra process anyway because of
the $(). The second one would not. Of course, the second one probably
needs &&-chaining and a "|| return 1" to work in our test snippet.

IMHO the nicest thing would be if our test_seq could take a format
parameter, like:

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2eccf100c0..c8f32eb409 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1404,6 +1404,13 @@ test_cmp_fspath () {
 # from 1.
 
 test_seq () {
+	local fmt="%d"
+	case "$1" in
+	-f)
+		fmt="$2"
+		shift 2
+		;;
+	esac
 	case $# in
 	1)	set 1 "$@" ;;
 	2)	;;
@@ -1412,7 +1419,7 @@ test_seq () {
 	test_seq_counter__=$1
 	while test "$test_seq_counter__" -le "$2"
 	do
-		echo "$test_seq_counter__"
+		printf "$fmt\n" "$test_seq_counter__"
 		test_seq_counter__=$(( $test_seq_counter__ + 1 ))
 	done
 }

Then you could just write:

  test_seq -f "create refs/heads/branch-%d HEAD" 0 9999

and I suspect there are several other spots which could be simplified as
well.

Anyway, I don't think it is worth derailing your series for this, but
just general food for thought.

-Peff