Re: [GSoC RFC PATCH v2 2/7] repo-info: add the --format flag

From: Patrick Steinhardt <ps@pks.im>
Date: Jul 3, 2025, 11:31 UTC
Message-ID: <aGZqIJe2K3jGYeI3@pks.im>
In-Reply-To: <20250619225751.99699-3-lucasseikioshiro@gmail.com>

On Thu, Jun 19, 2025 at 07:57:46PM -0300, Lucas Seiki Oshiro wrote:

Show 25 quoted lines

> diff --git a/builtin/repo-info.c b/builtin/repo-info.c
> index a5c43e253f..cbe1475e30 100644
> --- a/builtin/repo-info.c
> +++ b/builtin/repo-info.c
> @@ -1,21 +1,73 @@
>  #include "builtin.h"
> +#include "json-writer.h"
>  #include "parse-options.h"
>  
> +enum output_format {
> +	FORMAT_JSON
> +};
> +
> +struct repo_info {
> +	struct repository *repo;
> +	enum output_format format;
> +};
> +
> +static void repo_info_init(struct repo_info *repo_info,
> +			   struct repository *repo,
> +			   char *format)
> +{
> +	repo_info->repo = repo;
> +
> +	if (format == NULL || !strcmp(format, "json"))

Nit: we typically don't compare to NULL explicitly.

	if (!format || !strcmp(format, "json"))

Show 21 quoted lines

> +		repo_info->format = FORMAT_JSON;
> +	else
> +		die("invalid format %s", format);
> +}
> +
> +static void repo_info_print_json(struct repo_info *repo_info UNUSED)
> +{
> +	struct json_writer jw;
> +
> +	jw_init(&jw);
> +
> +	jw_object_begin(&jw, 1);
> +	jw_end(&jw);
> +
> +	puts(jw.json.buf);
> +	jw_release(&jw);
> +}
> +
> +static void repo_info_print(struct repo_info *repo_info)
> +{
> +	enum output_format format = repo_info->format;

This variable indirection feels a bit pointless.

> +	switch (format) {
> +	case FORMAT_JSON:
> +		repo_info_print_json(repo_info);
> +		break;

We should probably `BUG()` in case there's any other unknown format value stored in `repo_info`.

Show 15 quoted lines

> +	}
> +}
> +
>  int cmd_repo_info(int argc,
>  		  const char **argv,
>  		  const char *prefix,
> -		  struct repository *repo UNUSED)
> +		  struct repository *repo)
>  {
>  	const char *const repo_info_usage[] = {
>  		"git repo-info",
>  		NULL
>  	};
> +	struct repo_info repo_info;
> +	char *format = NULL;

This should be const.

Show 24 quoted lines

>  	struct option options[] = {
> +		OPT_STRING(0, "format", &format, N_("format"),
> +			   N_("output format")),
>  		OPT_END()
>  	};
>  
>  	argc = parse_options(argc, argv, prefix, options, repo_info_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN_OPT);
> +	repo_info_init(&repo_info, repo, format);
> +	repo_info_print(&repo_info);
>  
>  	return 0;
>  }
> diff --git a/t/t1900-repo-info.sh b/t/t1900-repo-info.sh
> new file mode 100755
> index 0000000000..f634e1a285
> --- /dev/null
> +++ b/t/t1900-repo-info.sh
> @@ -0,0 +1,22 @@
> +#!/bin/sh
> +
> +test_description='test git repo-info'
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

We should avoid setting the default initial branch name in new tests. The variable really only exists for legacy tests where we assume that the branch has a specific name.

Show 13 quoted lines

> +. ./test-lib.sh
> +
> +parse_json () {
> +	tr '\n' ' ' | "$PERL_PATH" "$TEST_DIRECTORY/t0019/parse_json.perl"
> +}
> +
> +test_lazy_prereq PERLJSON '
> +	perl -MJSON -e "exit 0"
> +'
> +
> +test_expect_success PERLJSON 'json: returns empty output with allow-empty' '
> +	git repo-info --format=json >output &&
> +	test_line_count = 2 output

This test doesn't really test much. In any case, two lines of output certainly isn't empty output from my perspective. We should verify that the generated output really is the empty JSON object.

Patrick