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>

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

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

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

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

The huge laundary list (below) strongly tells us that this single
patch is doing too many things at once and it is better done as a
multi-patch series.  Also, some descriptions seem to hint how the
implementation evolved during the development of this patch, which
nobody is interested when they read "git log" output (which is the
ultimate target audience we write our commit log messages for).

I would say this should be split into at least 3 patches.

 (1) Add "-z" output mode.

     To allow reading scripts to unambiguously parse output from
     "git var (-l | <var>)" command, implement a NUL terminated
     output mode, similar to how "git config -l -z" shows list of
     configuration variables and their values.  When showing the
     value of a single variable this only makes difference for
     variable with multiple values, but in the next step in this
     series, we will introduce a mode where multiple variables are
     queried.

 (2) Add (2 <= argc) mode that displays like "var -l" mode on top.

     To allow reading values for multiple variables with a single
     command invocation, teach "var" to take more than one variable,
     and show output the same way as "git var -l [-z]", giving list
     of "var=value" but only for variables requested by the user.

 (3) Add new variables.

     Scripts reading from "git var GIT_AUTHOR_IDENT" needs to parse
     the output if they want to extract only the author name.  

     To allow scripts to easily access broken-out fields of
     GIT_{AUTHOR,COMMITTER}_IDENT, add a GIT_AUTHOR_NAME variable
     and its friends, as well as GIT_SIGNING_KEY.

After you receive a review, you should respond and try to engage in
a dialog with reviewers, before sending a new iteration of a patch.

When your new iteration is different from what reviewer suggested,
without such an exchange beforehand, reviewers cannot tell if that
is merely due to miscommunication, or you had a good reason to do
things differently.  Don't make reviewers feel as if they are
talking to silent machine that takes an earlier iteration of the
patch with their input and spits out a new iteration.

Thanks.

> - 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').
> - When a single variable is requested, print its bare value for backward
>   compatibility.
> - When multiple variables are requested, model the output after
>   'git var -l' by printing 'VARIABLE=value' pairs (or 'VARIABLE\nvalue\0'
>   when '-z' is given).
> - Format multi-valued variables in multi-variable mode as repeated
>   'VARIABLE=value' entries (or 'VARIABLE\nvalue\0' with '-z'),
>   eliminating stream ambiguity without extra trailing delimiters.
> - When querying multiple variables, omit any variable that has no value,
>   continue processing remaining variables, and exit with code 1.
> - 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).
> - Use parse_options() to strictly require options before arguments.
> - Update Documentation/git-var.adoc and t/t0007-git-var.sh.