Re: [PATCH] repo: add --all to git-repo-info

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 16, 2025, 08:06 UTC
Message-ID: <aMkaePi90Q6sXuO4@pks.im>
In-Reply-To: <20250915223618.13093-1-lucasseikioshiro@gmail.com>

On Mon, Sep 15, 2025 at 07:36:17PM -0300, Lucas Seiki Oshiro wrote:

> Add a new flag `--all` to git-repo-info for requesting all the available
> keys. By using this flag, the user can retrieve all the values instead
> of searching what are the desired keys for what they wants.

One thing I wonder is whether we actually need the "--all" flag in the first place. Right now, when saying `git repo info` without any further arguments, then the user will be met with complete silence. I don't really think that this is useful as a default in any way, as it makes it very difficult for the user to figure out what kind of information exists in the first place.

So how about we don't introduce a separate flag, but instead detect the case where the user passed no arguments at all and then print all values by default? It changes the current behaviour, but on the other hand I would argue that the current behaviour is not useful in the first place. And the command is labelled as experimental anyway, so for now we still can change it.

Show 29 quoted lines

> diff --git a/builtin/repo.c b/builtin/repo.c
> index bbb0966f2d..906d8a3e12 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -77,6 +77,24 @@ static get_value_fn *get_value_fn_for_key(const char *key)
>  	return found ? found->get_value : NULL;
>  }
>  
> +static void print_field(enum output_format format, const char *key,
> +			struct strbuf *valbuf, struct strbuf *quotbuf)
> +{
> +	strbuf_reset(quotbuf);
> +
> +	switch (format) {
> +	case FORMAT_KEYVALUE:
> +		quote_c_style(valbuf->buf, quotbuf, NULL, 0);
> +		printf("%s=%s\n", key, quotbuf->buf);
> +		break;
> +	case FORMAT_NUL_TERMINATED:
> +		printf("%s\n%s%c", key, valbuf->buf, '\0');
> +		break;
> +	default:
> +		BUG("not a valid output format: %d", format);
> +	}
> +}
> +
>  static int print_fields(int argc, const char **argv,
>  			struct repository *repo,
>  			enum output_format format)

Changes like this which are preparatory refactorings can also be split out into separate commits. That makes it easier to see and review such a change standalone.

Show 14 quoted lines

> @@ -119,6 +124,26 @@ static int print_fields(int argc, const char **argv,
>  	return ret;
>  }
>  
> +static void print_all_fields(struct repository *repo,
> +			     enum output_format format)
> +{
> +	struct strbuf valbuf = STRBUF_INIT;
> +	struct strbuf quotbuf = STRBUF_INIT;
> +
> +	for (unsigned long i = 0; i < ARRAY_SIZE(repo_info_fields); i++) {
> +		struct field field = repo_info_fields[i];
> +		get_value_fn *get_value = field.get_value;
> +		const char *key = field.key;

Nit: I don't really feel like the `get_value` or `key` variables help
much. I'd just drop those.

Show 22 quoted lines

> @@ -140,6 +165,7 @@ static int repo_info(int argc, const char **argv, const char *prefix,
>  		     struct repository *repo)
>  {
>  	enum output_format format = FORMAT_KEYVALUE;
> +	int all_keys = 0;
>  	struct option options[] = {
>  		OPT_CALLBACK_F(0, "format", &format, N_("format"),
>  			       N_("output format"),
> @@ -148,11 +174,17 @@ static int repo_info(int argc, const char **argv, const char *prefix,
>  			       N_("synonym for --format=nul"),
>  			       PARSE_OPT_NONEG | PARSE_OPT_NOARG,
>  			       parse_format_cb),
> +		OPT_BOOL(0, "all", &all_keys, N_("return all keys")),
>  		OPT_END()
>  	};
>  
>  	argc = parse_options(argc, argv, prefix, options, repo_usage, 0);
>  
> +	if (all_keys) {
> +		print_all_fields(repo, format);
> +		return 0;
> +	}

Instead of checking for `all_keys` we could check for `if (!argc)` if you want to follow my suggestion.

Show 16 quoted lines

>  	return print_fields(argc, argv, repo, format);
>  }
>  
> diff --git a/t/t1900-repo.sh b/t/t1900-repo.sh
> index 2beba67889..b1391a47b6 100755
> --- a/t/t1900-repo.sh
> +++ b/t/t1900-repo.sh
> @@ -110,4 +110,10 @@ test_expect_success 'git repo info uses the last requested format' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'git repo info --all returns all fields' '
> +	git repo info layout.bare layout.shallow object.format references.format >expect &&
> +	git repo info --all >actual &&
> +	test_cmp expect actual
> +'

This test will obviously need to be adapted every time we add a new field, which I think is fine. But we should adapt it so that it's easily extensible without by just adding another line. E.g. like this:

    test_expect_success 'git repo info --all returns all fields' '
            git repo info \
                layout.bare \
                layout.shallow \
                object.format \
                references.format \
                >expect &&
            git repo info --all >actual &&
            test_cmp expect actual
    '

This ensures that it's as simple as adding a new line when we add a specific field without having to rewrite the whole line.

Patrick