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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 10, 2026, 23:17 UTC
Message-ID: <xmqqv7g4w5mk.fsf@gitster.g>
In-Reply-To: <20260210222745.78575-2-jonatan@jontes.page>

Jonatan Holmgren <jonatan@jontes.page> writes:

Show 8 quoted lines

>  	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.

Show 8 quoted lines

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

Show 32 quoted lines

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

Show 9 quoted lines

>  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++) {