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

[Phillip Wood <phillip.wood123@gmail.com>]

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