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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 8, 2026, 21:53 UTC
Message-ID: <xmqqpkyntnov.fsf@gitster.g>
In-Reply-To: <pull.2388.v5.git.git.1788900182711.gitgitgadget@gmail.com>

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

Show 13 quoted lines

>  VARIABLES
>  ---------
>  `GIT_AUTHOR_IDENT`::
> +    The author.
> +
> +`GIT_AUTHOR_NAME`::
> +    The name of the author.
> +
> +`GIT_AUTHOR_EMAIL`::
> +    The email of the author.
> +
> +`GIT_AUTHOR_DATE`::
> +    The date and timezone of the author.

The above (and the COMMITTER counterparts) gives almost no useful information. On the other hand, the description used here ...

> +`GIT_SIGNING_KEY`::
> +    The key that would be used to sign the resulting commit if you were
> +    to run `git commit` right now.

... explains what significance this key has much better.

Show 13 quoted lines

> @@ -85,9 +127,12 @@ endif::git-default-pager[]
>      The path to the global (per-user) configuration files, if any.
>  
>  Most path values contain only one value. However, some can contain multiple
> -values, which are separated by newlines, and are listed in order from highest to
> -lowest priority.  Callers should be prepared for any such path value to contain
> -multiple items.
> +values, which are separated by newlines (or NUL bytes if `-z` is given),
> +and are listed in order from highest to lowest priority. When querying
> +multiple variables, an extra newline (or an extra NUL byte if `-z` is
> +given) is printed after the values of a multi-valued variable to mark the
> +end of its list. Callers should be prepared for any such path value to
> +contain multiple items.

Hmph. The added bulk of this description is only because we can now optionally use NUL delimiting as opposed to LF? Are we changing the output in a backward incompatible way for callers that do not pass the -z option?

For example, "git var -l" output ends like so for me in today's Git:

    $ git var -l | tail -n 3
    GIT_CONFIG_SYSTEM=/home/gitster/g/seen/etc/gitconfig
    GIT_CONFIG_GLOBAL=/home/gitster/.config/git/config
    GIT_CONFIG_GLOBAL=/home/gitster/.gitconfig

    $ git var GIT_CONFIG_GLOBAL
    /home/gitster/.config/git/config
    /home/gitster/.gitconfig

You mention "an extra newline". Will it appear in the above output with this version? If so, wouldn't that be a breaking change?

Show 36 quoted lines

> +static char *ident_part(const char *ident, enum ident_part part)
> +{
> +	struct ident_split split;
> +
> +	if (!ident)
> +		return NULL;
> +	if (split_ident_line(&split, ident, strlen(ident)))
> +		return NULL;
> +
> +	switch (part) {
> +	case IDENT_NAME:
> +		if (!split.name_begin || !split.name_end)
> +			return NULL;
> +		return xmemdupz(split.name_begin,
> +				split.name_end - split.name_begin);
> +	case IDENT_MAIL:
> +		if (!split.mail_begin || !split.mail_end)
> +			return NULL;
> +		return xmemdupz(split.mail_begin,
> +				split.mail_end - split.mail_begin);
> +	case IDENT_DATE:
> +		if (!split.date_begin)
> +			return NULL;
> +		if (split.tz_end)
> +			return xmemdupz(split.date_begin,
> +					split.tz_end -
> +					split.date_begin);
> +		if (split.date_end)
> +			return xmemdupz(split.date_begin,
> +					split.date_end -
> +					split.date_begin);
> +		return NULL;
> +	default:
> +		return NULL;
> +	}
> +}

In many cases in the above you punt and return NULL, but aren't there some cases where it clearly is a data error that deserves die() or a programming error that deserves BUG()?

For example, my quick read of split_ident_line() tells me that it is impossible for split.name_begin or split.name_end to be NULL if the function signals success by returning 0. If I am not misreading the code, returning NULL when IDENT_NAME is requested in the above code is sweeping a programming error under the rug.

If the caller passed anything other than IDENT_{NAME,MAIL,DATE}, it similarly is a programming error. The default: arm should not hide it underr the rug by returning NULL, but complain loudly with a BUG(), no?

Show 14 quoted lines

> @@ -99,19 +190,21 @@ static char *git_config_val_global(int ident_flag UNUSED)
>  	git_global_config_paths(&user, &xdg);
>  	if (xdg && *xdg) {
>  		normalize_path_copy(xdg, xdg);
> -		strbuf_addf(&buf, "%s\n", xdg);
> +		strbuf_addstr(&buf, xdg);
> +		strbuf_addch(&buf, '\0');
>  	}
>  	if (user && *user) {
>  		normalize_path_copy(user, user);
> -		strbuf_addf(&buf, "%s\n", user);
> +		strbuf_addstr(&buf, user);
> +		strbuf_addch(&buf, '\0');
>  	}

Mental note: we used to use LF at the end, but in this version we add NUL here.

Show 9 quoted lines

>  	free(xdg);
>  	free(user);
> -	strbuf_trim_trailing_newline(&buf);
> -	if (buf.len == 0) {
> +	if (!buf.len) {
>  		strbuf_release(&buf);
>  		return NULL;
>  	}
> +	strbuf_addch(&buf, '\0');

And then we add an extra NUL after that.

>  	return strbuf_detach(&buf, &unused);
>  }

Show 19 quoted lines

> @@ -172,34 +293,35 @@ static struct git_var git_vars[] = {
>  	},
>  };
>  
> -static void list_vars(void)
> +static void list_vars(int nul_term)
>  {
>  	struct git_var *ptr;
> -	char *val;
> -
> -	for (ptr = git_vars; ptr->read; ptr++)
> -		if ((val = ptr->read(0))) {
> -			if (ptr->multivalued && *val) {
> -				struct string_list list = STRING_LIST_INIT_DUP;
> -
> -				string_list_split(&list, val, "\n", -1);
> -				for (size_t i = 0; i < list.nr; i++)
> -					printf("%s=%s\n", ptr->name, list.items[i].string);
> -				string_list_clear(&list, 0);

We used to split at LF (because we used to concatenate with LF in the git_config_val_globa() that grabs potentially multiple values) and then showed them one by one.

Show 16 quoted lines

> -			} else {
> -				printf("%s=%s\n", ptr->name, val);
> -			}
> -			free(val);
> +	char delim = nul_term ? '\n' : '=';
> +	char term = nul_term ? '\0' : '\n';
> +
> +	for (ptr = git_vars; ptr->read; ptr++) {
> +		char *val = ptr->read(0);
> +
> +		if (!val)
> +			continue;
> +
> +		if (ptr->multivalued) {
> +			for (const char *s = val; *s; s += strlen(s) + 1)
> +				printf("%s%c%s%c", ptr->name, delim, s, term);

Now we use each string pieces (s), skip the string we just showed by advancing the pointer by strlen(s) + 1. If multi-valued variable has ever an empty string as one of the possible values, this scheme would break down, but right now GIT_CONFIG_GLOBAL is the only thing that is .multivalued, and neither the HOME or XDG path is likely to be ever empty, so this may be OK, perhaps? If xdg is defined to be a non-empty string (i.e., "if (xdg && *xdg)" is taken) but if calling normalize_path_copy(xdg, xdg) makes it an empty string, then git_config_val_global() will give "\0/home/gitster/.gitconfig\0\0" for me (the first NUL is after the empty xdg value, the second NUL is terminating HOME value, and the third NUL concludes the whole thing), and then this loop will exit without showing anything (not just skipping an empty XDG, but hiding perfectly healthy HOME value). Is that a concern?

I wonder if we should correct how .multivalued field is handled before we add more of them. For example, .multivalued = 1 item may use something different from .read that uses a string-list to carry the information

	{
		.name = "GIT_CONFIG_GLOBAL",
		.multiread = git_config_val_global,
	},

	static int git_config_val_global(struct string_list *list)
	{
		git_global_config_paths(...);
		if (xdg available)
			string_list_append(list, xdg);
		if (user availble)
			string_list_append(list, user);
		return 0;
	}
	
and then the above part of the code would look more like

	for (ptr = git_vars; ptr->read || ptr->multiread; ptr++) {

		if (ptr->read) {
			... single read as before ...
		} else (ptr->multiread) {
			struct string_list list = STRING_LIST_INIT;

			ptr->multiread(&list);
			for (size_t i = 0; i < list.nr; i++)
				... use list.items[i].string ...
		}

Show 12 quoted lines

>  static const struct git_var *get_git_var(const char *var)
>  {
>  	struct git_var *ptr;
> +
>  	for (ptr = git_vars; ptr->read; ptr++) {
> -		if (strcmp(var, ptr->name) == 0) {
> +		if (!strcmp(var, ptr->name))
>  			return ptr;
> -		}
>  	}
>  	return NULL;
>  }

An unrelated change like this is distracting and makes it less likely for your patch to succeed. Leave such a clean-up out of a patch that is about a new feature, or fixing a bug.