Re: [PATCH v3 05/11] merge-resolve: rewrite in C

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 16, 2020, 19:19 UTC
Message-ID: <xmqqr1pyashe.fsf@gitster.c.googlers.com>
In-Reply-To: <20201005122646.27994-6-alban.gruin@gmail.com>

Alban Gruin <alban.gruin@gmail.com> writes:

Show 23 quoted lines

> +#include "cache.h"
> +#include "builtin.h"
> +#include "merge-strategies.h"
> +
> +static const char builtin_merge_resolve_usage[] =
> +	"git merge-resolve <bases>... -- <head> <remote>";
> +
> +int cmd_merge_resolve(int argc, const char **argv, const char *prefix)
> +{
> +	int i, is_baseless = 1, sep_seen = 0;
> +	const char *head = NULL;
> +	struct commit_list *bases = NULL, *remote = NULL;
> +	struct commit_list **next_base = &bases;
> +
> +	if (argc < 5)
> +		usage(builtin_merge_resolve_usage);
> +
> +	setup_work_tree();
> +	if (repo_read_index(the_repository) < 0)
> +		die("invalid index");
> +
> +	/* The first parameters up to -- are merge bases; the rest are
> +	 * heads. */

Style (I won't repeat).

> +	for (i = 1; i < argc; i++) {
> +		if (strcmp(argv[i], "--") == 0)

	if (!strcmp(...))

is more typical than comparing with "== 0".

Show 16 quoted lines

> +			sep_seen = 1;
> +		else if (strcmp(argv[i], "-h") == 0)
> +			usage(builtin_merge_resolve_usage);
> +		else if (sep_seen && !head)
> +			head = argv[i];
> +		else if (remote) {
> +			/* Give up if we are given two or more remotes.
> +			 * Not handling octopus. */
> +			return 2;
> +		} else {
> +			struct object_id oid;
> +
> +			get_oid(argv[i], &oid);
> +			is_baseless &= sep_seen;
> +
> +			if (!oideq(&oid, the_hash_algo->empty_tree)) {

What is this business about an empty tree about?

Show 14 quoted lines

> +				struct commit *commit;
> +				commit = lookup_commit_or_die(&oid, argv[i]);
> +
> +				if (sep_seen)
> +					commit_list_append(commit, &remote);
> +				else
> +					next_base = commit_list_append(commit, next_base);
> +			}
> +		}
> +	}
> +
> +	/* Give up if this is a baseless merge. */
> +	if (is_baseless)
> +		return 2;

This is quite convoluted.

The original is much more straight-forward. We just said "grab everything before we see '--' and call them bases; immediately after '--' is HEAD and everything else is remote. Now do we have any base? Otherwise we cannot handle it".

I cannot see an equivalence to it in the rewritten result, with the bit operation with is_baseless and sep_seen. Wouldn't it be the matter of checking if next_base is NULL, or is there something more subtle that deserves in-code comment going on?

Thanks.