Re: [RFC PATCH v2 2/6] test-tool run-command testsuite: get shell from env

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 7, 2024, 20:55 UTC
Message-ID: <xmqqle7w6lkt.fsf@gitster.g>
In-Reply-To: <c8448406d71151714e89893208c46b8a4c369cb5.1706921262.git.steadmon@google.com>

Josh Steadmon <steadmon@google.com> writes:

Show 9 quoted lines

> When running tests through `test-tool run-command testsuite`, we
> currently hardcode `sh` as the command interpreter. As discussed in [1],
> this is incorrect, and we should be using the shell set in
> TEST_SHELL_PATH instead.
>
> Add a shell_path field in struct testsuite so that we can pass this to
> the task runner callback. If this is non-null, we'll use it as the
> argv[0] of the subprocess. Otherwise, we'll just execute the test
> program directly.

That sounds nice in theory, but ...

> When setting up the struct testsuite in testsuite(), use the value
> of TEST_SHELL_PATH if it's set, otherwise default to `sh`.

... this done in the testsuite() function, doesn't suite.shell_path always gets some non-NULL value? Perhaps in a later step we will add a mechanism to allow suite.shell_path to be NULL when we know we are running an executable, or something?

Leaving readers in a bit of suspense may, especially in a series that is short like this, be a good technique to entice them to keep reading, perhaps, but anyway, if that is what is intended, a simple "this feature is not used in this step, but we will take advantage of it soon in a later step" would be a good idea.