From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mon, 18 May 2015 18:56:00 GMT
Subject: Re: [PATCH 05/14] pull: error on no merge candidates
Message-ID: <5c9746a0a2ce12b200c472bc534fd770@www.dscho.org>
In-Reply-To: <1431961571-20370-6-git-send-email-pyokagan@gmail.com>

Hi Paul,

On 2015-05-18 17:06, Paul Tan wrote:
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 07ad783..8982fdf 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -207,6 +209,130 @@ static void argv_push_force(struct argv_array *arr)
>  		argv_array_push(arr, "-f");
>  }
>  
> +struct known_remote {
> +	struct known_remote *next;
> +	struct remote *remote;
> +};
> +
> +/**
> + * Use this callback with for_each_remote() to get the configured remotes as
> + * a singly linked known_remote list. cb_data must be a pointer to a
> + * struct known_remote*, which must be initialized to NULL. For example,
> + * For example:
> + *
> + *     struct known_remote *list = NULL;
> + *     for_each_remote(add_known_remote, &list);
> + */
> +static int add_known_remote(struct remote *remote, void *cb_data)
> +{
> +	struct known_remote **list = cb_data;
> +	struct known_remote *item;
> +
> +	item = xmalloc(sizeof(*item));
> +	item->remote = remote;
> +	item->next = *list;
> +	*list = item;
> +	return 0;
> +}

My first reaction to this was: let's use an array and `ALLOC_GROW()` to make this look nicer. But then I saw that there is only one user:

> +static void NORETURN die_no_merge_candidates(const char *repo, const
> char **refspecs)
> +{
> [...]
> +	} else if (!curr_branch->merge_nr) {
> +		struct known_remote *remotes = NULL;
> +		const char *remote_name = "<remote>";
> +
> +		for_each_remote(add_known_remote, &remotes);
> +		if (remotes && !remotes->next)
> +			remote_name = remotes->remote->name;
> +
> [...]

How about this instead:

static int get_only_remote(struct remote *remote, void *cb_data)
{
	const char **p = cb_data;

	if (*p)
		return -1;

	*p = remote->name;
	return 0;
}

[...]
		const char *remote_name = NULL;

		if (for_each_remote(get_only_remote, &remote_name) || !remote_name)
			remote_name = "<remote>";

Ciao,
Dscho