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

"Andrew Pleeter via GitGitGadget" [off-list ref] 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.

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?

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

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.