Re: [GSoC RFC PATCH v2 5/7] repo-info: add the field references.format

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jun 19, 2025 at 07:57:49PM -0300, Lucas Seiki Oshiro wrote:
> diff --git a/builtin/repo-info.c b/builtin/repo-info.c
> index 6499be0eae..6ce3e6134f 100644
> --- a/builtin/repo-info.c
> +++ b/builtin/repo-info.c
> @@ -1,21 +1,56 @@
>  #include "builtin.h"
>  #include "json-writer.h"
>  #include "parse-options.h"
> +#include "quote.h"
> +#include "refs.h"
>  
>  enum output_format {
>  	FORMAT_JSON,
>  	FORMAT_PLAINTEXT
>  };
>  
> +enum repo_info_category {
> +	CATEGORY_REFERENCES = 1 << 0
> +};
> +
> +enum repo_info_references_field {
> +	FIELD_REFERENCES_FORMAT = 1 << 0
> +};

Missing commas after both enum values.

> +struct repo_info_field {
> +	enum repo_info_category category;
> +	union {
> +		enum repo_info_references_field references;
> +	} field;
> +};

Hm, okay. I guess later patches will add separate enums for each
category, so this saves us a couple bytes as the number of categories
grows.

>  struct repo_info {
>  	struct repository *repo;
>  	enum output_format format;
> +	int n_fields;

This should be `size_t`. Also, it's typical in our code base to call
this `fields_nr`.

> +	struct repo_info_field *fields;
>  };
>  
> +static struct repo_info_field default_fields[] = {
> +	{
> +		.category = CATEGORY_REFERENCES,
> +		.field.references = FIELD_REFERENCES_FORMAT
> +	}

Missing comma.

> +};
> +
> +static void print_key_value(const char *key, const char *value) {

Formatting: the curly brace should sit on its own line.

> +	printf("%s=", key);
> +	quote_c_style(value, NULL, stdout, 0);
> +	putchar('\n');
> +}
> +
>  static void repo_info_init(struct repo_info *repo_info,
>  			   struct repository *repo,
> -			   char *format)
> +			   char *format,
> +			   int allow_empty,
> +			   int argc, const char **argv)
>  {
> +	int i;

This variable can be declared in the loop itself:

    for (int i = 0; ...)

>  	repo_info->repo = repo;
>  
>  	if (format == NULL || !strcmp(format, "json"))
> @@ -24,19 +59,83 @@ static void repo_info_init(struct repo_info *repo_info,
>  		repo_info->format = FORMAT_PLAINTEXT;
>  	else
>  		die("invalid format %s", format);
> +
> +	if (argc == 0 && !allow_empty) {
> +		repo_info->n_fields = ARRAY_SIZE(default_fields);
> +		repo_info->fields = default_fields;
> +	} else {
> +		repo_info->n_fields = argc;
> +		ALLOC_ARRAY(repo_info->fields, argc);
> +
> +		for (i = 0; i < argc; i++) {
> +			const char *arg = argv[i];
> +			struct repo_info_field *field = repo_info->fields + i;
> +
> +			if (!strcmp(arg, "references.format")) {
> +				field->category = CATEGORY_REFERENCES;
> +				field->field.references = FIELD_REFERENCES_FORMAT;
> +			} else {
> +				die("invalid field '%s'", arg);
> +			}
> +		}
> +	}
>  }
>  
> -static void repo_info_print_plaintext(struct repo_info *repo_info UNUSED)
> +static void repo_info_release(struct repo_info *repo_info)
>  {
> +	if (repo_info->fields != default_fields) free(repo_info->fields);
>  }
>  
> -static void repo_info_print_json(struct repo_info *repo_info UNUSED)
> +static void repo_info_print_plaintext(struct repo_info *repo_info) {

Formatting.

> +	struct repository *repo = repo_info->repo;
> +	int i;

Variable can be declared in the loop.

> +	for (i = 0; i < repo_info->n_fields; i++) {
> +		struct repo_info_field *field = &repo_info->fields[i];
> +		switch (field->category) {
> +		case CATEGORY_REFERENCES:
> +			switch (field->field.references) {

I wonder a bit what it buys us that we have the difference between the
category and reference format. Right now it feels like it can cause more
errors that it prevents, as we now always have doubly-nested switches.

Wouldn't it make more sense to only only pass around the fields as
`repo_info_references_field`? We could then have two arrays that we
define globally:

    static const char const* name_by_field[] = {
        [FIELD_REFERENCES_FORMAT] = "references.format",
    };

    static repo_info_category category_by_field[] = {
        [FIELD_REFERENCES_FORMAT] = CATEGORY_REFERENCES,
    };

So `name_by_field[FIELD_REFERENCES_FORMAT]` would yield the name and
`category_by_field[CATEGORY_REFERENCES]` would yield its category. But
the benefit is that you only need to pass around the field enum from now
on, all other information is implicit.

The reverse information can also be obtained easily. To e.g. get all
fields of a reference you'd iterate through `category_by_field` and take
all array indices whose value matches the desired category.

> +			case FIELD_REFERENCES_FORMAT:
> +				print_key_value("references.format",
> +						ref_storage_format_to_name(
> +							repo->ref_storage_format));
> +				break;
> +			}
> +			break;
> +		}
> +	}
> +}
> +
> +static void repo_info_print_json(struct repo_info *repo_info)
>  {
>  	struct json_writer jw;
> +	int i;

Variable can be declared in the loop.

> +	unsigned int categories = 0;
> +	unsigned int references_fields = 0;
> +	struct repository *repo = repo_info->repo;
> +
> +	for (i = 0; i < repo_info->n_fields; i++) {
> +		struct repo_info_field *field = repo_info->fields + i;
> +		categories |= field->category;
> +		switch (field->category) {
> +		case CATEGORY_REFERENCES:
> +			references_fields |= field->field.references;
> +			break;
> +		}
> +	}
>  
>  	jw_init(&jw);
>  
>  	jw_object_begin(&jw, 1);
> +
> +	if (categories & CATEGORY_REFERENCES) {
> +		jw_object_inline_begin_object(&jw, "references");
> +		if (references_fields & FIELD_REFERENCES_FORMAT) {
> +			const char *format_name = ref_storage_format_to_name(
> +				repo->ref_storage_format);
> +			jw_object_string(&jw, "format", format_name);
> +		}
> +		jw_end(&jw);
> +	}
>  	jw_end(&jw);
>  
>  	puts(jw.json.buf);

Patrick