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

[Junio C Hamano <gitster@pobox.com>]

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  | 108 +++++++++++++++++++++++++++++++++++++++++--
>  t/t1900-repo-info.sh |  58 +++++++++++++++++++++++
>  2 files changed, 162 insertions(+), 4 deletions(-)
>
> 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
> +};
> +
> +struct repo_info_field {
> +	enum repo_info_category category;
> +	union {
> +		enum repo_info_references_field references;
> +	} field;
> +};

Are we going to gain more than one union in this struct?  If not,
name the union simply 'u', so that the accesses look like
"info.u.references" instead of "info.field.references".  In that
construct, a long name ".field" does not add much information value
anyway---the member in the union, like "references", is where all
the information value is.

>  struct repo_info {
>  	struct repository *repo;
>  	enum output_format format;
> +	int n_fields;
> +	struct repo_info_field *fields;
>  };
>  
> +static struct repo_info_field default_fields[] = {
> +	{
> +		.category = CATEGORY_REFERENCES,
> +		.field.references = FIELD_REFERENCES_FORMAT
> +	}
> +};
> +
> +static void print_key_value(const char *key, const char *value) {
> +	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;
>  	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;

Are we going to have duplicate information between this
if/elseif/... cascade and the default_fields[] array?

There needs a catalog of "everything we support" somewhere, which
can be used to determine what "--all" should show, whose entry can
be looked up with the textual name (like "references.format") for
the purpose of command line parsing.  Once you have such a
"everything we support" table, this parser loop and "--all" handling
can become more table-driven.

> -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);

Do not write if() and its body on the same line.

>  }
>  
> -static void repo_info_print_json(struct repo_info *repo_info UNUSED)
> +static void repo_info_print_plaintext(struct repo_info *repo_info) {
> +	struct repository *repo = repo_info->repo;
> +	int i;
> +	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:
> +				print_key_value("references.format",
> +						ref_storage_format_to_name(
> +							repo->ref_storage_format));
> +				break;
> +			}
> +			break;
> +		}

Can't this also be made more table driven, by having a callback
function pointer in the "everything we support" table?  That way,
this deeply nested loop does not have to be here and you do not have
to chomp a line that is overly deeply nested in the middle just to
go under 80-column limit.

IOW, the above code may become something like:

	for (int i = 0; i < repo_info->n_fields; i++) {
		struct repo_info_field *f = &repo_info->fields[i];
                f->show(repo_info, f);
	}

and the repo_info_field structure may have a pointer to the
implementation of "show" method for that particular type, which may
look like

        static void show_references_format(struct repo_info *i,
                                           struct repo_info_field *f)
        {
                struct repository *repo = i->repo;
                enum ref_storage_format fmt = repo->ref_storage_format;
                const char *name = ref_storage_format_to_name(fmt); 

                print_key_value("references.format", name);
        }

Hmm?

I'll stop here for now.

Thanks.