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

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

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

> +static void print_all_fields(struct repository *repo,
> +			     enum output_format format)
> +{
> +	struct strbuf valbuf = STRBUF_INIT;
> +
> +	for (unsigned long i = 0; i < ARRAY_SIZE(repo_info_fields); i++) {
> +		const struct field *field = &repo_info_fields[i];
> +
> +		strbuf_reset(&valbuf);
> +		field->get_value(repo, &valbuf);
> +		print_field(format, field->key, valbuf.buf);
> +	}
> +	strbuf_release(&valbuf);
> +}

I am not sure if "unsigned long i" is the type you want here.  I do
not mind, and actually I prefer, a simple platform natural "int i"
for something simple like this [*], but I know other people prefer to
use "size_t" to work with ARRAY_SIZE() these days.

    Side note: The reason they insist using size_t here is that
        "-Wsign-compare" makes the compiler complain.  But I would
        say that it only shows what a misguided feature
        -Wsign-compare warning is, especially given that the
        compiler perfectly well knows how big repo_info_fields[]
        array is and the iteration cannot do any harm if done with a
        signed integer smaller than size_t

Anyway.

We grab each field, ask it for its value, and then print it.  What a
straight-forward flow that is pleasant to read! ;-).

Compared to that, printing of individual keys given by the end-user
is much uglier, but it is not a fault of this two-patch series, so
my comment here is only as #leftoverbits for later clean-up.

The print_fields() function does this (modulo error checking for
missing key):

	for (int i = 0; i < argc; i++) {
		get_value_fn *get_value;
		const char *key = argv[i];

		get_value = get_value_fn_for_key(key);
		get_value(repo, &valbuf);

We should get rid of the get_value_fn_for_key() helper, and instead
add and use repo_info_field(const char *key) helepr.  That way, the
logic become exactly the same as the "get all" case.  The body of
the loop would read (modulo error checking for missing key):

		const struct field *field = repo_info_field(argv[i]);
	        field->get_value(repo, &valbuf);

whcih is much nicer, when the repo_info_fields[] gains more
attributes other than a callback function, we do not want to keep
adding get_this_attr_for_key() functions.

    Side note: By the way, it should be named repo_info_field[].
        Name arrays singular so that you can name its 0th element by
        saying dog[0], not dogs[0].  "dog[1] and dog[2] are friends"
        not "dogs[1] and dogs[2] are friends".  An exception is when
        most of the time you use the array as a single unit as a
        collection, passing it around in the call chain, and you
        rarely address each individual element (other than outside
        the implementation of the API).  I am OK to see such an
        array that is mostly used as a collection named plural (but
        of course, singular names are always fine).  Adding this to
        CodingGuidelines is perhaps a #leftoverbits material.

> @@ -167,6 +185,14 @@ static int cmd_repo_info(int argc, const char **argv, const char *prefix,
>  	if (format != FORMAT_KEYVALUE && format != FORMAT_NUL_TERMINATED)
>  		die(_("unsupported output format"));
>  
> +	if (all_keys) {
> +		if (argc)
> +			die(_("--all and <key> cannot be used together"));
> +
> +		print_all_fields(repo, format);
> +		return 0;
> +	}
>  	return print_fields(argc, argv, repo, format);

This would work, but the symmetry between a list of keys vs the
"--all" option is lost.

I'd rather see something like the following after a #leftoverbits
clean-up commit:

	if (all_keys && argc)
		die(_("--all and <key> cannot be used together"));

	if (all_keys)
		return print_all_fields(repo, format);
	else
		return print_fields(argc, argv, repo, format);

> diff --git a/t/t1900-repo.sh b/t/t1900-repo.sh
> index 2beba67889..51d55f11a5 100755
> --- a/t/t1900-repo.sh
> +++ b/t/t1900-repo.sh
> @@ -4,6 +4,15 @@ test_description='test git repo-info'
>  
>  . ./test-lib.sh
>  
> +# git-repo-info keys. It must contain the same keys listed in the const
> +# repo_info_fields, in lexicographical order.
> +REPO_INFO_KEYS='
> +	layout.bare
> +	layout.shallow
> +	object.format
> +	references.format
> +'

Again, this would work for now, but maybe "git repo info --keys"
that emits these would be easier to manage.  This can be left to
#leftoverbits of course.

But then we have seem to have seen too many #leftoverbits material,
you might want to handle some or all of them in this series in a
reroll?  I am starting to become undecided.

With "repo info --keys", the user could even do

    git repo info $(git repo info --keys)

if they wanted to.

No, I am not suggesting to discard the "--all" option; only pointing
out that conceptually, "--all" can be explained in terms of
"--keys".