Re: [PATCH v2 1/2] help: use list_aliases() for alias listing and lookup

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

Jonatan Holmgren <jonatan@jontes.page> writes:

> help.c has its own get_alias() config callback that duplicates the
> parsing logic in alias.c. Consolidate by teaching list_aliases() to
> also store the alias values (via the string_list util field), then
> use it in list_all_cmds_help_aliases() instead of the private
> callback.
>
> While at it, switch git_unknown_cmd_config() from skip_prefix() to
> parse_config_key() for alias parsing, which properly handles the
> config key structure and prepares for multi-level alias config keys
> in a subsequent commit.
>
> No functional change intended.
>
> Signed-off-by: Jonatan Holmgren <jonatan@jontes.page>
> ---
>  alias.c |  4 +++-
>  help.c  | 26 ++++++++------------------
>  2 files changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/alias.c b/alias.c
> index 1a1a141a0a..c66a6095bb 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -29,7 +29,9 @@ static int config_alias_cb(const char *key, const char *value,
>  						 key, value);
>  		}
>  	} else if (data->list) {
> -		string_list_append(data->list, p);
> +		if (value)
> +			string_list_append(data->list, p)->util =
> +				xstrdup(value);
>  	}

If !value, the original still added p to data->list, but the updated
code discards p when value is not there.  Is that an intended change?

If not,

	} else if (data->list) {
		struct string_list_item *item;

                item = string_list_append(data->list, p);
		if (value)
			item->util = xstrdup(value);
	}

perhaps.

> -static int get_alias(const char *var, const char *value,
> -		     const struct config_context *ctx UNUSED, void *data)
> -{
> -	struct string_list *list = data;
> -
> -	if (skip_prefix(var, "alias.", &var)) {
> -		if (!value)
> -			return config_error_nonbool(var);
> -		string_list_append(list, var)->util = xstrdup(value);
> -	}
> -
> -	return 0;
> -}
> -

A value-less 

	[alias]
		foo

used to get an configuuration error with a friendly message from
help.c:get_alias(), which was removed.  The config_alias_cb() called
by alias.c:list_aliases() either silently ignores foo altogether
(the posted patch) or creates an entry for 'foo' but leaves its
expansion to NULL (the above "silent ignore fix").  Either way,
there needs some new code to compensate for the loss of the error
detection somehow.

> @@ -501,7 +488,7 @@ static void list_all_cmds_help_aliases(int longest)
>  	struct cmdname_help *aliases;
>  	int i;
>  
> -	repo_config(the_repository, get_alias, &alias_list);
> +	list_aliases(&alias_list);
>  	string_list_sort(&alias_list);
>  
>  	for (i = 0; i < alias_list.nr; i++) {

OK.

> @@ -586,7 +573,8 @@ static int git_unknown_cmd_config(const char *var, const char *value,
>  				  void *cb)
>  {
>  	struct help_unknown_cmd_config *cfg = cb;
> -	const char *p;
> +	const char *subsection, *key;
> +	size_t subsection_len;
>  
>  	if (!strcmp(var, "help.autocorrect")) {
>  		int v = parse_autocorrect(value);
> @@ -601,8 +589,10 @@ static int git_unknown_cmd_config(const char *var, const char *value,
>  	}
>  
>  	/* Also use aliases for command lookup */
> -	if (skip_prefix(var, "alias.", &p))
> -		add_cmdname(&cfg->aliases, p, strlen(p));
> +	if (!parse_config_key(var, "alias", &subsection, &subsection_len, &key)) {
> +		if (!subsection)
> +			add_cmdname(&cfg->aliases, key, strlen(key));
> +	}
>  
>  	return 0;
>  }

Arguably, the last two hunks are about preparing for three-level
alias.*.command support.

It is a bit unfortunate that with

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

in your configuration, 

    $ git foo.command
    $ git foo.bar

used to invoke the alias 'foo.command' and 'foo.bar' just fine, but
now with these two preparatory hunks, it no longer is the case and
they are silently ignored.  With the next patch, 'git foo' starts
working in place for 'git foo.command', but 'git foo.bar' has become
forever inaccessible.  I wonder if we want to warn about foo.bar if
not foo.command, or if it is too much?  It is conceivable that we
may add variables like alias.*.help so it may not be a great idea to
warn on anything alias.<subsection>.<key> where <key> is not "command"

Perhaps we can claim that we are fixing a bug that allowed aliases
with a dot in its name by mistake?  I dunno.  No matter what we
claim here, some people will be hit by this behaviour change and
complain about a regression X-<.