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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 14, 2026, 19:34 UTC
Message-ID: <xmqq5x07630q.fsf@gitster.g>
In-Reply-To: <pull.2388.v7.git.git.1789009798902.gitgitgadget@gmail.com>

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

Show 7 quoted lines

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

Show 40 quoted lines

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