Re: [PATCH v7] var: support broken-down idents, signing key, multiple args, and -z

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

"Andrew Pleeter via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  DESCRIPTION
>  -----------
> +Prints Git logical variables. Exits with code 1 if any requested
> +variable has no value. When multiple variables are requested, an empty
> +record (a blank line, or an empty NUL-terminated record when `-z` is given)
> +is printed for any variable that has no value, and the command continues
> +processing the remaining variables.

Very clearly described.  Although it makes it sound as if the
command always notices a variable without any value and reports
failure with its exit value, no matter in what mode, but I do not
think that matches what the code does (below).

>  int cmd_var(int argc,
> ...
> +	term = nul_term ? '\0' : '\n';
> +
> +	for (i = 0; i < argc; i++) {
> +		const struct git_var *git_var = get_git_var(argv[i]);
>  
> +		if (!git_var)
> +			usage_with_options(var_usage, options);
>  
> +		if (git_var->read) {
> +			char *val = git_var->read(IDENT_STRICT);
> +
> +			if (!val) {
> +				if (argc == 1)
> +					return 1;
> +				putc(term, stdout);
> +				continue;
> +			}
> +			printf("%s%c", val, term);
> +			free(val);
> +		} else {
> +			struct string_list list = STRING_LIST_INIT_DUP;
> +			size_t j;
> +
> +			git_var->multiread(&list);
> +			if (argc == 1 && !list.nr) {
> +				string_list_clear(&list, 0);
> +				return 1;
> +			}
> +			for (j = 0; j < list.nr; j++)
> +				printf("%s%c", list.items[j].string, term);
> +			if (argc > 1)
> +				putc(term, stdout);
> +			string_list_clear(&list, 0);
> +		}
> +	}
>  
>  	return 0;
>  }

When we ask for a single variable, 'argc' is 1 (and we never update
'argc' in the loop, which is good), and we return 1 upon seeing a
missing value.  We also do the same when we receive a 0-element list
back for a multi-valued variable.  Otherwise, nobody in the loop
remembers that we had any such failure; the loop continues, and we
return 0 unconditionally.  A "missing value" anomaly noticed during
the loop gets forgotten.

Either the documentation or the code needs to be updated, I
think.

I am still not convinced this output format is easy for scripts to
handle when multi-valued variables are involved.  It is also a bit
unclear what exactly "variable has no value" means.  A variable
whose value is an empty string is not such a variable, right?  If a
multi-valued variable has an empty string and the string "hello" as
its value, would the output from the command confuse the reading
script into thinking that the first blank line signals that the
variable has no value, for example?  Having to know which variables
are multi-valued and which are not before parsing the output format
does not help, either.

We could, of course, disambiguate by prefixing these lines with
variable names followed by '=' (or NUL), which would likely
eliminate the ambiguity.  But I understand that you are trying to
allow the parsers to proceed without having to strip prefixes from
each input, which is why the format tries to rely solely on the
correspondence between command-line arguments and output lines.  I,
however, doubt you succeeded in doing so without making the output
ambiguous.

Thanks.