Re: [PATCH v2 2/2] alias: support non-alphanumeric names via subsection syntax

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

Jonatan Holmgren <jonatan@jontes.page> writes:

> Git alias names are limited to alphanumeric characters and dashes
> because config variable names are validated by iskeychar(). This

"ASCII alphanumeric", perhaps, as accented alphabet characters are
still alphanumeric ;-).  "because aliases are implemented as config
variable names" is probably an explanation that talks to readers in
terms closer to them (iskeychar() function is an implementation detail).

> prevents non-English speakers from creating aliases in their native
> languages.

True.

> Add support for arbitrary alias names by using config subsections:
>
>     [alias "förgrena"]
>         command = branch
>
> The subsection name is matched as-is (case-sensitive byte comparison),
> while the existing definition without a subsection (e.g.,
> "[alias] co = checkout") remains case-insensitive for backward
> compatibility. This uses existing config infrastructure since
> subsections already support arbitrary bytes, and avoids introducing
> Unicode normalization.
>
> Also teach the help subsystem about the new syntax so that "git help
> -a" properly lists subsection aliases and the autocorrect feature can
> suggest them. Use utf8_strwidth() instead of strlen() for column
> alignment so that non-alphanumeric alias names display correctly.

Either move the last two hunks from [1/2] to this step, or make it a
separate patch [1.5/2] between this and the other steps, and explain
it as a change that breaks compatibility in a way that hopefully
would not affect anybody in practice.  The alias configuration
parser used to be overly loose and took "alias.<subsection>.<key>"
as defining an alias "<subsection>.<key>"; that change tightens the
parser and alias.<subsection>.<key> are silently ignored.  This step
then take alias.<subsection>.command to be defining a new-style
alias that can be invoked as "<subsection>", which is case sensitive
and is not limited to ASCII alphanumeric and dashes.

> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Jonatan Holmgren <jonatan@jontes.page>
> ---
>  Documentation/config/alias.adoc | 44 +++++++++++++++++++++-----
>  alias.c                         | 45 ++++++++++++++++++++++-----
>  help.c                          | 12 +++++--
>  t/t0014-alias.sh                | 55 +++++++++++++++++++++++++++++++++
>  4 files changed, 137 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/config/alias.adoc b/Documentation/config/alias.adoc
> index 80ce17d2de..17a548cd64 100644
> --- a/Documentation/config/alias.adoc
> +++ b/Documentation/config/alias.adoc
> @@ -1,12 +1,40 @@
>  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 alphanumeric characters and `-` (the same
> +   limitation as configuration variable names), and is matched
> +   case-insensitively.
> +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.
> +--
> ++
> +Examples:
> ++
> +----
> +# Without subsection
> +[alias]
> +    co = checkout
> +    st = status
> +
> +# With subsection (allows UTF-8 and special characters)
> +[alias "hämta"]
> +    command = fetch
> +[alias "gömma"]
> +    command = stash
> +----
> ++
> +E.g. after defining `alias.last = cat-file commit HEAD`, the invocation
> +`git last` is equivalent to `git cat-file commit HEAD`. To avoid
> +confusion and troubles with script usage, aliases that
> +hide existing Git commands are ignored except for deprecated
> +commands.  Arguments are split by
> +spaces, the usual shell quoting and escaping are supported.
> +A quote pair or a backslash can be used to quote them.
>  +
>  Note that the first word of an alias does not necessarily have to be a
>  command. It can be a command-line option that will be passed into the
> diff --git a/alias.c b/alias.c
> index c66a6095bb..cfd313ce5d 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -17,21 +17,50 @@ 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;
>  
> -	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) {
> +		if (strcmp(subkey, "command"))
> +			return 0;

This silently ignores

	[alias "foo"]
		bar = !date

which may or may not be a feature.  If the variable name is "help"
instead of "bar", it certainly is a feature to silently skip it, as
it is not inconceivable that we would add such a variable name in
the future, and because we won't be able to predict the future, not
limiting us to "help" but ignoring anything we do not understand
like this code does may probably be a good thing.  I dunno.

> +	}
> +
>  	if (data->alias) {
> -		if (!strcasecmp(p, data->alias)) {
> +		int match;
> +
> +		if (subsection)
> +			match = (strlen(data->alias) == subsection_len &&
> +				 !strncmp(data->alias, subsection,
> +					  subsection_len));
> +		else
> +			match = !strcasecmp(data->alias, subkey);
> +
> +		if (match) {
>  			FREE_AND_NULL(data->v);
> -			return git_config_string(&data->v,
> -						 key, value);
> +			return git_config_string(&data->v, key, value);
>  		}
>  	} else if (data->list) {
> -		if (value)
> -			string_list_append(data->list, p)->util =
> -				xstrdup(value);
> +		struct string_list_item *item;
> +
> +		if (!value)
> +			return 0;
> +
> +		if (subsection)
> +			item = string_list_append_nodup(data->list,
> +				xmemdupz(subsection, subsection_len));
> +		else
> +			item = string_list_append(data->list, subkey);
> +		item->util = xstrdup(value);

This still silently ignores

	[alias "foo"]
		command

which is a much more grave problem than ignoring alias.foo.bar in
the earlier part of this function.  We would want to preserve the
existing diagnosis on broken configuration.

> @@ -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;
>  	}
> @@ -590,8 +591,13 @@ 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) {
> +			if (!strcmp(key, "command"))
> +				add_cmdname(&cfg->aliases, subsection,
> +					    subsection_len);
> +		} else {
>  			add_cmdname(&cfg->aliases, key, strlen(key));
> +		}
>  	}

OK.  Alternatively, out of

	[alias "foo"]
		command = !echo foo
		bar = !echo bar

we _could_ list "foo" (a new style alias) and "foo.bar" (an old
style alias that we have been accepting forever by mistake) for
maximum backward compatibility.  I am still undecided if it is a
good idea.

Thanks.