public inbox for git@vger.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 2/2] test-lib: teach test_seq the -f option
Date: Mon, 23 Jun 2025 09:10:14 -0700	[thread overview]
Message-ID: <xmqqikkm782h.fsf@gitster.g> (raw)
In-Reply-To: <20250623105625.GB654412@coredump.intra.peff.net> (Jeff King's message of "Mon, 23 Jun 2025 06:56:25 -0400")

Jeff King <peff@peff.net> writes:

> The "seq" tool has a "-f" option to produce printf-style formatted
> lines. Let's teach our test_seq helper the same trick. This lets us get
> rid of some shell loops in test snippets (which are particularly verbose
> in our test suite because we have to "|| return 1" to keep the &&-chain
> going).
>
> This converts a few call-sites I found by grepping around the test
> suite. A few notes on these:
>
>   - In "seq", the format specifier is a "%g" float. Since test_seq only
>     supports integers, I've kept the more natural "%d" (which is what
>     these call sites were using already).

Yeah, that was the first thing I started wondering about after
learning that "seq --format" is a thing.

>   - Like "seq", test_seq automatically adds a newline to the specified
>     format. This is what all callers are doing already except for t0021,
>     but there we do not care about the exact format. We are just trying
>     to printf a large number of bytes to a file. It's not worth
>     complicating other callers or adding an option to avoid the newline
>     in that caller.

OK.  Other than the newline, the update to t0021 changes the actual
payload used in the test, but it does not affect the characteristic
of the data (like how compressible the result is) all that much, I
think.

>   - Most conversions are just replacing a shell loop (which does get rid
>     of an extra fork, since $() requires a subshell). In t0612 we can
>     replace an awk invocation, which I think makes the end result more
>     readable, as there's less quoting.

;-)

>   - In t7422 we can replace one loop, but sadly we have to leave the
>     loop directly above it. This is because that earlier loop wants to
>     include the seq value twice in the output, which test_seq does not
>     support (nor does regular seq). If you run:
>
>       test_seq -f "foo-%d %d" 10
>
>     the second "%d" will always be the empty string. You might naively
>     think that test_seq could add some extra arguments, like:
>
>       # 3 ought to be enough for anyone...
>       printf "$fmt\n" "$i "$i" $i"
>
>     but that just triggers printf to format multiple lines, one per
>     extra set of arguments.
>
>     So we'd have to actually parse the format string, figure out how
>     many "%" placeholders are there, and then feed it that many
>     instances of the sequence number. The complexity isn't worth it.

And it would deviate from the normal "seq", which would contaminate
our developers' mind, which is not a direction worth going.

The changes look all good.

Thanks.

  reply	other threads:[~2025-06-23 16:10 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-06-23 10:55 [PATCH 0/2] test_seq format option Jeff King
2025-06-23 10:55 ` [PATCH 1/2] t7422: replace confusing printf with echo Jeff King
2025-06-23 17:59   ` Eric Sunshine
2025-06-24 10:05     ` Jeff King
2025-06-23 10:56 ` [PATCH 2/2] test-lib: teach test_seq the -f option Jeff King
2025-06-23 16:10   ` Junio C Hamano [this message]
2025-06-23 16:25   ` Justin Tobler
2025-06-24 10:11     ` [PATCH 3/2] test-lib: document test_seq's "-f" option Jeff King
2025-06-25  0:02       ` Justin Tobler
2025-06-23 17:27   ` [PATCH 2/2] test-lib: teach test_seq the -f option Todd Zullinger
2025-06-24 10:16     ` Jeff King
2025-06-24 13:42       ` Todd Zullinger
2025-06-24  6:22   ` Eric Sunshine
2025-06-24 10:36     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqikkm782h.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox