On 2026-08-25 at 20:46:42, Andrew Pleeter via GitGitGadget wrote:
> From: anpl1623 <andrewpleeter@gmail.com>

You will probably want this to match your real name since you're using
it in the email.  We prefer that people use their real names, but don't
require it, but I assume that it's not a problem since you just CC'd
yourself using it (and it's also in your email address).

> Add a builtin 'whoami' command to inspect and display the resolved
> author and committer identity along with the commit signing
> configuration (GPG/SSH key ID and commit.gpgsign status) used when
> creating Git commits.
> 
> Support optional flags (--author, --committer, --name, --email,
> --signing-key, and --verbose) for targeted querying and scripting.

I suspect users will want a way to get _all_ of the output in a
machine-readable way, so you'd probably want to provide some method of
doing that.  Note that because your existing endpoints provide
translated strings, they are not suitable for this.  That doesn't mean
that they should not be translated (because they should) but we'd
probably want a format like the following:

user.author.name=A U Thor
user.author.email=author@example.com

Other formats are possible, though.

Possibly a `-z` option for NUL-terminated instead of LF-terminated
output might be warranted as well unless we're certain that our output
will never contain a newline (hint: config options can).

> Include documentation in Documentation/git-whoami.adoc and regression
> tests in t/t0015-whoami.sh.
> 
> Signed-off-by: anpl1623 <andrewpleeter@gmail.com>

Again, you'll want to sign this off with your real name.

>     MOTIVATION
>     
>     Users often work across multiple environments, profiles, or repositories
>     with different global/local configs and signing keys. Currently,
>     verifying what identity and signing key will be attached to a new commit
>     requires checking several individual git config and git var settings.
>     git whoami provides a simple, direct porcelain command to verify this in
>     one step.

I think this should go in the commit message.  I thought to myself,
"Well, there are already ways to get this information, so why add a new
one?"  Telling us why your patch is compelling and solves an important
purpose is appropriate for the commit message.

I might also like to see an explanation as to why this wouldn't work
better in `git var` or elsewhere instead, since much of the information
is already there.  Since that's an alternative you've rejected, tell us
why and sell us on your vision.

> +	repo_config(the_repository, git_default_config, NULL);

Let's not add more uses of `the_repository`.  Use the `repo` argument to
the main function above, taking care to handle the NULL case.

> +	repo_config_get_bool(the_repository, "commit.gpgsign", &gpgsign);
> +	repo_config_get_string(the_repository, "user.signingkey", &signing_key);
> +	repo_config_get_string(the_repository, "gpg.format", &gpg_format);
> +	repo_config_get_string(the_repository, "gpg.ssh.defaultkeycommand", &ssh_default_key_cmd);
> +
> +	is_ssh = gpg_format && !strcmp(gpg_format, "ssh");
> +
> +	if (signing_key && *signing_key) {
> +		resolved_key = xstrdup(signing_key);
> +	} else if (is_ssh) {
> +		if (ssh_default_key_cmd && *ssh_default_key_cmd)
> +			resolved_key = get_signing_key_id();
> +	} else if (gpgsign) {
> +		resolved_key = get_signing_key_id();
> +	}

Should this also do something useful for X.509 keys?

Overall, I don't have a strong need for this and I'm fine using the
existing functionality.  However, I see how it could be useful and if it
were merged and available in the versions of Git I use, then I might
make use of it.

Perhaps others think this is compelling, though, so I'm interested to
hear other opinions about the utility of the command.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

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

>     builtin/whoami: add new 'whoami' command

In general, I would really really want to see us refrain from adding
any more random subcommands.

> +`-a`::
> +`--author`::
> +	Show author identity.
> +
> +`-c`::
> +`--committer`::
> +	Show committer identity.

This pair hints the users they are equals.

But the code tells us otherwise:

> +	if (show_name) {
> +		if (show_author)
> +			puts(author_name.buf);
> +		else
> +			puts(committer_name.buf);
> +		goto cleanup;
> +	}
> +

So when "-n" is in effect, "-c" is completely ignored.  Lack of "-a"
means "-c" instead.  The same story holds for "-e".

Yet later in the code that is executed when neither "-n" or "-e" is
in effect:

> +	if (show_author) {
> +		puts(author_info.buf);
> +		goto cleanup;
> +	}
> +
> +	if (show_committer) {
> +		puts(committer_info.buf);
> +		goto cleanup;
> +	}
> +

Here, lack of "-a" is not sufficient to view committer information
and you'd explicitly need to pass "-c" if you want to view committer
information.

So confusing.

> +`-n`::
> +`--name`::
> +	Show name only.
> +
> +`-e`::
> +`--email`::
> +	Show email only.

Why not make -a/-c/-n/-e more additive instead?  Something along the
lines of ...

    $ git ident -a -e -n
    Andrew Pleeter <andrewpleeter@gmail.com>
    $ git ident -a -n -v
    Author: Andrew Pleeter
    $ git ident -a -c -e
    <andrewpleeter@gmail.com>
    <andrewpleeter@gmail.com>
    $ git ident -a -c -e -v
    Author: <andrewpleeter@gmail.com>
    Committer: <andrewpleeter@gmail.com>

Hi Andrew

On 03/09/2026 03:49, Andrew Pleeter via GitGitGadget wrote:
> From: Andrew Pleeter <andrewpleeter@gmail.com>
> > - Support '-z' to terminate variable outputs and 'git var -l -z'
>    entries with NUL bytes.

Hurray! I agree with Junios comments and have left a few of my own below.
>   static const struct git_var *get_git_var(const char *var)
>   {
>   	struct git_var *ptr;
> +	if (!strcmp(var, "GIT_SIGNING_KEY"))
> +		var = "GIT_DEFAULT_KEY";

Do we really need an alias? GIT_DEFAULT_KEY is pretty meaningless to me, whereas GIT_SIGNING_KEY is clearly a key for signing. What's the usecase for this by the way. If a script is using git to sign then it does not need to query the default key because git will use it automatically. If a script wants to use the key to sign something else doesn't it need to also know which signing scheme git is using (ssh, gpg, etc) , or is that obvious from the key?

>   	for (ptr = git_vars; ptr->read; ptr++) {
>   		if (strcmp(var, ptr->name) == 0) {
>   			return ptr;
> @@ -207,10 +316,13 @@ static const struct git_var *get_git_var(const char *var)
>   static int show_config(const char *var, const char *value,
>   		       const struct config_context *ctx, void *cb)
>   {
> +	int null_term = cb ? *(int *)cb : 0;
> +	char eol = null_term ? '\0' : '\n';
> +
>   	if (value)
> -		printf("%s=%s\n", var, value);
> +		printf("%s=%s%c", var, value, eol);

A key can contain "=" so this format is ambiguous. When the user passes "-z" we should use the same format as "git config list -z" which avoids that ambiguity

	printf("%s%c%s%c", var, eol == '\n' ? '=' : '\n', value, eol);

>   	else
> -		printf("%s\n", var);
> +		printf("%s%c", var, eol);

It would be worth checking what "git config list -z" does when there is no value and matching that. Does it print "key\n\0", or "key\0"?

> +	for (i = 1; i < argc; i++) {
> +		const char *arg = argv[i];
> +
> +		if (!strcmp(arg, "-l")) {
> +			list = 1;
> +		} else if (!strcmp(arg, "-z")) {
> +			null_term = 1;
> +		} else if (!strcmp(arg, "--")) {
> +			for (i = i + 1; i < argc; i++)
> +				strvec_push(&vars, argv[i]);
> +			break;
> +		} else if (arg[0] == '-') {
> +			usage(var_usage);
> +		} else {
> +			strvec_push(&vars, arg);

I think we should break out of the loop when arg is "--", or does not begin with '-', and treat the rest as variable names to print. There is not need to support "git var GIT_AUTHOR_NAME -z GIT_AUTHOR_EMAIL" in a plumbing command.

> [...]
> -	printf("%s\n", val);
> -	free(val);
> +		printf("%s%c", val, null_term ? '\0' : '\n');

Multi-valued variables are a bit of a problem here, they're built on the assumption that the individual values do not contain a newline, but as they are paths I'm not sure that is necessarily true. With -z it would be better to print '\0' after each value as we do in list_vars(). Ideally we wouldn't use a single string to pass multiple values around, but a simple fix would be to use '\0' to separate the individual values instead of '\n' so that we can split them unambiguously when we print them.

Thanks for working on this, being able to specify multiple variables that are printed unambiguously is a really useful improvement.

Phillip
> +		free(val);
> +	}
>   > +	strvec_clear(&vars);
>   	return 0;
>   }
> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
> index 2b60317758..c437c968bb 100755
> --- a/t/t0007-git-var.sh
> +++ b/t/t0007-git-var.sh
> @@ -276,4 +276,81 @@ test_expect_success '`git var -l` works even without HOME' '
>   	)
>   '
>   > +test_expect_success 'get author identity components' '
> +	test_tick &&
> +	echo "$GIT_AUTHOR_NAME" >expect.name &&
> +	echo "$GIT_AUTHOR_EMAIL" >expect.email &&
> +	echo "$GIT_AUTHOR_DATE" >expect.date &&
> +	git var GIT_AUTHOR_NAME >actual.name &&
> +	git var GIT_AUTHOR_EMAIL >actual.email &&
> +	git var GIT_AUTHOR_DATE >actual.date &&
> +	test_cmp expect.name actual.name &&
> +	test_cmp expect.email actual.email &&
> +	test_cmp expect.date actual.date
> +'
> +
> +test_expect_success 'get committer identity components' '
> +	test_tick &&
> +	echo "$GIT_COMMITTER_NAME" >expect.name &&
> +	echo "$GIT_COMMITTER_EMAIL" >expect.email &&
> +	echo "$GIT_COMMITTER_DATE" >expect.date &&
> +	git var GIT_COMMITTER_NAME >actual.name &&
> +	git var GIT_COMMITTER_EMAIL >actual.email &&
> +	git var GIT_COMMITTER_DATE >actual.date &&
> +	test_cmp expect.name actual.name &&
> +	test_cmp expect.email actual.email &&
> +	test_cmp expect.date actual.date
> +'
> +
> +test_expect_success 'get multiple variables' '
> +	test_tick &&
> +	cat >expect <<-EOF &&
> +	$GIT_AUTHOR_NAME
> +	$GIT_AUTHOR_EMAIL
> +	$GIT_COMMITTER_NAME
> +	$GIT_COMMITTER_EMAIL
> +	EOF
> +	git var GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'get multiple variables with -z' '
> +	test_tick &&
> +	printf "%s\0%s\0" "$GIT_AUTHOR_NAME" "$GIT_AUTHOR_EMAIL" >expect &&
> +	git var -z GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git var -l -z' '
> +	git var -l -z >actual &&
> +	tr "\0" "\n" <actual | grep "^GIT_AUTHOR_NAME=" >filtered &&
> +	echo "GIT_AUTHOR_NAME=$GIT_AUTHOR_NAME" >expect &&
> +	test_cmp expect filtered
> +'
> +
> +test_expect_success 'get GIT_DEFAULT_KEY with user.signingkey configured' '
> +	test_config user.signingkey "TEST_KEY_ID" &&
> +	echo "TEST_KEY_ID" >expect &&
> +	git var GIT_DEFAULT_KEY >actual &&
> +	test_cmp expect actual &&
> +	git var GIT_SIGNING_KEY >actual.alias &&
> +	test_cmp expect actual.alias
> +'
> +
> +test_expect_success 'get GIT_DEFAULT_KEY fails when unset and signing disabled' '
> +	test_config user.signingkey "" &&
> +	test_config commit.gpgsign false &&
> +	test_must_fail git var GIT_DEFAULT_KEY
> +'
> +
> +test_expect_success 'git var -l lists new variables' '
> +	git var -l >actual &&
> +	grep "^GIT_AUTHOR_NAME=" actual &&
> +	grep "^GIT_AUTHOR_EMAIL=" actual &&
> +	grep "^GIT_AUTHOR_DATE=" actual &&
> +	grep "^GIT_COMMITTER_NAME=" actual &&
> +	grep "^GIT_COMMITTER_EMAIL=" actual &&
> +	grep "^GIT_COMMITTER_DATE=" actual
> +'
> +
>   test_done
> > base-commit: 2c3adbb2c475981e340c79fdc5e7f4f9b5d9054e

Phillip Wood <phillip.wood123@gmail.com> writes:

>>   	if (value)
>> -		printf("%s=%s\n", var, value);
>> +		printf("%s=%s%c", var, value, eol);
>
> A key can contain "=" so this format is ambiguous. When the user passes 
> "-z" we should use the same format as "git config list -z" which avoids 
> that ambiguity
>
> 	printf("%s%c%s%c", var, eol == '\n' ? '=' : '\n', value, eol);
>
>>   	else
>> -		printf("%s\n", var);
>> +		printf("%s%c", var, eol);
>
> It would be worth checking what "git config list -z" does when there is 
> no value and matching that. Does it print "key\n\0", or "key\0"?

By "key" do you mean "var"?  The namespace of "var" for "git var"
proper (like GIT_COMMITTER_IDENT) are very much under our control,
but it also gives all the configuration variables, whose names can
indeed have '=' in a three-level varlable name.  This is an
excellent suggestion.

> I think we should break out of the loop when arg is "--", or does not 
> begin with '-', and treat the rest as variable names to print. There is 
> not need to support "git var GIT_AUTHOR_NAME -z GIT_AUTHOR_EMAIL" in a 
> plumbing command.

Not limited to plumbing, but anywhere in Git.  Let's stick to and
force users adopt the simple rule that "git help cli" gives them.
Options first and then args, among which revs coe first and then
paths after disambiguating "--".  I know as historical wart some
commands may take dashed options after args, but I am fine if we
tightened the rule at Git 3.0 boundary to more strictly enforced
the option/argument ordering rule.

>> [...]
>> -	printf("%s\n", val);
>> -	free(val);
>> +		printf("%s%c", val, null_term ? '\0' : '\n');
>
> Multi-valued variables are a bit of a problem here, they're built on the 
> assumption that the individual values do not contain a newline, but as 
> they are paths I'm not sure that is necessarily true. With -z it would 
> be better to print '\0' after each value as we do in list_vars(). 
> Ideally we wouldn't use a single string to pass multiple values around, 
> but a simple fix would be to use '\0' to separate the individual values 
> instead of '\n' so that we can split them unambiguously when we print them.

Hmph, what does "git config -l" do for multi-valued keys?  We should
mimick it, I would think.

Another thing that might be worth doing is to see if we can separate
out "git config -l" handling out of "git var" with a breaking change
at big version boundary.

> Thanks for working on this, being able to specify multiple variables 
> that are printed unambiguously is a really useful improvement.

Indeed.  Thanks, both.

Hi Junio

On 04/09/2026 16:57, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> >>>    	if (value)
>>> -		printf("%s=%s\n", var, value);
>>> +		printf("%s=%s%c", var, value, eol);
>>
>> A key can contain "=" so this format is ambiguous. When the user passes
>> "-z" we should use the same format as "git config list -z" which avoids
>> that ambiguity
>>
>> 	printf("%s%c%s%c", var, eol == '\n' ? '=' : '\n', value, eol);
>>
>>>    	else
>>> -		printf("%s\n", var);
>>> +		printf("%s%c", var, eol);
>>
>> It would be worth checking what "git config list -z" does when there is
>> no value and matching that. Does it print "key\n\0", or "key\0"?
> > By "key" do you mean "var"? I meant the config key which is in variable var

> The namespace of "var" for "git var"
> proper (like GIT_COMMITTER_IDENT) are very much under our control,
> but it also gives all the configuration variables, whose names can
> indeed have '=' in a three-level varlable name.  This is an
> excellent suggestion.

>>> [...]
>>> -	printf("%s\n", val);
>>> -	free(val);
>>> +		printf("%s%c", val, null_term ? '\0' : '\n');
>>
>> Multi-valued variables are a bit of a problem here, they're built on the
>> assumption that the individual values do not contain a newline, but as
>> they are paths I'm not sure that is necessarily true. With -z it would
>> be better to print '\0' after each value as we do in list_vars().
>> Ideally we wouldn't use a single string to pass multiple values around,
>> but a simple fix would be to use '\0' to separate the individual values
>> instead of '\n' so that we can split them unambiguously when we print them.
> > Hmph, what does "git config -l" do for multi-valued keys?  We should
> mimick it, I would think.

With -z it nul terminates each value. I wonder if we should be printing the variable names here when the user passes more than one var name. That would make it easier to parse multivalued vars which can have a variable number of values, or we could print an extra delimiter after the last value of multivalued vars like "git merge-tree" does to separate the different sections of its output.

> Another thing that might be worth doing is to see if we can separate
> out "git config -l" handling out of "git var" with a breaking change
> at big version boundary.

Yes, it would be nice to be able to print just the GIT_* vars without having to print the config as well.

>> Thanks for working on this, being able to specify multiple variables
>> that are printed unambiguously is a really useful improvement.
> > Indeed.  Thanks, both.
> 

On 08/09/2026 05:09, Andrew Pleeter via GitGitGadget wrote:
> From: Andrew Pleeter <andrewpleeter@gmail.com>
> > While 'git var' exposes GIT_AUTHOR_IDENT and GIT_COMMITTER_IDENT,
> extracting individual components (name, email, or date) currently
> requires callers to manually parse the composite string. Furthermore,
> there is no way to query the resolved commit signing key through
> 'git var', and the command only accepts a single variable at a time.
> > Teach 'git var' to expose individual identity components and commit
> signing configuration, and allow querying multiple variables with
> optional NUL-termination:
> > - Add GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and GIT_AUTHOR_DATE.
> - Add GIT_COMMITTER_NAME, GIT_COMMITTER_EMAIL, and GIT_COMMITTER_DATE.
> - Add GIT_SIGNING_KEY to resolve the key that would be used to sign
>    the resulting commit if you were to run 'git commit' right now.
> - Allow passing multiple variable arguments (e.g., 'git var
>    GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL') to output each variable
>    sequentially.
> - Support '-z' to terminate variable outputs with NUL bytes.
> - Format 'git var -l -z' using the same convention as 'git config
>    list -z' (newline separating key and value, NUL separating entries).
> - Delimit values of multi-valued variables with NUL when '-z' is given.
> - Use parse_options() to strictly require options before arguments.
> - Update Documentation/git-var.adoc and t/t0007-git-var.sh.
> > Signed-off-by: Andrew Pleeter <andrewpleeter@gmail.com>
> ---
>      var: support broken-down idents, signing key, multiple args, and -z
>      >      Teach git var to expose individual identity components and commit
>      signing configuration, and allow querying multiple variables with
>      optional NUL-termination.
>      >      >      Changes since v3:
>      =================
>      >       * Renamed GIT_DEFAULT_KEY to GIT_SIGNING_KEY per feedback from Phillip
>         Wood and Junio C Hamano; dropped the alias mechanism and
>         commit.gpgsign check.
>       * Used parse_options() with PARSE_OPT_STOP_AT_NON_OPTION in
>         builtin/var.c, strictly enforcing that options precede variable
>         arguments.
>       * Adopted git config list -z format (key\nvalue\0) for git var -l -z to
>         prevent ambiguity with = in config keys.
>       * Delimited multi-valued variable outputs (e.g. GIT_CONFIG_GLOBAL) with
>         NUL bytes under -z.
>       * Replaced char part in ident_part() with enum ident_part.
>       * Split synopsis in Documentation/git-var.adoc into separate lines for
>         -l and <variable>..., and removed awkward legacy phrasing ("of a
>         piece of code").
>       * Added tests in t/t0007-git-var.sh covering the new -z format,
>         multi-valued -z, and argument ordering.
> That all sounds good, lets look at the code ...

> diff --git a/builtin/var.c b/builtin/var.c
> index cc3a43cde2..6fc037543a 100644
> --- a/builtin/var.c
> +++ b/builtin/var.c
> [...]> +static char *git_signing_key(int ident_flag UNUSED)
> +{
> +	char *signing_key = NULL;
> +
> +	/*
> +	 * An empty string in user.signingkey allows overriding and
> +	 * clearing a key defined in an outer (e.g. global) config.
> +	 */
> +	if (!repo_config_get_string(the_repository,
> +				    "user.signingkey", &signing_key)) {
> +		if (!signing_key || !*signing_key) {
> +			free(signing_key);
> +			return NULL;
> +		}
> +		return signing_key;
> +	}
> +
> +	signing_key = get_signing_key_id();
> +	if (signing_key && !*signing_key) {
> +		free(signing_key);
> +		return NULL;
> +	}
> +	return signing_key;
> +}

Looking at sign_buffer() in gpg-interface.c it looks like git calls get_signing_key() to obtain the default key - why are we doing something different here? I'm also still curious how this is expected to be used.

> -static void list_vars(void)
> +static void list_vars(int null_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);
> -			} else {
> -				printf("%s=%s\n", ptr->name, val);
> -			}
> -			free(val);
> +	char delim = null_term ? '\n' : '=';

We are in control of the variable names and we know they do not currently contain '=' so we don't currently need a different format here with '-z'. However it is possible that might change in the future (for example using "GIT_PAGER:<my-command>" to return the pager for "<my-command>" that could be an alias containing '=') so using the same format as config keys is probably a good idea. We should document the format above.

> +	char eol = null_term ? '\0' : '\n';
> +
> +	for (ptr = git_vars; ptr->read; ptr++) {
> +		char *val = ptr->read(0);
> +
> +		if (!val)
> +			continue;
> +
> +		if (ptr->multivalued && *val) {
> +			struct string_list list = STRING_LIST_INIT_DUP;
> +
> +			string_list_split(&list, val, "\n", -1);

As I said before, I think we should switch to using '\0' instead of '\n' when we build the multivalued string so that we can safely handle values that contain '\n'.

> +			for (size_t i = 0; i < list.nr; i++)
> +				printf("%s%c%s%c", ptr->name, delim,
> +				       list.items[i].string, eol);
> +			string_list_clear(&list, 0);
> +		} else {
> +			printf("%s%c%s%c", ptr->name, delim, val, eol);
>   		}
> +		free(val);
> +	}
>   }   > @@ -207,42 +346,76 @@ static const struct git_var *get_git_var(const char *var)
>   static int show_config(const char *var, const char *value,
>   		       const struct config_context *ctx, void *cb)
>   {
> +	int null_term = cb ? *(int *)cb : 0;

This seems unnecessarily complicated, can't we just make sure we always pass a non-null pointer cb? Also '\0' is known as NUL, not NULL.

	int *nul_term = cb;
	char term = *nul_term ? '\0' : '\n';
	char delim = *nul_term ? '\n' : '=';

and then use term and delim below.

> +
>   	if (value)
> -		printf("%s=%s\n", var, value);
> +		printf("%s%c%s%c", var, null_term ? '\n' : '=',
> +		       value, null_term ? '\0' : '\n');
>   	else
> -		printf("%s\n", var);
> +		printf("%s%c", var, null_term ? '\0' : '\n');
>   	return git_default_config(var, value, ctx, cb);
>   }
>   >   int cmd_var(int argc,
>   	    const char **argv,
> -	    const char *prefix UNUSED,
> +	    const char *prefix,
>   	    struct repository *repo UNUSED)
>   {
> -	const struct git_var *git_var;
> -	char *val;
> +	int list = 0;
> +	int null_term = 0;
> +	int i;
> +	struct option options[] = {
> +		OPT_BOOL('l', NULL, &list,
> +			 N_("list all variables")),
> +		OPT_BOOL('z', NULL, &null_term,
> +			 N_("terminate entries with NUL")),

The help is correct, we should use nul_term as the variable name. Using parse_options() is a nice improvement.

> +		OPT_END(),
> +	};
>   > -	show_usage_if_asked(argc, argv, var_usage);
> -	if (argc != 2)
> -		usage(var_usage);
> +	argc = parse_options(argc, argv, prefix, options,
> +			     var_usage, PARSE_OPT_STOP_AT_NON_OPTION);
>   > -	if (strcmp(argv[1], "-l") == 0) {
> -		repo_config(the_repository, show_config, NULL);
> -		list_vars();
> +	if (list) {
> +		if (argc)
> +			usage_with_options(var_usage, options);
> +		repo_config(the_repository, show_config, &null_term);
> +		list_vars(null_term);
>   		return 0;
>   	}
> +
> +	if (!argc)
> +		usage_with_options(var_usage, options);
> +
> +	for (i = 0; i < argc; i++) {
> +		if (!get_git_var(argv[i]))
> +			usage_with_options(var_usage, options);

Do we really need to walk all the var names here - can't we just error out if we see an invalid one later?

> +	}
> +
>   	repo_config(the_repository, git_default_config, NULL);
>   > -	git_var = get_git_var(argv[1]);
> -	if (!git_var)
> -		usage(var_usage);
> +	for (i = 0; i < argc; i++) {
> +		const struct git_var *git_var = get_git_var(argv[i]);
> +		char *val;
> +
> +		val = git_var->read(IDENT_STRICT);
> +		if (!val)
> +			return 1;

If the user asked for multiple vars to be printed, erroring out because one is not set is not very friendly, It would be better to print a blank record and continue.

>   > -	val = git_var->read(IDENT_STRICT);
> -	if (!val)
> -		return 1;
> +		if (git_var->multivalued && null_term && *val) {

Why "*val" ?

> +			struct string_list values = STRING_LIST_INIT_DUP;
>   > -	printf("%s\n", val);
> -	free(val);
> +			string_list_split(&values, val, "\n", -1);
> +			for (size_t j = 0; j < values.nr; j++) {
> +				const char *s = values.items[j].string;
> +
> +				printf("%s%c", s, '\0');

If we're printing multiple var then the caller has no way to tell if a var has multiple values which makes it tricky or impossible to match up the values we print to the vars that were requested. We could change the output format when multiple vars are requested to print the var name as will like we do with '-l', or we could print an extra terminator after a multi-valued var and properly document which vars are multi-valued. The latter means the caller can match up the values without worrying about parsing the var names.

Thanks

Phillip

> +			}
> +			string_list_clear(&values, 0);
> +		} else {
> +			printf("%s%c", val, null_term ? '\0' : '\n');
> +		}
> +		free(val);
> +	}
>   >   	return 0;
>   }
> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
> index 2b60317758..27cc595291 100755
> --- a/t/t0007-git-var.sh
> +++ b/t/t0007-git-var.sh
> @@ -276,4 +276,99 @@ test_expect_success '`git var -l` works even without HOME' '
>   	)
>   '
>   > +test_expect_success 'get author identity components' '
> +	test_tick &&
> +	echo "$GIT_AUTHOR_NAME" >expect.name &&
> +	echo "$GIT_AUTHOR_EMAIL" >expect.email &&
> +	echo "$GIT_AUTHOR_DATE" >expect.date &&
> +	git var GIT_AUTHOR_NAME >actual.name &&
> +	git var GIT_AUTHOR_EMAIL >actual.email &&
> +	git var GIT_AUTHOR_DATE >actual.date &&
> +	test_cmp expect.name actual.name &&
> +	test_cmp expect.email actual.email &&
> +	test_cmp expect.date actual.date
> +'
> +
> +test_expect_success 'get committer identity components' '
> +	test_tick &&
> +	echo "$GIT_COMMITTER_NAME" >expect.name &&
> +	echo "$GIT_COMMITTER_EMAIL" >expect.email &&
> +	echo "$GIT_COMMITTER_DATE" >expect.date &&
> +	git var GIT_COMMITTER_NAME >actual.name &&
> +	git var GIT_COMMITTER_EMAIL >actual.email &&
> +	git var GIT_COMMITTER_DATE >actual.date &&
> +	test_cmp expect.name actual.name &&
> +	test_cmp expect.email actual.email &&
> +	test_cmp expect.date actual.date
> +'
> +
> +test_expect_success 'get multiple variables' '
> +	test_tick &&
> +	cat >expect <<-EOF &&
> +	$GIT_AUTHOR_NAME
> +	$GIT_AUTHOR_EMAIL
> +	$GIT_COMMITTER_NAME
> +	$GIT_COMMITTER_EMAIL
> +	EOF
> +	git var GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'get multiple variables with -z' '
> +	test_tick &&
> +	printf "%s\0" "$GIT_AUTHOR_NAME" "$GIT_AUTHOR_EMAIL" >expect &&
> +	git var -z GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'get multi-valued variable with -z' '
> +	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
> +	HOME="$TRASHDIR" XDG_CONFIG_HOME="$TRASHDIR/foo" git var -z GIT_CONFIG_GLOBAL >actual &&
> +	printf "%s\0" "$TRASHDIR/foo/git/config" "$TRASHDIR/.gitconfig" >expected &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'git var -l -z' '
> +	git var -l -z >actual &&
> +	tr "\0" "\n" <actual >actual.lines &&
> +	echo "$GIT_AUTHOR_NAME" >expect &&
> +	sed -n "/^GIT_AUTHOR_NAME$/{n;p;}" actual.lines >actual.author &&
> +	test_cmp expect actual.author &&
> +	echo false >expect &&
> +	sed -n "/^core\.bare$/{n;p;}" actual.lines >actual.bare &&
> +	test_cmp expect actual.bare
> +'
> +
> +test_expect_success 'get GIT_SIGNING_KEY with user.signingkey configured' '
> +	test_config user.signingkey "TEST_KEY_ID" &&
> +	echo "TEST_KEY_ID" >expect &&
> +	git var GIT_SIGNING_KEY >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'get GIT_SIGNING_KEY fails when unset' '
> +	test_config user.signingkey "" &&
> +	test_must_fail git var GIT_SIGNING_KEY
> +'
> +
> +test_expect_success 'git var -l lists new variables' '
> +	git var -l >actual &&
> +	test_grep "^GIT_AUTHOR_NAME=" actual &&
> +	test_grep "^GIT_AUTHOR_EMAIL=" actual &&
> +	test_grep "^GIT_AUTHOR_DATE=" actual &&
> +	test_grep "^GIT_COMMITTER_NAME=" actual &&
> +	test_grep "^GIT_COMMITTER_EMAIL=" actual &&
> +	test_grep "^GIT_COMMITTER_DATE=" actual
> +'
> +
> +test_expect_success 'git var -l lists GIT_SIGNING_KEY when configured' '
> +	test_config user.signingkey "TEST_KEY_ID" &&
> +	git var -l >actual &&
> +	test_grep "^GIT_SIGNING_KEY=TEST_KEY_ID" actual
> +'
> +
> +test_expect_success 'options must precede variable arguments' '
> +	test_must_fail git var GIT_AUTHOR_NAME -z
> +'
> +
>   test_done
> > base-commit: 2c3adbb2c475981e340c79fdc5e7f4f9b5d9054e

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

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

> @@ -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?

> +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?

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

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

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

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

	}

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

Hi Andrew

On 09/09/2026 02:24, Andrew Pleeter via GitGitGadget wrote:
> From: Andrew Pleeter <andrewpleeter@gmail.com>
> > While 'git var' exposes GIT_AUTHOR_IDENT and GIT_COMMITTER_IDENT,
> extracting individual components (name, email, or date) currently
> requires callers to manually parse the composite string. Furthermore,
> there is no way to query the resolved commit signing key through
> 'git var', and the command only accepts a single variable at a time.
> > Teach 'git var' to expose individual identity components and commit
> signing configuration, and allow querying multiple variables with
> optional NUL-termination:
> > - Add GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and GIT_AUTHOR_DATE.
> - Add GIT_COMMITTER_NAME, GIT_COMMITTER_EMAIL, and GIT_COMMITTER_DATE.
> - Add GIT_SIGNING_KEY to resolve the key that would be used to sign
>    the resulting commit if you were to run 'git commit' right now.

I'm still curious what the use case for GIT_SIGNING_KEY is. Is the key alone enough for the caller to determine if they should be using gpg or ssh? I've asked this twice already - when a reviewer asks a question it is helpful to rely rather than just sending a new version of the patch.
> - Allow passing multiple variable arguments (e.g., 'git var
>    GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL') to output each variable
>    sequentially.
> - Support '-z' to terminate variable outputs with NUL bytes.
> - Format 'git var -l -z' using the same convention as 'git config
>    list -z' (newline separating key and value, NUL separating entries).
> - Delimit values of multi-valued variables with NUL when '-z' is given,
>    and output an extra delimiter after multi-valued variables when
>    querying multiple variables to disambiguate the stream.
> - When querying multiple variables, print an empty record for any
>    variable that has no value and continue processing remaining variables.
> - Use parse_options() to strictly require options before arguments.
> - Update Documentation/git-var.adoc and t/t0007-git-var.sh.
> > Signed-off-by: Andrew Pleeter <andrewpleeter@gmail.com>
> ---

> diff --git a/Documentation/git-var.adoc b/Documentation/git-var.adoc
> index 697c10aded..2c1eaf3cf7 100644
> --- a/Documentation/git-var.adoc
> +++ b/Documentation/git-var.adoc
> @@ -9,12 +9,16 @@ git-var - Show a Git logical variable
>   SYNOPSIS
>   --------
>   [synopsis]
> -git var (-l | <variable>)
> +git var [-z] -l
> +git var [-z] <variable>...
>   >   DESCRIPTION
>   -----------
> -Prints a Git logical variable. Exits with code 1 if the variable has
> -no value.
> +Prints Git logical variables. Exits with code 1 if any requested
> +variable has no value.

I'm not sure that is very useful when the user asks for more than one variable - they can see the value was empty by looking at the output and means callers cannot check for fatal errors such as an invalid variable name by simply looking for a non-zero exit code.

> 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.
>   >   OPTIONS
>   -------
> @@ -24,19 +28,65 @@ OPTIONS
>   	as well. (However, the configuration variables listing functionality
>   	is deprecated in favor of `git config list`.)
>   > +`-z`::
> +	Terminate entries with NUL instead of newline. When used with
> +	`-l`, the variable name and its value are separated by a newline,
> +	and each entry is terminated with a NUL byte.

Good

> @@ -85,9 +135,13 @@ 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.

We should mark each mult-valued varibale in its description so that users know when to expect a list.

> (Single-variable queries and `git var -l` do not print
> +an extra delimiter). Callers should be prepared for any such path value to
> +contain multiple items.
>   >   Note that paths are printed even if they do not exist, but not if they are
>   disabled by other environment variables.

> -static char *git_config_val_global(int ident_flag UNUSED)
> +static int git_config_val_global(struct string_list *list)
>   {
> -	struct strbuf buf = STRBUF_INIT;
>   	char *user, *xdg;
> -	size_t unused;
>   >   	git_global_config_paths(&user, &xdg);
>   	if (xdg && *xdg) {
>   		normalize_path_copy(xdg, xdg);
> -		strbuf_addf(&buf, "%s\n", xdg);
> +		string_list_append(list, xdg);
>   	}
>   	if (user && *user) {
>   		normalize_path_copy(user, user);
> -		strbuf_addf(&buf, "%s\n", user);
> +		string_list_append(list, user);
>   	}
>   	free(xdg);
>   	free(user);
> -	strbuf_trim_trailing_newline(&buf);
> -	if (buf.len == 0) {
> -		strbuf_release(&buf);
> -		return NULL;
> -	}
> -	return strbuf_detach(&buf, &unused);
> +	return !list->nr;
>   }

This is a nice improvement that could perhaps be split out into a separate preparatory change together with the change from a flag to a different read function for multi-valued variables below.

>   >   struct git_var {
>   	const char *name;
>   	char *(*read)(int);
> -	int multivalued;
> +	int (*multiread)(struct string_list *);
>   };

> -static void list_vars(void)
> +static void list_vars(int nul_term)
>   {
>   	struct git_var *ptr;
> -	char *val;
> +	char delim = nul_term ? '\n' : '=';
> +	char term = nul_term ? '\0' : '\n';
>   > -	for (ptr = git_vars; ptr->read; ptr++)
> -		if ((val = ptr->read(0))) {
> -			if (ptr->multivalued && *val) {
> -				struct string_list list = STRING_LIST_INIT_DUP;
> +	for (ptr = git_vars; ptr->read || ptr->multiread; ptr++) {
> +		if (ptr->read) {
> +			char *val = ptr->read(0);
>   > -				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);
> -			} else {
> -				printf("%s=%s\n", ptr->name, val);
> +			if (val) {
> +				printf("%s%c%s%c", ptr->name, delim, val, term);
> +				free(val);
>   			}
> -			free(val);
> +		} else if (ptr->multiread) {

We should just assume that ptr->multread is set when ptr->read is not, or possibly add an else clause that calls BUG().

> +			struct string_list list = STRING_LIST_INIT_DUP;
> +			size_t i;
> +
> +			if (!ptr->multiread(&list)) {
> +				for (i = 0; i < list.nr; i++)
> +					printf("%s%c%s%c", ptr->name, delim,
> +					       list.items[i].string, term);
> +			}
> +			string_list_clear(&list, 0);

>   int cmd_var(int argc,
>   	    const char **argv,
> -	    const char *prefix UNUSED,
> +	    const char *prefix,
>   	    struct repository *repo UNUSED)
> [...]
> +	for (i = 0; i < argc; i++) {
> +		const struct git_var *git_var = get_git_var(argv[i]);
>   > -	printf("%s\n", val);
> -	free(val);
> +		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;
> +				ret = 1;

What's the benefit of this? The caller can see there was an empty value so why do we want a non-zero exit code as well. For example, if the caller is asking for GIT_CONFIG_SYSTEM and GIT_CONFIG_GLOBAL but the user ran the script with GIT_CONFIG_NOSYSTEM then that shouldn't be an error - the caller should just not use the system config.

> +				printf("%c", term);
> +				continue;
> +			}
> +			printf("%s%c", val, term);
> +			free(val);
> +		} else if (git_var->multiread) {
> +			struct string_list list = STRING_LIST_INIT_DUP;
> +			size_t j;
> +
> +			if (git_var->multiread(&list) || !list.nr) {

Why are we checking the return value of the function and the list length - surely the list length tells us everything we need to know.

> +				if (argc == 1) {
> +					string_list_clear(&list, 0);
> +					return 1;
> +				}
> +				ret = 1;
> +				printf("%c", term);
> +			} else {
> +				for (j = 0; j < list.nr; j++)
> +					printf("%s%c", list.items[j].string, term);
> +				if (argc > 1)
> +					printf("%c", term);
> +			}
> +			string_list_clear(&list, 0);

I think the above can be simplified to

	} else {
		struct string_list list = STRING_LIST_INIT_NODUP;
		
		git_var->multiread(&list);
		if (argc == 1 && !list.nr) {
			return 1;
		}
		for (j = 0; j < list.nr; j++)
			printf("%s%c", list.items[j].string, term);
		if (argc > 1)
			putc(term);

		string_list_clear(&list, 0);
	}

I've not had time to look too closely at the tests, but I did notice they use test_cmp() on files containing '\0' which isn't a good idea because diff will see them as binary files. We have helpers like nul_to_q to translate nul to a printable character. I'm going to be off the list from tomorrow until the middle of next week so it will be a few days before I look at the next (and hopefully final) version.

Thanks

Phillip

> +		}
> +	}
>   > -	return 0;
> +	return ret;
>   }
> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
> index 2b60317758..92b68b9ab4 100755
> --- a/t/t0007-git-var.sh
> +++ b/t/t0007-git-var.sh
> @@ -276,4 +276,127 @@ test_expect_success '`git var -l` works even without HOME' '
>   	)
>   '
>   > +test_expect_success 'get author identity components' '
> +	test_tick &&
> +	echo "$GIT_AUTHOR_NAME" >expect.name &&
> +	echo "$GIT_AUTHOR_EMAIL" >expect.email &&
> +	echo "$GIT_AUTHOR_DATE" >expect.date &&
> +	git var GIT_AUTHOR_NAME >actual.name &&
> +	git var GIT_AUTHOR_EMAIL >actual.email &&
> +	git var GIT_AUTHOR_DATE >actual.date &&
> +	test_cmp expect.name actual.name &&
> +	test_cmp expect.email actual.email &&
> +	test_cmp expect.date actual.date
> +'
> +
> +test_expect_success 'get committer identity components' '
> +	test_tick &&
> +	echo "$GIT_COMMITTER_NAME" >expect.name &&
> +	echo "$GIT_COMMITTER_EMAIL" >expect.email &&
> +	echo "$GIT_COMMITTER_DATE" >expect.date &&
> +	git var GIT_COMMITTER_NAME >actual.name &&
> +	git var GIT_COMMITTER_EMAIL >actual.email &&
> +	git var GIT_COMMITTER_DATE >actual.date &&
> +	test_cmp expect.name actual.name &&
> +	test_cmp expect.email actual.email &&
> +	test_cmp expect.date actual.date
> +'
> +
> +test_expect_success 'get multiple variables' '
> +	test_tick &&
> +	cat >expect <<-EOF &&
> +	$GIT_AUTHOR_NAME
> +	$GIT_AUTHOR_EMAIL
> +	$GIT_COMMITTER_NAME
> +	$GIT_COMMITTER_EMAIL
> +	EOF
> +	git var GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'get multiple variables with -z' '
> +	test_tick &&
> +	printf "%s\0" "$GIT_AUTHOR_NAME" "$GIT_AUTHOR_EMAIL" >expect &&
> +	git var -z GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'get multi-valued variable with -z' '
> +	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
> +	HOME="$TRASHDIR" XDG_CONFIG_HOME="$TRASHDIR/foo" git var -z GIT_CONFIG_GLOBAL >actual &&
> +	printf "%s\0" "$TRASHDIR/foo/git/config" "$TRASHDIR/.gitconfig" >expected &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'git var -l -z' '
> +	git var -l -z >actual &&
> +	tr "\0" "\n" <actual >actual.lines &&
> +	echo "$GIT_AUTHOR_NAME" >expect &&
> +	sed -n "/^GIT_AUTHOR_NAME$/{n;p;}" actual.lines >actual.author &&
> +	test_cmp expect actual.author &&
> +	echo false >expect &&
> +	sed -n "/^core\.bare$/{n;p;}" actual.lines >actual.bare &&
> +	test_cmp expect actual.bare
> +'
> +
> +test_expect_success 'get GIT_SIGNING_KEY with user.signingkey configured' '
> +	test_config user.signingkey "TEST_KEY_ID" &&
> +	echo "TEST_KEY_ID" >expect &&
> +	git var GIT_SIGNING_KEY >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'get GIT_SIGNING_KEY fails when unset' '
> +	test_config user.signingkey "" &&
> +	test_must_fail git var GIT_SIGNING_KEY
> +'
> +
> +test_expect_success 'git var -l lists new variables' '
> +	git var -l >actual &&
> +	test_grep "^GIT_AUTHOR_NAME=" actual &&
> +	test_grep "^GIT_AUTHOR_EMAIL=" actual &&
> +	test_grep "^GIT_AUTHOR_DATE=" actual &&
> +	test_grep "^GIT_COMMITTER_NAME=" actual &&
> +	test_grep "^GIT_COMMITTER_EMAIL=" actual &&
> +	test_grep "^GIT_COMMITTER_DATE=" actual
> +'
> +
> +test_expect_success 'git var -l lists GIT_SIGNING_KEY when configured' '
> +	test_config user.signingkey "TEST_KEY_ID" &&
> +	git var -l >actual &&
> +	test_grep "^GIT_SIGNING_KEY=TEST_KEY_ID" actual
> +'
> +
> +test_expect_success 'options must precede variable arguments' '
> +	test_must_fail git var GIT_AUTHOR_NAME -z
> +'
> +
> +test_expect_success 'get multiple variables with unset variable outputs blank record' '
> +	test_config user.signingkey "" &&
> +	cat >expect <<-EOF &&
> +	$GIT_AUTHOR_NAME
> +
> +	$GIT_COMMITTER_NAME
> +	EOF
> +	test_must_fail git var GIT_AUTHOR_NAME GIT_SIGNING_KEY GIT_COMMITTER_NAME >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'get multiple variables with -z and unset variable' '
> +	test_config user.signingkey "" &&
> +	printf "%s\0\0%s\0" "$GIT_AUTHOR_NAME" "$GIT_COMMITTER_NAME" >expect &&
> +	test_must_fail git var -z GIT_AUTHOR_NAME GIT_SIGNING_KEY GIT_COMMITTER_NAME >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'get multiple variables including multi-valued variable with -z' '
> +	TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" &&
> +	printf "%s\0%s\0%s\0\0%s\0" "$GIT_AUTHOR_NAME" \
> +		"$TRASHDIR/foo/git/config" "$TRASHDIR/.gitconfig" \
> +		"$GIT_AUTHOR_EMAIL" >expect &&
> +	HOME="$TRASHDIR" XDG_CONFIG_HOME="$TRASHDIR/foo" \
> +		git var -z GIT_AUTHOR_NAME GIT_CONFIG_GLOBAL GIT_AUTHOR_EMAIL >actual &&
> +	test_cmp expect actual
> +'
> +
>   test_done
> > base-commit: 2c3adbb2c475981e340c79fdc5e7f4f9b5d9054e

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

>  VARIABLES
>  ---------
>  `GIT_AUTHOR_IDENT`::
> -    The author of a piece of code.
> +    The author name, email, and date that would be used if you were to
> +    run `git commit` right now.
> +
> +`GIT_AUTHOR_NAME`::
> +    The author name that would be used if you were to run `git commit`
> +    right now.
> +
> +`GIT_AUTHOR_EMAIL`::
> +    The author email that would be used if you were to run `git commit`
> +    right now.
> +
> +`GIT_AUTHOR_DATE`::
> +    The author date and timezone that would be used if you were to run
> +    `git commit` right now.

This is better than the previous iteration, but wastes reader's time
with full of repetitions.  Have you looked at the one suggested in
https://lore.kernel.org/git/xmqqbjaecjxb.fsf@gitster.g/ for example
to present more information in much more concise way?

>  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. (Single-variable queries and `git var -l` do not print
> +an extra delimiter). Callers should be prepared for any such path value to
> +contain multiple items.

This makes it clear that the query forms we had before this change
will produce the same output, and that queries asking for more than
one value use a new format, which is good.

However, I am not sure why we want an extra delimiter only after a
multi-valued variable.  Does it mean that the reading script needs
to be aware of which variables are multi-valued and which are not?
It is not clear whether this extra delimiter is present only when a
potentially multi-valued variable actually has multiple values, or if
we will have the extra delimiter even when such a variable happens to
have only a single (or perhaps zero) value.

Especially given that ...

>  Note that paths are printed even if they do not exist, but not if they are
>  disabled by other environment variables.

... some paths may not be printed even when explicitly requested in
the new "give me values of these variables" form, it appears to me
that the extra delimiter, even with the reader's knowledge of which
variables are multi-valued, does not help identify which value
corresponds to which requested variable.  I can accept, to a limited
degree, the argument that a list of 'val' is less work to parse than
a list of 'var=val' simply because you do not have to strip 'var='
from the front.  However, it looks to me that the proposed format
makes the wrong trade-off by making it harder to match a variable to
its value(s).

> diff --git a/builtin/var.c b/builtin/var.c
> index cc3a43cde2..decada1602 100644
> +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:
> +		return xmemdupz(split.name_begin,
> +				split.name_end - split.name_begin);
> +	case IDENT_MAIL:
> +		return xmemdupz(split.mail_begin,
> +				split.mail_end - split.mail_begin);

This is better in that it no longer returns NULL upon an impossible
condition like the previous iteration did.  Even better, we could
protect ourselves against breakage caused by careless updates to
split_ident_line() and git_*_info() functions we rely on by keep the
check but mark BUG(), e.g.,

                case IDENT_NAME:
        +		if (!split.name_begin || !split.name_end)
        +			BUG("split_ident_line() gave NULL names???");
                        return xmemdupz(split.name_begin,
                                        split.name_end - split.name_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;

I gave ".name_begin/.name_end cannot be NULL with the way you call
the other routines" in my previous response as a mere example, while
hoping that you'd do similar due dilligence to other values.  With
the way committer_date() and author_date() are called (below), can
fmt_ident() ever return an ident without datestamp and timezone,
requiring us to fall back on NULL returns like this?  You are not
passing IDENT_NO_DATE flag anywhere, are you?

>  struct git_var {
>  	const char *name;
>  	char *(*read)(int);
> -	int multivalued;
> +	int (*multiread)(struct string_list *);
>  };
> ...
>  int cmd_var(int argc,
>  	    const char **argv,
> -	    const char *prefix UNUSED,
> +	    const char *prefix,
>  	    struct repository *repo UNUSED)
>  {
> +	int list = 0;
> +	int nul_term = 0;
> +	int ret = 0;
> +	int i;
> +	char term;
> +	struct option options[] = {
> +		OPT_BOOL('l', NULL, &list,
> +			 N_("list all variables")),
> +		OPT_BOOL('z', NULL, &nul_term,
> +			 N_("terminate entries with NUL")),
> +		OPT_END(),
> +	};
>  
> +	argc = parse_options(argc, argv, prefix, options,
> +			     var_usage, PARSE_OPT_STOP_AT_NON_OPTION);
>  
> +	if (list) {
> +		if (argc)
> +			usage_with_options(var_usage, options);
> +		repo_config(the_repository, show_config, &nul_term);
> +		list_vars(nul_term);
>  		return 0;
>  	}

OK.  Using "-l" and having named variables are incompatible.

> +
> +	if (!argc)
> +		usage_with_options(var_usage, options);

And not having named variables without "-l" invites the usage
message.  We used to call usage() that exits with 129 and
usage_with_options() does so, too.

>  	repo_config(the_repository, git_default_config, NULL);
>  
> +	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;
> +				ret = 1;
> +				printf("%c", term);
> +				continue;
> +			}

So the idea is when there is a single var on the command line,
missing value gives no output and exit(1), but in the new "more than
one variable" mode, we note the fact that we had one failing
variable, emit a line terminator (NUL or LF) to help readers that
expect one "line" per request.

> +			printf("%s%c", val, term);
> +			free(val);
> +		} else if (git_var->multiread) {
> +			struct string_list list = STRING_LIST_INIT_DUP;
> +			size_t j;
> +
> +			if (git_var->multiread(&list) || !list.nr) {
> +				if (argc == 1) {
> +					string_list_clear(&list, 0);
> +					return 1;
> +				}
> +				ret = 1;
> +				printf("%c", term);

The same for variables that may have multiple values when they lack
any value.

> +			} else {
> +				for (j = 0; j < list.nr; j++)
> +					printf("%s%c", list.items[j].string, term);
> +				if (argc > 1)
> +					printf("%c", term);

So this answers the question I had on ambiguous documentation.  A
variable that can have multiple values (including 0 values) will
have N "lines" of N values, plus an empty "line".


> +			}
> +			string_list_clear(&list, 0);
> +		}
> +	}
>  
> +	return ret;
>  }

And we return "ret" that memorizes if we ever had a failure in the
middle.  When there is no failure, we return 0 that is the value
"ret" was initialized with.

If I were designing this, I'd rather (1) get rid of the "empty line"
convention for multi-valued variables, and (2) model multi-variable
mode more after "-l" mode.  IOW, instead of thinking of the case
where the user gave us two variables like two "git var VARIBLE$N"
calls given back to back, thinking it more like "git var -l | grep
-e VARIABLE1= -e VARIABLE2=".

Thanks.