Re: [PATCH v4 1/3] help: use list_aliases() for alias listing

[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.
>
> This preserves the existing error checking for value-less alias
> definitions by checking in alias.c rather than help.c.
>
> No functional change intended.
>
> Signed-off-by: Jonatan Holmgren <jonatan@jontes.page>
> ---
>  alias.c          |  8 +++++++-
>  help.c           | 17 ++---------------
>  t/t0014-alias.sh | 10 ++++++++++
>  3 files changed, 19 insertions(+), 16 deletions(-)

Looks good.  There is a small functional change not on the
help.c:get_alias() side (i.e., "git help --all") but on the
alias.c:list_aliases() side.  "git --list-cmds=alias", used by
command line completion, used to include such a broken alias, but it
no longer does (and gets an error).

I think it is fine to call it a bugfix ;-)


> diff --git a/alias.c b/alias.c
> index 1a1a141a0a..271acb9bf1 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -29,7 +29,13 @@ static int config_alias_cb(const char *key, const char *value,
>  						 key, value);
>  		}
>  	} else if (data->list) {
> -		string_list_append(data->list, p);
> +		struct string_list_item *item;
> +
> +		if (!value)
> +			return config_error_nonbool(key);
> +
> +		item = string_list_append(data->list, p);
> +		item->util = xstrdup(value);
>  	}
>  
>  	return 0;
> diff --git a/help.c b/help.c
> index fefd811f7a..0bdb7ca10f 100644
> --- a/help.c
> +++ b/help.c
> @@ -20,6 +20,7 @@
>  #include "prompt.h"
>  #include "fsmonitor-ipc.h"
>  #include "repository.h"
> +#include "alias.h"
>  
>  #ifndef NO_CURL
>  #include "git-curl-compat.h" /* For LIBCURL_VERSION only */
> @@ -468,20 +469,6 @@ void list_developer_interfaces_help(void)
>  	putchar('\n');
>  }
>  
> -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;
> -}
> -
>  static void list_all_cmds_help_external_commands(void)
>  {
>  	struct string_list others = STRING_LIST_INIT_DUP;
> @@ -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++) {
> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
> index 07a53e7366..a13d2be8ca 100755
> --- a/t/t0014-alias.sh
> +++ b/t/t0014-alias.sh
> @@ -112,4 +112,14 @@ test_expect_success 'cannot alias-shadow a sample of regular builtins' '
>  	done
>  '
>  
> +test_expect_success 'alias without value reports error' '
> +	test_when_finished "git config --unset alias.noval" &&
> +	cat >>.git/config <<-\EOF &&
> +	[alias]
> +		noval
> +	EOF
> +	test_must_fail git noval 2>error &&
> +	test_grep "alias.noval" error
> +'
> +
>  test_done