Re: [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 9, 2024, 18:20 UTC
Message-ID: <ZZ2OhkDhq1yOp+q3@nand.local>
In-Reply-To: <6a4d6a56-ab6f-4557-a5a3-1713f57cbfc9@gmail.com>

On Tue, Jan 09, 2024 at 04:29:57PM +0100, Rubén Justo wrote:

> Soon we're going to need to pass configuration values to a command in
> test-tool.
>
> Let's teach test-tool to take config values via command line arguments.

I wasn't expecting a step like this to appear in this series. I don't have strong feelings about it, especially since test-tool helpers already understand $GIT_DIR/config when they rely on library code which implicitly reads configuration.

But it does seem odd to have test-tool invocations that intimately depend on a particular set of configuration values. At the very least, this step seems to encourage passing finely tuned configuration values to test-tool helpers, which I am not sure is a good idea.

Your patch message suggests that this will be useful in the following patch, which makes sense. But I wonder if it would be easier to avoid the test-tool entirely and call some Git command in a state that we expect to generate advice. Then we can test its output with various values of advice.adviceOff.

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  t/helper/test-tool.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

The patch itself looks reasonable, though.

Thanks, Taylor