Re: [PATCH v4 3/3] alias: support non-alphanumeric names via subsection syntax

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

Jonatan Holmgren <jonatan@jontes.page> writes:

>  alias.*::
> +alias.*.command::
> +	Command aliases for the linkgit:git[1] command wrapper. Aliases
> +	can be defined using two syntaxes:
> ++
> +--
> +1. Without a subsection, e.g., `[alias] co = checkout`. The alias
> +   name is limited to ASCII alphanumeric characters and `-`,
> +   and is matched case-insensitively.

OK.  It is obvious to us that the "alias name" in the example is "co";
is it obvious enough for our first-time readers, or would we want to
do something like

	The alias name ("co", in this example) is limited to ...

to be extra clear, I wonder.

> +2. With a subsection, e.g., `[alias "name"] command = value`. The
> +   alias name can contain any characters including UTF-8, and is
> +   matched case-sensitively as raw bytes.

Unlike the previous example that is more realistic, this uses <name>
and <value> placeholders, with `command` that MUST be given verbatim
by the users.  Is that obvious enough to our first-time readers?

Thinking aloud.  How does it look with placeholder filled with
concrete values?

	... e.g., `[alias "co"] command = checkout`.  The alias name
	("co", in this example) can contain any characters ...

This does not look too bad to me.

We do not allow newlines or NULs in the subsection.  NULs may be too
obvious, but newlines might be worth mentioning.  I dunno.

> +--
> ++
> +Examples:
> ++
> +----
> +# Without subsection (ASCII alphanumeric and dash only)
> +[alias]
> +    co = checkout
> +    st = status
> +
> +# With subsection (allows any characters, including UTF-8)
> +[alias "hämta"]
> +    command = fetch
> +[alias "gömma"]
> +    command = stash
> +----

Good examples, even though I do not read Swedish ;-).

> +E.g. after defining `alias.last = cat-file commit HEAD`, the invocation
> +`git last` is equivalent to `git cat-file commit HEAD`.

This is not a new problem (it is an inherited text from before your
change), but I've always found this

	alias.last = cat-file commit HEAD

a poor thing to give to our users, as it does not match anything
they practically can use.  It is different from the valid command
line arguments to define the alias, which is

	$ git config set alias.last "cat-file commit HEAD"

and it is different from the way the result appears in the
configuration file, which is

	[alias] last = cat-file commit HEAD

Also, since the sentences are moved around, I am not sure that the
beginning "E.g." still fits there very well.  Taking them all
together, how about

    With a Git alias defined, e.g.,

	$ git config set alias.last "cat-file commit HEAD"

    you can run `git last` and it invokes `git cat-file commit
    HEAD`.

> diff --git a/alias.c b/alias.c
> index 271acb9bf1..896d0f80a4 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -17,13 +17,33 @@ static int config_alias_cb(const char *key, const char *value,
>  			   const struct config_context *ctx UNUSED, void *d)
>  {
>  	struct config_alias_data *data = d;
> -	const char *p;
> +	const char *subsection, *subkey;
> +	size_t subsection_len;

"subkey" is a confusing name for a variable.

The Synatx section in "git config --help" documentation says that a
configuration file consists of "sections and variables", and a
section can further be divided into subsections.

config.c seems to use "key" as a synonym for "variable" above, and
that is very understandable, because "section.subsection.variable"
or "section.variable" as a whole is what the users and documentation
calls a "configuration variable", and to avoid overloading the two
meanings on the same word "variable", we'd better use a different
name for that last-level thing.

Taken together, in

	[alias] co = checkout
	[alias "ci"] command = commit

it would be the best to call the parts like so:

	section: "alias"
	subsection: "ci"
	key: "co" and "command"

> -	if (!skip_prefix(key, "alias.", &p))
> +	if (parse_config_key(key, "alias", &subsection, &subsection_len,
> +			     &subkey) < 0)
> +		return 0;
> +
> +	/*
> +	 * Two config syntaxes:
> +	 * - alias.name = value   (without subsection, case-insensitive)
> +	 * - [alias "name"]
> +	 *       command = value  (with subsection, case-sensitive)
> +	 */
> +	if (subsection && strcmp(subkey, "command"))
>  		return 0;

OK.  We ignore alias.*.variable where variable is not "command".

>  	if (data->alias) {

When the caller is querying one specific alias ...

> +		int match;
> +
> +		if (subsection)
> +			match = (strlen(data->alias) == subsection_len &&
> +				 !strncmp(data->alias, subsection,
> +					  subsection_len));

... we pick either the one that literally matches the subsection
part (we have already verified that the key is "command"), or ...

> +		else
> +			match = !strcasecmp(data->alias, subkey);

... for a two-level variable, the one that matches variable name
case insensitively.  And when we see hit, ...

> +		if (match) {
>  			FREE_AND_NULL(data->v);
>  			return git_config_string(&data->v,
>  						 key, value);

... we report it to the caller.  Otherwise, when we are listing ...

> @@ -34,7 +54,11 @@ static int config_alias_cb(const char *key, const char *value,
>  		if (!value)
>  			return config_error_nonbool(key);
>  
> -		item = string_list_append(data->list, p);
> +		if (subsection)
> +			item = string_list_append_nodup(data->list,
> +				xmemdupz(subsection, subsection_len));
> +		else
> +			item = string_list_append(data->list, subkey);

... the alias name we create differs between the two- and
three-level names, but otherwise the handling is the same between
the two kinds.

>  		item->util = xstrdup(value);
>  	}

All makes sense.

> diff --git a/help.c b/help.c
> index eccd0c22f8..d7c6011780 100644
> --- a/help.c
> +++ b/help.c
> @@ -21,6 +21,7 @@
>  #include "fsmonitor-ipc.h"
>  #include "repository.h"
>  #include "alias.h"
> +#include "utf8.h"
>  
>  #ifndef NO_CURL
>  #include "git-curl-compat.h" /* For LIBCURL_VERSION only */
> @@ -108,7 +109,7 @@ static void print_command_list(const struct cmdname_help *cmds,
>  
>  	for (i = 0; cmds[i].name; i++) {
>  		if (cmds[i].category & mask) {
> -			size_t len = strlen(cmds[i].name);
> +			size_t len = utf8_strwidth(cmds[i].name);
>  			printf("   %s   ", cmds[i].name);
>  			if (longest > len)
>  				mput_char(' ', longest - len);
> @@ -492,7 +493,7 @@ static void list_all_cmds_help_aliases(int longest)
>  	string_list_sort(&alias_list);
>  
>  	for (i = 0; i < alias_list.nr; i++) {
> -		size_t len = strlen(alias_list.items[i].string);
> +		size_t len = utf8_strwidth(alias_list.items[i].string);
>  		if (longest < len)
>  			longest = len;
>  	}
> @@ -591,8 +592,15 @@ static int git_unknown_cmd_config(const char *var, const char *value,
>  	/* Also use aliases for command lookup */
>  	if (!parse_config_key(var, "alias", &subsection, &subsection_len,
>  			      &key)) {
> -		if (!subsection)
> +		if (subsection) {
> +			/* [alias "name"] command = value */
> +			if (!strcmp(key, "command"))
> +				add_cmdname(&cfg->aliases, subsection,
> +					    subsection_len);
> +		} else {
> +			/* alias.name = value */
>  			add_cmdname(&cfg->aliases, key, strlen(key));
> +		}
>  	}
>  
>  	return 0;

Looks very good.