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

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

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.

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

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".