Re: [PATCH v6] 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 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/ (local) 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).

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.