From b0ca120554be8d8f7faf98aa3b991441de76f5cf Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 16 Dec 2020 22:27:59 +0000 Subject: [PATCH 1/4] commit: move reverse_commit_list() from merge-recursive Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- commit.c | 11 +++++++++++ commit.h | 3 +++ merge-recursive.c | 11 ----------- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/commit.c b/commit.c index f53429c0ac..dc08a47b07 100644 --- a/commit.c +++ b/commit.c @@ -563,6 +563,17 @@ struct commit_list *copy_commit_list(struct commit_list *list) return head; } +struct commit_list *reverse_commit_list(struct commit_list *list) +{ + struct commit_list *next = NULL, *current, *backup; + for (current = list; current; current = backup) { + backup = current->next; + current->next = next; + next = current; + } + return next; +} + void free_commit_list(struct commit_list *list) { while (list) diff --git a/commit.h b/commit.h index 5467786c7b..3e9139a000 100644 --- a/commit.h +++ b/commit.h @@ -177,6 +177,9 @@ void commit_list_sort_by_date(struct commit_list **list); /* Shallow copy of the input list */ struct commit_list *copy_commit_list(struct commit_list *list); +/* Modify list in-place to reverse it, returning new head; list will be tail */ +struct commit_list *reverse_commit_list(struct commit_list *list); + void free_commit_list(struct commit_list *list); struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */ diff --git a/merge-recursive.c b/merge-recursive.c index f736a0f632..b052974f19 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3517,17 +3517,6 @@ static int merge_trees_internal(struct merge_options *opt, return clean; } -static struct commit_list *reverse_commit_list(struct commit_list *list) -{ - struct commit_list *next = NULL, *current, *backup; - for (current = list; current; current = backup) { - backup = current->next; - current->next = next; - next = current; - } - return next; -} - /* * Merge the commits h1 and h2, returning a flag (int) indicating the * cleanness of the merge. Also, if opt->priv->call_depth, create a From 4296d8f17d45465c9712fab099a1a6787a8e9913 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 16 Dec 2020 22:28:00 +0000 Subject: [PATCH 2/4] merge-ort: copy a few small helper functions from merge-recursive.c In a subsequent commit, we will implement the traditional recursiveness that gave merge-recursive its name, namely merging non-unique merge-bases to come up with a single virtual merge base. Copy a few helper functions from merge-recursive.c that we will use in the implementation. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-ort.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/merge-ort.c b/merge-ort.c index 414e7b7eea..6eac0cef49 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -17,8 +17,10 @@ #include "cache.h" #include "merge-ort.h" +#include "alloc.h" #include "blob.h" #include "cache-tree.h" +#include "commit.h" #include "commit-reach.h" #include "diff.h" #include "diffcore.h" @@ -1348,6 +1350,24 @@ void merge_finalize(struct merge_options *opt, /*** Function Grouping: helper functions for merge_incore_*() ***/ +static inline void set_commit_tree(struct commit *c, struct tree *t) +{ + c->maybe_tree = t; +} + +MAYBE_UNUSED +static struct commit *make_virtual_commit(struct repository *repo, + struct tree *tree, + const char *comment) +{ + struct commit *commit = alloc_commit_node(repo); + + set_merge_remote_desc(commit, comment, (struct object *)commit); + set_commit_tree(commit, tree); + commit->object.parsed = 1; + return commit; +} + static void merge_start(struct merge_options *opt, struct merge_result *result) { /* Sanity checks on opt */ From 43e9c4eeccc069dbe6ca8a65dc5d0093b46acc03 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 16 Dec 2020 22:28:01 +0000 Subject: [PATCH 3/4] merge-ort: make clear_internal_opts() aware of partial clearing In order to handle recursive merges, after merging merge-bases we need to clear away most of the data we had built up but some of it needs to be kept -- in particular the "output" field. Rename the function to reflect its future change in use. Further, since "reinitialize" means we'll be reusing the fields immediately, take advantage of this to only partially clear maps, leaving the hashtable allocated and pre-sized. (This may be slightly out-of-order since the speedups aren't realized until there are far more strmaps in use, but the patch submission process already went out of order because of various questions and requests for strmap. Anyway, see commit 6ccdfc2a20 ("strmap: enable faster clearing and reusing of strmaps", 2020-11-05), for performance details about the use of strmap_partial_clear().) Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-ort.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index 6eac0cef49..bd237f3472 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -253,10 +253,11 @@ static void free_strmap_strings(struct strmap *map) } } -static void clear_internal_opts(struct merge_options_internal *opti, - int reinitialize) +static void clear_or_reinit_internal_opts(struct merge_options_internal *opti, + int reinitialize) { - assert(!reinitialize); + void (*strmap_func)(struct strmap *, int) = + reinitialize ? strmap_partial_clear : strmap_clear; /* * We marked opti->paths with strdup_strings = 0, so that we @@ -266,14 +267,14 @@ static void clear_internal_opts(struct merge_options_internal *opti, * to deallocate them. */ free_strmap_strings(&opti->paths); - strmap_clear(&opti->paths, 1); + strmap_func(&opti->paths, 1); /* * All keys and values in opti->conflicted are a subset of those in * opti->paths. We don't want to deallocate anything twice, so we * don't free the keys and we pass 0 for free_values. */ - strmap_clear(&opti->conflicted, 0); + strmap_func(&opti->conflicted, 0); /* * opti->paths_to_free is similar to opti->paths; we created it with @@ -1344,7 +1345,7 @@ void merge_finalize(struct merge_options *opt, assert(opt->priv == NULL); - clear_internal_opts(opti, 0); + clear_or_reinit_internal_opts(opti, 0); FREE_AND_NULL(opti); } From 8119214f4e7036cef01a8e13f161ec510b3ff710 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 16 Dec 2020 22:28:02 +0000 Subject: [PATCH 4/4] merge-ort: implement merge_incore_recursive() Implement merge_incore_recursive(), mostly through the use of a new helper function, merge_ort_internal(), which itself is based off merge_recursive_internal() from merge-recursive.c. This drops the number of failures in the testsuite when run under GIT_TEST_MERGE_ALGORITHM=ort from around 1500 to 647. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-ort.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++-- merge-ort.h | 10 ++++++ 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index bd237f3472..31103d2140 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -1356,7 +1356,6 @@ static inline void set_commit_tree(struct commit *c, struct tree *t) c->maybe_tree = t; } -MAYBE_UNUSED static struct commit *make_virtual_commit(struct repository *repo, struct tree *tree, const char *comment) @@ -1466,6 +1465,89 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt, } } +/* + * Originally from merge_recursive_internal(); somewhat adapted, though. + */ +static void merge_ort_internal(struct merge_options *opt, + struct commit_list *merge_bases, + struct commit *h1, + struct commit *h2, + struct merge_result *result) +{ + struct commit_list *iter; + struct commit *merged_merge_bases; + const char *ancestor_name; + struct strbuf merge_base_abbrev = STRBUF_INIT; + + if (!merge_bases) { + merge_bases = get_merge_bases(h1, h2); + /* See merge-ort.h:merge_incore_recursive() declaration NOTE */ + merge_bases = reverse_commit_list(merge_bases); + } + + merged_merge_bases = pop_commit(&merge_bases); + if (merged_merge_bases == NULL) { + /* if there is no common ancestor, use an empty tree */ + struct tree *tree; + + tree = lookup_tree(opt->repo, opt->repo->hash_algo->empty_tree); + merged_merge_bases = make_virtual_commit(opt->repo, tree, + "ancestor"); + ancestor_name = "empty tree"; + } else if (merge_bases) { + ancestor_name = "merged common ancestors"; + } else { + strbuf_add_unique_abbrev(&merge_base_abbrev, + &merged_merge_bases->object.oid, + DEFAULT_ABBREV); + ancestor_name = merge_base_abbrev.buf; + } + + for (iter = merge_bases; iter; iter = iter->next) { + const char *saved_b1, *saved_b2; + struct commit *prev = merged_merge_bases; + + opt->priv->call_depth++; + /* + * When the merge fails, the result contains files + * with conflict markers. The cleanness flag is + * ignored (unless indicating an error), it was never + * actually used, as result of merge_trees has always + * overwritten it: the committed "conflicts" were + * already resolved. + */ + saved_b1 = opt->branch1; + saved_b2 = opt->branch2; + opt->branch1 = "Temporary merge branch 1"; + opt->branch2 = "Temporary merge branch 2"; + merge_ort_internal(opt, NULL, prev, iter->item, result); + if (result->clean < 0) + return; + opt->branch1 = saved_b1; + opt->branch2 = saved_b2; + opt->priv->call_depth--; + + merged_merge_bases = make_virtual_commit(opt->repo, + result->tree, + "merged tree"); + commit_list_insert(prev, &merged_merge_bases->parents); + commit_list_insert(iter->item, + &merged_merge_bases->parents->next); + + clear_or_reinit_internal_opts(opt->priv, 1); + } + + opt->ancestor = ancestor_name; + merge_ort_nonrecursive_internal(opt, + repo_get_commit_tree(opt->repo, + merged_merge_bases), + repo_get_commit_tree(opt->repo, h1), + repo_get_commit_tree(opt->repo, h2), + result); + strbuf_release(&merge_base_abbrev); + opt->ancestor = NULL; /* avoid accidental re-use of opt->ancestor */ +} + void merge_incore_nonrecursive(struct merge_options *opt, struct tree *merge_base, struct tree *side1, @@ -1483,5 +1565,9 @@ void merge_incore_recursive(struct merge_options *opt, struct commit *side2, struct merge_result *result) { - die("Not yet implemented"); + /* We set the ancestor label based on the merge_bases */ + assert(opt->ancestor == NULL); + + merge_start(opt, result); + merge_ort_internal(opt, merge_bases, side1, side2, result); } diff --git a/merge-ort.h b/merge-ort.h index 55ae7ee865..d53a0a339f 100644 --- a/merge-ort.h +++ b/merge-ort.h @@ -34,6 +34,16 @@ struct merge_result { /* * rename-detecting three-way merge with recursive ancestor consolidation. * working tree and index are untouched. + * + * merge_bases will be consumed (emptied) so make a copy if you need it. + * + * NOTE: empirically, the recursive algorithm will perform better if you + * pass the merge_bases in the order of oldest commit to the + * newest[1][2]. + * + * [1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.1907252055500.21907@tvgsbejvaqbjf.bet/ + * [2] commit 8918b0c9c2 ("merge-recur: try to merge older merge bases + * first", 2006-08-09) */ void merge_incore_recursive(struct merge_options *opt, struct commit_list *merge_bases,