Edmundo Carmona Antoranz <eantoranz@xxxxxxxxx> writes:
> When rebasing merge commits and dealing with conflicts, having the
> original merge commit as a reference can help us avoid some of
> them.
>
> With this patch, we leverage the original merge commit to handle the most
> obvious case:
> - HEAD tree has to match the tree of the first parent of the original merge
> commit.
> - MERGE_HEAD tree has to match the tree of the second parent of the original
> merge commit.
> - At least one tree in the merge bases of HEAD/MERGE_HEAD has to match
> a tree in the merge bases of the parent commits of the original merge
> commit.
The first two conditions look a bit too restrictive for the purpose
of reusing previous conflict resolution, while I am not sure ...
> If all of those conditions are met, we can safely use the tree of the
> original merge commit as the resulting tree of this merge that is being
> attempted at the time.
...if the "at least one" in the last condition is a safe and
sensible loosening; if it introduces a mismerge by ignoring bases
that are different from the original, then it is a bit too bold to
declare that we can safely use the tree of the original.
What was the motivating usecase behind this new feature? Was it
more about reusing the structural merge conflict resolution, or
about the textual merge conflict resolution? For the latter, after
doing the usual three-way file-level merge and seeing a conflicted
textual merge, requiring the match of the blob objects for only these
conflicted paths and taking the previous merge result may give you a
safe way to raising the chance to find more reusable merges.
> +static void load_tree_oids(struct oid_array *oids, struct commit_list *bases)
> +{
> + struct commit_list *i;
> + for (i = bases; i; i = i->next)
Using 'i' for a pointer looks novel. Don't.
> +static int find_oid(const struct object_id *oid,
> + void *data)
The result of unfolding these two lines would not be overly long, I suspect?
> +{
> + struct oid_array *other_list = (struct oid_array *) data;
> + int pos = oid_array_lookup(other_list, oid);
> + return pos >= 0 ? 1 : 0;
That's an unusual way to say
return pos >= 0;
or even
return 0 <= oid_array_lookup(other_list, oid);
without otherwise unused variable.
> +static int run_tms_merge(struct tms_options *options)
> +{
> + struct commit *head, *merge_head, *tip;
> + struct commit_list *i;
> +
> + head = lookup_commit_reference_by_name("HEAD");
> + merge_head = lookup_commit_reference_by_name(options->merge_head);
> + tip = lookup_commit_reference_by_name(options->tip);
> +
> + if (!(head && merge_head && tip)) {
> + return 2;
> + }
Unnecessary {} around a single statement block.
> + if (commit_list_count(tip->parents) != 2)
> + return 2;
> +
> + for (i = tip->parents; i; i = i->next)
> + parse_commit(i->item);
> + if (!oideq(get_commit_tree_oid(head),
> + get_commit_tree_oid(tip->parents->item)))
> + return 2;
> + if (!oideq(get_commit_tree_oid(merge_head),
> + get_commit_tree_oid(tip->parents->next->item)))
> + return 2;
> +
> + if (!base_match(tip, head, merge_head))
> + return 2;
> +
> + if (restore(tip))
> + return 2;
I somehow thought that reverting the trashed working tree and the
index to their original state was not the responsibility for a merge
strategy but for the caller? Shouldn't this restoration be on the
caller's side?
Oh, has this code even touched anything in the working tree and the
index at this point? It looks more like everything we did above in
order to punt by returning 2 was to see if the condition for us to
reuse the resulting tree holds and nothing else.
Ah, "restore()" is misnamed, perhaps? I thought it was about "oh,
we made a mess and need to go back to the state that was given to us
before failing", but is this the real "ok, the condition holds and
we can just reuse the tree from the previous merge"? Then it makes
sense for the code to attempt to check out that tree and return 2
when it fails. Only the function name was misleading.
> + if (!opts->strategy || !strcmp(opts->strategy, "tms")) {
> + rollback_lock_file(&lock);
> + ret = try_tms_merge(opts, commit, to_merge->item);
> + if (ret) {
> + discard_index(r->index);
> + if (repo_read_index(r) < 0) {
> + ret = error(_("could not read index"));
> + goto leave_merge;
> + }
> + goto ran_merge;
> + }
> + // regain lock to go into recursive
No // comments here.
> + if (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
