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

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8118 bytes --]

Lucas Seiki Oshiro <lucasseikioshiro@gmail.com> writes:

> 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>
> ---
>  builtin/repo-info.c  | 97 +++++++++++++++++++++++++++++++++++++++++---
>  t/t1518-repo-info.sh | 20 +++++++++
>  2 files changed, 112 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/repo-info.c b/builtin/repo-info.c
> index 4d539a17fb..a1c9d3942e 100644
> --- a/builtin/repo-info.c
> +++ b/builtin/repo-info.c
> @@ -9,18 +9,40 @@ enum output_format {
>  	FORMAT_JSON
>  };
>
> +enum repo_info_category {
> +	CATEGORY_REFERENCES = 1
> +};
> +
> +enum repo_info_references_field {
> +	FIELD_REFERENCES_FORMAT = 1
> +};
> +
> +struct repo_info_field {
> +	enum repo_info_category category;
> +	union {
> +		enum repo_info_references_field references;
> +	} field;
> +};
> +
>  struct repo_info {
>  	struct repository *repo;
>  	enum output_format format;
> +	int n_fields;
> +	struct repo_info_field *fields;
> +};
> +
> +const char *default_fields[] = {
> +	"references.format",
>  };
>
>  static void repo_info_init(struct repo_info *repo_info,
>  			   struct repository *repo,
>  			   char *format,
> -			   int allow_empty UNUSED,
> -			   int argc UNUSED,
> -			   const char **argv UNUSED
> +			   int allow_empty,
> +			   int argc,
> +			   const char **argv
>  			   ) {
>

Nit: we wrap to 80 chars generally, so you can put multiple arguments on
the same line.

> +	int i;
>  	repo_info->repo = repo;
>
>  	if (format == NULL || !strcmp(format, "json"))
> @@ -29,18 +51,82 @@ 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) {
> +		argc = ARRAY_SIZE(default_fields);
> +		argv = default_fields;
> +	}
> +
> +	repo_info->n_fields = argc;
> +	repo_info->fields = xmalloc(argc * sizeof(struct repo_info_field));
> +

Nit: perhaps use ALLOC_ARRAY or family here?

> +	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;
> +		}

Makes me wonder if the default fields can be defined as an array of
'repo_info_field' and avoid the strcmp since the information is
pre-defined. Perhaps something like:

  diff --git a/builtin/repo-info.c b/builtin/repo-info.c
  index a1c9d3942e..81c7b5f896 100644
  --- a/builtin/repo-info.c
  +++ b/builtin/repo-info.c
  @@ -31,8 +31,13 @@ struct repo_info {
   	struct repo_info_field *fields;
   };

  -const char *default_fields[] = {
  -	"references.format",
  +const struct repo_info_field default_fields[] = {
  +	{
  +		.category = CATEGORY_REFERENCES,
  +		.field = {
  +			.references = FIELD_REFERENCES_FORMAT
  +		},
  +	},
   };

   static void repo_info_init(struct repo_info *repo_info,
  @@ -53,8 +58,8 @@ static void repo_info_init(struct repo_info *repo_info,
   		die("invalid format %s", format);

   	if (argc == 0 && !allow_empty) {
  -		argc = ARRAY_SIZE(default_fields);
  -		argv = default_fields;
  +		repo_info->fields = (struct repo_info_field *)&default_fields;
  +		return;
   	}

   	repo_info->n_fields = argc;


> +		else {
> +			die("invalid field '%s'", arg);
> +		}
> +	}
> +}
> +
> +static void repo_info_release(struct repo_info *repo_info) {
> +	free(repo_info->fields);
>  }
>
> -static void repo_info_print_plaintext(struct repo_info *repo_info UNUSED) {
> +static void repo_info_print_plaintext(struct repo_info *repo_info) {

This should definitely go into its own commit or at least be mentioned in
the commit message.

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

Nit: always nice to leave a newline between the variable declarations
and following code. Also we can move move `int i` declaration directly
inside 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) {
> +			case FIELD_REFERENCES_FORMAT:
> +				puts(ref_storage_format_to_name(
> +					repo->ref_storage_format));
> +				break;
> +			}
> +			break;
> +		}
> +	}
>  }
>
> -static void repo_info_print_json(struct repo_info *repo_info UNUSED)
> +static void repo_info_print_json(struct repo_info *repo_info)
>  {
>  	struct json_writer jw;
> +	int i;
> +	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);
>

Doesn't this mean that each value of CATEGORY_REFERENCES and
FIELD_REFERENCES_FORMAT should have no common bits? Especially in the
former across different categories too.

I wonder if we can solve this in a easier way. We care about:
- Retaining the order of fields input by the user
- Having a default set of fields if no input received

But, for JSON formatting, order of fields doesn't matter as per the spec
[1]:

  An object is an unordered set of name/value pairs. An object begins
  with {left brace and ends with }right brace. Each name is followed by
  :colon and the name/value pairs are separated by ,comma.

So, do we really want to keep the order? Especially since one suggestion
is to only work with the JSON format for now.

If we don't care about the order, we can simply have a structure with
bit fields and work with that.

>  	puts(jw.json.buf);
> @@ -92,6 +178,7 @@ int cmd_repo_info(
>  			     PARSE_OPT_KEEP_UNKNOWN_OPT);
>  	repo_info_init(&repo_info, repo, format, allow_empty, argc, argv);
>  	repo_info_print(&repo_info);
> +	repo_info_release(&repo_info);
>
>  	return 0;
>  }
> diff --git a/t/t1518-repo-info.sh b/t/t1518-repo-info.sh
> index 2e1a6f0c34..a99198b0f6 100755
> --- a/t/t1518-repo-info.sh
> +++ b/t/t1518-repo-info.sh
> @@ -6,6 +6,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
>  . ./test-lib.sh
>
> +DEFAULT_NUMBER_OF_FIELDS=1
> +
>  parse_json () {
>  	tr '\n' ' ' | "$PERL_PATH" "$TEST_DIRECTORY/t0019/parse_json.perl"
>  }
> @@ -46,4 +48,22 @@ test_expect_success 'plaintext: returns empty output with allow-empty' '
>  	test_line_count = 0 output
>  '
>
> +test_repo_info 'ref format files is retrieved correctly' \
> +	'' \
> +	'references.format' 'files'
> +

This expects that the repository is created with the 'files' backend by
default, but that is defined by the 'GIT_TEST_DEFAULT_REF_FORMAT' env
variable. So wouldn't this fail when in the CI job for reftables?

> +test_repo_info 'ref format reftable is retrieved correctly' \
> +	'--ref-format=reftable' \
> +	'references.format' 'reftable'
> +
> +test_expect_success 'plaintext: output all default fields' "
> +	git repo-info --format=plaintext >actual &&
> +	test_line_count = $DEFAULT_NUMBER_OF_FIELDS actual
> +"
> +
> +test_expect_success 'json: output all default fields' "
> +	git repo-info --format=json | parse_json | grep '.*\..*\..*' >actual &&
> +	test_line_count = $DEFAULT_NUMBER_OF_FIELDS actual
> +"
> +

While line count is good, we should also check the default values, no?

>  test_done
> --
> 2.39.5 (Apple Git-154)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]