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

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

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

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

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.

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

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.

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

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

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.

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.

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

Beware overly long lines.

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.

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?

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.

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.

Just a trivia, but

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

would be equivalent.