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

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

Jonatan Holmgren <jonatan@jontes.page> writes:

>  	if (data->alias) {
>  		if (!strcasecmp(p, data->alias)) {
> +			if (!value)
> +				return config_error_nonbool(key);
>  			FREE_AND_NULL(data->v);
>  			return git_config_string(&data->v,
>  						 key, value);
>  		}

Hmph, git_config_string() would trigger config_error_nonbool()
anyway if your feed value==NULL, so this change looks a noop.

>  	} else if (data->list) {
> -		string_list_append(data->list, p);
> +		struct string_list_item *item;
> +
> +		item = string_list_append(data->list, p);
> +		if (value)
> +			item->util = xstrdup(value);
> +		/* if !value, item->util remains NULL but item is still added */

This side is silent.  We hold onto the value, when available, and
otherwise we just remember the fact that there is a (misconfigured)
alias by keeping the NULL in the .util member.  Presumably it is now
the responsibility of the caller to deal with these entries with
NULL in their .util member?

>  	}
>  
>  	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;
> -}

We used to use this callback when listing aliases, which (1) added
an alias with proper value to the list, and (2) reported a
misconfigured variable without adding it to the list.  So the net
effect was that the user got diagnosis necessary to fix their
configuration file, while the caller did not have to worry about
getting a broken entry appended to the list.

>  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);

We call exactly the same alias.c:list_aliases(), which does not feed
data->alias at all, so we will take the "else if (data->list)"
codepath there.  Now we have these broken entries in the returned
list.  Because the shared callback did not give any diagnosis message,
it is on up to us to do so, right?

Perhaps in the code that begins in the post-context of this hunk, here...

	for (i = 0; i < alias_list.nr; i++) {
		if (alias_list.items[i].util)
			continue;
		give error equivanent to config_error_nonbool();
		release resources held by alias_list.items[i];
		shift alias_list.items[i+1..alias_list.nr] by one;
		i-- to compensate for the shift of the array;
	}

or something?

Have you considered doing the config_error_nonbool(key) on the
data->list side of the if/else inside alias.c:config_alias_cb(),
just like help.c:get_alias() callback used to do?

I haven't stared at this code as long as you have, so it is very
possible I am missing the reason why that code path wants to be
silent, though.  But if we can do so, then this caller does not have
to worry about having to handle broken entries at all.

Thanks.

>  	string_list_sort(&alias_list);
>  
>  	for (i = 0; i < alias_list.nr; i++) {