Alban Gruin <alban.gruin@xxxxxxxxx> writes:
> +#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".
> + 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?
> + 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.
