Hi Alban,
On Wed, 17 Mar 2021, Alban Gruin wrote:
> diff --git a/merge-strategies.c b/merge-strategies.c
> index 2717af51fd..a51700dae5 100644
> --- a/merge-strategies.c
> +++ b/merge-strategies.c
> @@ -272,3 +275,95 @@ int merge_all_index(struct index_state *istate, int oneshot, int quiet,
>
> return err;
> }
> +
> +static int fast_forward(struct repository *r, struct tree_desc *t,
> + int nr, int aggressive)
> +{
> + struct unpack_trees_options opts;
> + struct lock_file lock = LOCK_INIT;
> +
> + refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
> + repo_hold_locked_index(r, &lock, LOCK_DIE_ON_ERROR);
Shouldn't we lock the index first, and _then_ refresh it? I guess not,
seeing as we don't do that either in `cmd_status()`: there, we also
refresh the index and _then_ lock it.
> +
> + memset(&opts, 0, sizeof(opts));
> + opts.head_idx = 1;
> + opts.src_index = r->index;
> + opts.dst_index = r->index;
> + opts.merge = 1;
> + opts.update = 1;
> + opts.aggressive = aggressive;
> +
> + if (nr == 1)
> + opts.fn = oneway_merge;
> + else if (nr == 2) {
> + opts.fn = twoway_merge;
> + opts.initial_checkout = is_index_unborn(r->index);
> + } else if (nr >= 3) {
> + opts.fn = threeway_merge;
> + opts.head_idx = nr - 1;
> + }
Given the function's name `fast_forward()`, I have to admit that I
somewhat stumbled over these merges.
> +
> + if (unpack_trees(nr, t, &opts))
> + return -1;
> +
> + if (write_locked_index(r->index, &lock, COMMIT_LOCK))
> + return error(_("unable to write new index file"));
> +
> + return 0;
> +}
> +
> +static int add_tree(struct tree *tree, struct tree_desc *t)
> +{
> + if (parse_tree(tree))
> + return -1;
> +
> + init_tree_desc(t, tree->buffer, tree->size);
> + return 0;
> +}
This is a really trivial helper, but it is used a couple times below, so
it makes sense to have it encapsulated in a separate function.
> +
> +int merge_strategies_resolve(struct repository *r,
> + struct commit_list *bases, const char *head_arg,
> + struct commit_list *remote)
Since it is a list, and since the original variable in the shell script
had been named in the plural form, let's do the same here: `remotes`.
> +{
> + struct tree_desc t[MAX_UNPACK_TREES];
> + struct object_id head, oid;
> + struct commit_list *i;
> + int nr = 0;
> +
> + if (head_arg)
> + get_oid(head_arg, &head);
> +
> + puts(_("Trying simple merge."));
Good. Usually I would recommend to print this to `stderr`, but the
original script prints it to `stdout`, so we should do that here, too.
> +
> + for (i = bases; i && i->item; i = i->next) {
> + if (add_tree(repo_get_commit_tree(r, i->item), t + (nr++)))
> + return 2;
Since we're talking about a library function, not a `cmd_*()` function,
the return value on error should probably be negative.
Even better would be to let the function return an `enum` that contains
labels with more intuitive meaning than "2".
It _is_ the expected exit code when calling `git merge-resolve`, of course
(because of the `|| exit 2` after that `read-tree` call), but I wonder
whether a better layer for that `2` would be the `cmd_merge_resolve()`
function, letting `merge_strategies_resolve()` report failures in a more
fine-grained fashion.
> + }
> +
> + if (head_arg) {
It would probably be easier to read if the `if (head_arg)` clause above
was merged into this here clause.
> + struct tree *tree = parse_tree_indirect(&head);
> + if (add_tree(tree, t + (nr++)))
> + return 2;
> + }
> +
> + if (remote && add_tree(repo_get_commit_tree(r, remote->item), t + (nr++)))
> + return 2;
You get away with assuming that `remotes` only contains at most a single
entry because `cmd_merge_resolve()` verified it.
However, as the intention is to use this as a library function, I think
the input validation needs to be moved here instead of relying on all
callers to verify that they send at most one "remote" ref.
Other than that, this patch looks good to me.
Thanks,
Dscho
> +
> + if (fast_forward(r, t, nr, 1))
> + return 2;
> +
> + if (write_index_as_tree(&oid, r->index, r->index_file,
> + WRITE_TREE_SILENT, NULL)) {
> + int ret;
> + struct lock_file lock = LOCK_INIT;
> +
> + puts(_("Simple merge failed, trying Automatic merge."));
> + repo_hold_locked_index(r, &lock, LOCK_DIE_ON_ERROR);
> + ret = merge_all_index(r->index, 1, 0, merge_one_file_func, NULL);
> +
> + write_locked_index(r->index, &lock, COMMIT_LOCK);
> + return !!ret;
> + }
> +
> + return 0;
> +}
> diff --git a/merge-strategies.h b/merge-strategies.h
> index 8705a550ca..bba4bf999c 100644
> --- a/merge-strategies.h
> +++ b/merge-strategies.h
> @@ -1,6 +1,7 @@
> #ifndef MERGE_STRATEGIES_H
> #define MERGE_STRATEGIES_H
>
> +#include "commit.h"
> #include "object.h"
>
> int merge_three_way(struct index_state *istate,
> @@ -28,4 +29,8 @@ int merge_index_path(struct index_state *istate, int oneshot, int quiet,
> int merge_all_index(struct index_state *istate, int oneshot, int quiet,
> merge_fn fn, void *data);
>
> +int merge_strategies_resolve(struct repository *r,
> + struct commit_list *bases, const char *head_arg,
> + struct commit_list *remote);
> +
> #endif /* MERGE_STRATEGIES_H */
> --
> 2.31.0
>
>
