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

[Junio C Hamano <gitster@pobox.com>]

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

> - Allow passing multiple variable arguments (e.g., 'git var
>   GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL') to output each variable
>   sequentially.

Yes.  This would be really useful if anybody wants to make it a
component in serious scripting.

>  Documentation/git-var.adoc |  43 ++++++++-
>  builtin/var.c              | 189 ++++++++++++++++++++++++++++++++-----
>  t/t0007-git-var.sh         |  77 +++++++++++++++
>  3 files changed, 284 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/git-var.adoc b/Documentation/git-var.adoc
> index 697c10aded..30bf2c12a8 100644
> --- a/Documentation/git-var.adoc
> +++ b/Documentation/git-var.adoc
> @@ -9,7 +9,7 @@ git-var - Show a Git logical variable
>  SYNOPSIS
>  --------
>  [synopsis]
> -git var (-l | <variable>)
> +git var (-l [-z] | [-z] <variable>...)

It might make sense to split the two vastly different modes of
operation into separate lines in the synopsis, i.e.,

    git var [-z] -l
    git var [-z] <variable>...

>  VARIABLES
>  ---------
>  `GIT_AUTHOR_IDENT`::
>      The author of a piece of code.

This shows that " of a piece of code" was inherited from the
original, and while it is not your fault, the phrasing is awkward
and misleading.  When a user runs:

    $ git var GIT_AUTHOR_DATE

the command does not look at any particular piece of code or report
when it was written.  We are better off without " of a piece of
code" in this entry (unless we can replace it with something better)
and in all the other new entries.

> +`GIT_AUTHOR_NAME`::
> +    The name of the author of a piece of code.
> +
> +`GIT_AUTHOR_EMAIL`::
> +    The email of the author of a piece of code.
> +
> +`GIT_AUTHOR_DATE`::
> +    The date and timezone of the author of a piece of code.

So let's discuss what we can replace "of a piece of code" with.

The original motivation for GIT_AUTHOR_IDENT (and similarly
GIT_COMMITTER_IDENT) was to give scripts a way to construct a
string that they can pass to 'git hash-object -t commit' to create a
commit object.  GIT_AUTHOR_IDENT is what would appear on the
"author" line (and GIT_COMMITTER_IDENT on the "committer" line) in
the resulting commit object if you were to run 'git commit' right
now.  IDENT has a clear meaning (given above); the other three are
individual fields broken out of it.

Explaining these four (IDENT, NAME, EMAIL, DATE) along those lines
would make it easier for readers to understand.

Here is my attempt:

    GIT_AUTHOR_IDENT::
    GIT_AUTHOR_NAME::
    GIT_AUTHOR_EMAIL::
    GIT_AUTHOR_DATE::
        The authorship information that would be recorded in the
        resulting commit object if you ran 'git commit' right now.
        GIT_AUTHOR_IDENT consists of the author's name, e-mail
        address, and timestamp+timezone.  These three pieces of
        information are available separately as GIT_AUTHOR_NAME,
        GIT_AUTHOR_EMAIL, and GIT_AUTHOR_DATE.

Similarly for GIT_COMMITTER_*.


> +`GIT_DEFAULT_KEY`::
> +    The default commit signing key ID or fingerprint, if configured or enabled.

In the same spirit as the explanation of 'GIT_AUTHOR_IDENT' above,
we could describe this as "the key that would be used to sign the
resulting commit if you were to run 'git commit' right now".  I
wonder if that is easier to reason about.

Yes, I am suggesting moving away from using "DEFAULT" in the name.

> +static char *ident_part(const char *ident, char part)

Use an enum instead of 'char part', unless you derive that single
character directly from end-user input.

> +{
> +	struct ident_split split;
> +
> +	if (!ident)
> +		return NULL;
> +	if (split_ident_line(&split, ident, strlen(ident)))
> +		return NULL;
> +
> +	switch (part) {
> +	case 'n':
> +		if (!split.name_begin || !split.name_end)
> +			return NULL;
> +		return xmemdupz(split.name_begin, split.name_end - split.name_begin);
> +	case 'e':
> +		if (!split.mail_begin || !split.mail_end)
> +			return NULL;
> +		return xmemdupz(split.mail_begin, split.mail_end - split.mail_begin);
> +	case 'd':
> +		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;

The line is getting overly long.  Aim to wrap at around ~70 columns.

> +	default:
> +		return NULL;
> +	}
> +}

> +static char *default_key(int ident_flag UNUSED)
> +{
> +	int gpgsign = 0;
> +	char *signing_key = NULL;
> +
> +	if (repo_config_get_string(the_repository, "user.signingkey", &signing_key) == 0 && signing_key && *signing_key)
> +		return signing_key;

An overly long line.

Do not make numeric comparison with 0 as "== 0" or "!= 0".

	if (!repo_config_get_string(the_repository,
				    "user.signingkey", &signing_key) &&
	    signing_key && *signing_key)
		return signing_key;

The reason why you pretend that user.signingkey is not even defined
when it is defined to be an empty string is because otherwise there
won't be a way for a user to override a key defined in per-user
configuration file in per-repository configuration file.  It may
deserve an in-code comment to explain that, or is it too obvious?

I dunno.

By the way, by using repo_config_get_string(), you are willing to
give an error message from config_error_nonbool() when the user has
user.signingkey mistakenly defined as a valueless true, i.e.,

	[user]
		name = A U Thor
		email = au@th.or
		signingkey

I think it is OK to give an error message here, to give the user a
chance to notice and fix the mistake in their configuration file,
and keep going as if the entry did not even exist.  I just wanted to
make sure we are all aware that it is what our new code is doing.

> +	free(signing_key);
> +
> +	if (repo_config_get_bool(the_repository, "commit.gpgsign", &gpgsign) == 0 && gpgsign)
> +		return get_signing_key_id();

I am not sure if this is a good idea.  Whether the "git commit"
command is told to trigger GPG signing via the configuration
variable, or via the "--gpg-sign" command line option, wouldn't the
signing key returned by get_signing_key_id() be used either way?

In other words, I can explain why the previous entry checks the
"user.signingkey" configuration variable, but I cannot give an
explanation why we check the "commit.gpgsign" configuration variable
here.

> @@ -125,10 +203,34 @@ static struct git_var git_vars[] = {
>  		.name = "GIT_COMMITTER_IDENT",
>  		.read = committer,
>  	},
> +	{
> +		.name = "GIT_COMMITTER_NAME",
> +		.read = committer_name,
> +	},
> ...

These are pretty straight-forward additions.  It shows that the
original code structure was designed for extensibility.

> @@ -172,10 +278,11 @@ static struct git_var git_vars[] = {
>  	},
>  };
>  
> -static void list_vars(void)
> +static void list_vars(int null_term)
>  {
>  	struct git_var *ptr;
>  	char *val;
> +	char eol = null_term ? '\0' : '\n';
>  
>  	for (ptr = git_vars; ptr->read; ptr++)
>  		if ((val = ptr->read(0))) {
> @@ -184,10 +291,10 @@ static void list_vars(void)
>  
>  				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);
> +					printf("%s=%s%c", ptr->name, list.items[i].string, eol);

Beware overly long lines.

> @@ -196,6 +303,8 @@ static void list_vars(void)
>  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";

We should be able to do much better than this.  Aim to stick to the
table-driven approach as much as possible.  For example, we could
add a "const char *" member to struct git_var that says it is an
alias to another key, add an entry

	{
		.name = "GIT_SIGNING_KEY",
		.alias = "GIT_DEFAULT_KEY",
		.read = default_key,
	},

there, and teach the enumerating iterator (aka list_vars) to skip an
entry that is an alias (because it would give the user redundant
information) while allowing the locating iterator (aka get_git_var)
to find it.

>  	for (ptr = git_vars; ptr->read; ptr++) {
>  		if (strcmp(var, ptr->name) == 0) {
>  			return ptr;
> @@ -219,30 +331,65 @@ int cmd_var(int argc,
>  	    const char *prefix UNUSED,
>  	    struct repository *repo UNUSED)
>  {
> -	const struct git_var *git_var;
> -	char *val;
> +	struct strvec vars = STRVEC_INIT;
> +	int list = 0;
> +	int null_term = 0;
> +	int i;
>  
>  	show_usage_if_asked(argc, argv, var_usage);
> -	if (argc != 2)
> -		usage(var_usage);
>  
> -	if (strcmp(argv[1], "-l") == 0) {
> -		repo_config(the_repository, show_config, NULL);
> -		list_vars();
> +	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;

Everything else in this loop seems sensible, but this one is iffy.
What makes us need this special casing of "--" and everything that
follows it?

> +		} else if (arg[0] == '-') {
> +			usage(var_usage);
> +		} else {
> +			strvec_push(&vars, arg);
> +		}
> +	}

Have you considered using parse_options() API instead of this
handwritten loop?  You'd only need to recognise two options '-l' and
'-z' into two booleans "int list" and "int null_termination", and
then

	if (list) {
		... barf if something remains in argc/argv[] ...
		
	} else {
		... everyhing in argc/argv[] are vars ...
	}

I do not see why we would even need a vars that is a copy of
argc/argv[] after options are parsed out.

> +	if (!vars.nr)
>  		usage(var_usage);

I do not know if this is friendly for a tool meant primarily for
scripts.  "git var foo bar" would give information about foo and
bar, "git var foo" would give informmation about foo, and it is
natural "git var" alone would give nothing.

This matters because it is plausible to have a list of vars in a
variable and then run "git var -z $variables | xargs -0 ..." to
process the vars and their values.  If you barf for an empty list,
then they need to see if they have nothing in $variable, which
becomes more than "test -z "$variable" because people often take
advantage of the fact that IFS is ignored to write their accumulator
like so

	variable=" "
	for n in ...
	do
		variable="$variable$n "
	done

	case "$variable" in
	*" "word" "*)
		: has the word
		;;
	esac

which means that $variable may not be an empty string.  It is easier
for scripts if "git var <variable>..." did not barf when the number
of <variable>s happens to be zero.

> +test_expect_success 'get multiple variables with -z' '
> +	test_tick &&
> +	printf "%s\0%s\0" "$GIT_AUTHOR_NAME" "$GIT_AUTHOR_EMAIL" >expect &&

Just a trivia, but

	printf "%s\0" "$GIT_AUTHOR_NAME "$GIT_AUTHOR_EMAIL" >expect

would be equivalent.