Re: [GSoC PATCH v5 2/5] repo: add the field references.format

From: Patrick Steinhardt <ps@pks.im>
Date: Jul 29, 2025, 09:35 UTC
Message-ID: <aIiV7cz2Tx50JZUv@pks.im>
In-Reply-To: <20250727175110.84770-3-lucasseikioshiro@gmail.com>

On Sun, Jul 27, 2025 at 02:51:07PM -0300, Lucas Seiki Oshiro wrote:

Show 13 quoted lines

> diff --git a/builtin/repo.c b/builtin/repo.c
> index d4f01e35e2..02d5821c77 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -1,12 +1,90 @@
>  #include "builtin.h"
>  #include "parse-options.h"
> +#include "refs.h"
> +#include "strbuf.h"
>  
> -static int repo_info(int argc UNUSED, const char **argv UNUSED,
> -		     const char *prefix UNUSED, struct repository *repo UNUSED)
> +typedef int get_value_fn(struct repository *repo, struct strbuf *buf);

Nice, this now uses a strbuf as recommended.

Show 11 quoted lines

> +struct field {
> +	const char *key;
> +	get_value_fn *get_value;
> +};
> +
> +static int get_references_format(struct repository *repo, struct strbuf *buf)
> +{
> +	strbuf_addstr(buf,
> +		      ref_storage_format_to_name(repo->ref_storage_format));
> +	return 0;
> +}

And this prints into the buffer diretcly. Makes sense.

[snip]

Show 10 quoted lines

> +static int print_fields(int argc, const char **argv, struct repository *repo)
> +{
> +	const char *last = "";
> +
> +	QSORT(argv, argc, qsort_strcmp);
> +
> +	for (int i = 0; i < argc; i++) {
> +		get_value_fn *get_value;
> +		const char *key = argv[i];
> +		struct strbuf value;

Let's declare the strbuf outside of the loop and `strbuf_reset()` it on every iteration.

Show 5 quoted lines

> +
> +		if (!strcmp(key, last))
> +			continue;
> +
> +		strbuf_init(&value, 64);

I don't think we should explicitly initialize it with a specific capacity. Let's just use `STRBUF_INIT`.

Show 11 quoted lines

> +		get_value = get_value_fn_for_key(key);
> +
> +		if (!get_value) {
> +			strbuf_release(&value);
> +			return error(_("key '%s' not found"), key);
> +		}
> +
> +		get_value(repo, &value);
> +		printf("%s=%s\n", key, value.buf);
> +		last = key;
> +		strbuf_release(&value);

And the call to `strbuf_release()` should be moved to the end of this function so that we know to reuse the buffer. The above early return would then be converted into a `goto out` so that we have a common exit path where we know to clean up all resources.

Patrick