From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 24 Jun 2025 14:03:01 GMT
Subject: Re: [GSoC RFC PATCH v2 5/7] repo-info: add the field references.format
Message-ID: <254e4819-a693-4fb7-aa92-260038cbfbe2@gmail.com>
In-Reply-To: <20250619225751.99699-6-lucasseikioshiro@gmail.com>

Hi Lucas

On 19/06/2025 23:57, Lucas Seiki Oshiro wrote:
> Add the field references.format to the repo-info command. The data
> retrieved in this field is the same that currently is obtained by
> running `git rev-parse --show-ref-format`.
> 
> Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> Mentored-by Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>

I've concentrated my comments on the tests as others have commented on 
the code itself. In general test bodies should be wrapped in single 
quotes rather than double quotes and one should prefer test_cmp() over 
test_line_count().

> +# Test if a field is correctly returned in both plaintext and json formats.
> +#
> +# Usage: test_repo_info <label> <init command> <key> <expected value>
> +#
> +# Arguments:
> +#   label: the label of the test
> +#   init command: a command that creates a repository called 'repo', configured
> +#      accordingly to what is being tested
> +#   key: the key of the field that is being tested
> +#   expected value: the value that the field should contain
> +test_repo_info () {
> +        label=$1
> +        init_command=$2
> +        key=$3
> +        expected_value=$4
> +
> +        test_expect_success PERLJSON "json: $label" "

This double quote should be a single quote. Unlike the test test title, 
the body is passed to eval so there is no need to use double quotes to 
expand shell variables. Indeed doing so is counter productive as it 
means we pass the result of the variable expansion to eval rather that 
the variable name.

> +                test_when_finished 'rm -rf repo' &&
> +                '$SHELL_PATH' -c '$init_command' &&

There is no need to fork a separate shell process here, you can use

     eval "$init_command"

instead.

> +                cd repo &&

If you change directory in a test then you must do so in a subshell so 
that we return to the original directory when the test finishes. In this 
case you're only running a single command in repo so you can use

     git -C repo repo-info ...

instead and avoid using "cd" all together.

> +                echo '$expected_value' >expect &&
> +                git repo-info '$key' >output &&
> +                cat output | parse_json >parsed &&
> +                grep -F 'row[0].$key' parsed | cut -d ' ' -f 2 >value &&
> +                cat value | sed 's/^0$/false/' | sed 's/^1$/true/' >actual &&

sed accepts filenames so there is no need to use "cat" here. It also 
accepts multiple expressions so you only need a single command

     sed "s/^0\$/false/; s/^1\$/true/" value >actual &&

> +                test_cmp expect actual
> +        "

Putting all of the above together the test looks like

	test_expect_success PERLJSON "json: $label" '
		test_when_finished "rm -rf repo" &&
		eval "$init_command" &&
		echo "$expected_value" >expect &&
		git -C repo repo-info "$key" >output &&
		cat output | parse_json >parsed &&
		grep -F "row[0].$key" parsed | cut -d " " -f 2 >value &&
		sed "s/^0\$/false/; s/^1\$/true/" value >actual &&
		test_cmp expect actual
	'

> +        test_expect_success "plaintext: $label" "
> +                test_when_finished 'rm -rf repo' &&
> +                '$SHELL_PATH' -c '$init_command' &&
> +                cd repo &&
> +                echo '$expected_value' >expect &&
> +                git repo-info --format=plaintext '$key' >output &&
> +                cat output | cut -d '=' -f 2 >actual &&
> +                test_cmp expect actual
> +        "

My comments above apply here as well.

> [...]
> +test_expect_success 'plaintext: output all default fields' "

The body should be single quoted as it is eval'd

> +	git repo-info --format=plaintext >actual &&
> +	test_line_count = $DEFAULT_NUMBER_OF_FIELDS actual
test_line_count is a pretty weak assertion. It would be better to use 
test_cmp()

	git repo-info --format=plaintext >actual &&
	sort actual >actual.sorted &&
	cat >expect <<-\EOF &&
	<EXPECTED OUTPUT>
	EOF
	test_cmp expect actual.sorted

> +"
> +
> +test_expect_success PERLJSON 'json: output all default fields' "
> +	git repo-info --format=json > output &&
> +	cat output | parse_json | grep '.*\..*\..*' >actual &&

You do not need to use "cat" here as you can redirect the standard input 
when you run parse_json.

	parse_json <output | grep ...

> +	test_line_count = $DEFAULT_NUMBER_OF_FIELDS actual
This is an even weaker assertion as the number of lines in a json file 
is not related to its content. Ideally we should normalize the json 
output into a standard format and use test_cmp. I don't know how 
practical that is.

Best Wishes

Phillip