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

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

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

> Add a new flag `--all` to git-repo-info for requesting all the available
> keys. By using this flag, the user can retrieve all the values instead
> of searching what are the desired keys for what they wants.

I initially read these three lines as "we let you grab all the keys
(without value), so that the caller do it once and then iterate over
them, asking for the values individually".

I think "for requesting all the available keys" can be tweaked to
avoid such a misunderstanding?

    for requesting values for all the available keys

or something, perhaps?

> -git repo info [--format=(keyvalue|nul)] [-z] [<key>...]
> +git repo info [--format=(keyvalue|nul)] [-z] [--all] [<key>...]

Wouldn't it be more like

	..... [--all | <key>...]

or does giving both --all and an indiviual key do something
interesting (like, just make sure these individual keys are valid,
but otherwise do the same as a simple --all)?

> +`info [--format=(keyvalue|nul)] [-z] [--all] [<key>...]`::
>  	Retrieve metadata-related information about the current repository. Only
>  	the requested data will be returned based on their keys (see "INFO KEYS"
>  	section below).
>  +
>  The values are returned in the same order in which their respective keys were
> -requested.
> +requested. The `--all` flag requests all keys.

"requests values for all the keys."

>  	argc = parse_options(argc, argv, prefix, options, repo_usage, 0);
>  
> +	if (all_keys) {
> +		print_all_fields(repo, format);
> +		return 0;
> +	}
> +
>  	return print_fields(argc, argv, repo, format);

OK, so "git repo info --all no-such-key" will silently ignore
no-such-key.  I do not have much problem as long as it is
documented, but there are a few equally plausible alternative
designs.

 * "git repo info --all anything" ignores "anything" no matter what
   they are, as "--all" makes all keys on the command line ignored.

 * The same as above, but it warns about the extra command line
   arguments that are ignored.

 * "git repo info --all object.format" is rejected merely because
   "--all" is defined to be incompatible with giving any individual
   key.

 * "git repo info --all object.format" works as if the command is
   given all the defined keys and then object.format, i.e.
   object.format is reported twice.  If you ask "git repo info
   --all no.such.key", it would fail while asking for no.such.key
   because there is no such key.

I think the first one is what you have implemented.

I see no practical reason why anybody want to pass a concrete key
when asking "--all", but the first one feels the least intuitive one
among these four.  I think the last one is the most logical that
lets users discover why it behaves that way the most easily, even
though it is debatable that succeeding and doing exactly what was
requested in that way is better than rejecting (or perhaps ignoring
with warning) these requests with extra command line arguments.

> diff --git a/t/t1900-repo.sh b/t/t1900-repo.sh
> index 2beba67889..b1391a47b6 100755
> --- a/t/t1900-repo.sh
> +++ b/t/t1900-repo.sh
> @@ -110,4 +110,10 @@ test_expect_success 'git repo info uses the last requested format' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'git repo info --all returns all fields' '
> +	git repo info layout.bare layout.shallow object.format references.format >expect &&
> +	git repo info --all >actual &&
> +	test_cmp expect actual

We would want tests that asks "--all object.format" and "--all no.key",
after deciding what should happen.

Thanks.