| Message ID | 9f73e54224d55b40faeb5d68ebd7ff0c13d69c7b.1741362522.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Small new merge-ort features, prepping for deletion of merge-recursive.[ch] | expand |
On Fri, Mar 07, 2025 at 03:48:40PM +0000, Elijah Newren via GitGitGadget wrote: > diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c > index d6f61359965..62834c30e9e 100644 > --- a/merge-ort-wrappers.c > +++ b/merge-ort-wrappers.c > @@ -64,3 +68,63 @@ int merge_ort_recursive(struct merge_options *opt, > > return tmp.clean; > } > + > +static struct commit *get_ref(struct repository *repo, > + const struct object_id *oid, > + const char *name) > +{ > + struct object *object; > + > + object = deref_tag(repo, parse_object(repo, oid), > + name, strlen(name)); > + if (!object) > + return NULL; > + if (object->type == OBJ_TREE) > + return make_virtual_commit(repo, (struct tree*)object, name); > + if (object->type != OBJ_COMMIT) > + return NULL; > + if (repo_parse_commit(repo, (struct commit *)object)) > + return NULL; > + return (struct commit *)object; > +} This is an exact copy of the same function in "merge-recursive.c". > +int merge_ort_generic(struct merge_options *opt, > + const struct object_id *head, > + const struct object_id *merge, > + int num_merge_bases, > + const struct object_id *merge_bases, > + struct commit **result) > +{ > + int clean; > + struct lock_file lock = LOCK_INIT; > + struct commit *head_commit = get_ref(opt->repo, head, opt->branch1); > + struct commit *next_commit = get_ref(opt->repo, merge, opt->branch2); > + struct commit_list *ca = NULL; > + > + if (merge_bases) { > + int i; > + for (i = 0; i < num_merge_bases; ++i) { > + struct commit *base; > + if (!(base = get_ref(opt->repo, &merge_bases[i], > + oid_to_hex(&merge_bases[i])))) > + return error(_("Could not parse object '%s'"), > + oid_to_hex(&merge_bases[i])); > + commit_list_insert(base, &ca); > + } > + } > + > + repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR); > + clean = merge_ort_recursive(opt, head_commit, next_commit, ca, > + result); > + free_commit_list(ca); > + if (clean < 0) { > + rollback_lock_file(&lock); > + return clean; > + } > + > + if (write_locked_index(opt->repo->index, &lock, > + COMMIT_LOCK | SKIP_IF_UNCHANGED)) > + return error(_("Unable to write index.")); > + > + return clean ? 0 : 1; > +} There are two differences here: - We use `error()` instead of the custom `err()` function that "merge-recursive.c" uses. I'm happy to see us using standard error reporting. - We don't have the check for `num_merge_bases == 1`. I have no idea why we don't have it, and it's likely that other readers may be puzzled in the same way. So this is something I'd expect to see explained in the commit message. Other than that this function looks identical. > diff --git a/merge-ort.c b/merge-ort.c > index 46e78c3ffa6..b4ff24403a1 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -5186,6 +5186,8 @@ static void merge_ort_internal(struct merge_options *opt, > ancestor_name = "empty tree"; > } else if (merge_bases) { > ancestor_name = "merged common ancestors"; > + } else if (opt->ancestor) { > + ancestor_name = opt->ancestor; > } else { > strbuf_add_unique_abbrev(&merge_base_abbrev, > &merged_merge_bases->object.oid, > @@ -5275,8 +5277,13 @@ void merge_incore_recursive(struct merge_options *opt, > { > trace2_region_enter("merge", "incore_recursive", opt->repo); > > - /* We set the ancestor label based on the merge_bases */ > - assert(opt->ancestor == NULL); > + /* > + * We set the ancestor label based on the merge_bases...but we > + * allow one exception through so that builtin/am can override > + * with its constructed fake ancestor. > + */ > + assert(opt->ancestor == NULL || > + (merge_bases && !merge_bases->next)); > > trace2_region_enter("merge", "merge_start", opt->repo); > merge_start(opt, result); These two hunks look related to my above observation that we don't have the check for `num_merge_bases == 1`, as in "merge-recursive.c" we used to set `opt->ancestor = "constructed merge base" if so. Patrick
On Wed, Mar 12, 2025 at 09:06:24AM +0100, Patrick Steinhardt wrote: > These two hunks look related to my above observation that we don't have > the check for `num_merge_bases == 1`, as in "merge-recursive.c" we used > to set `opt->ancestor = "constructed merge base" if so. Yeah, I noticed the same thing and agree that it would be helpful to see that spelled out in the commit message. Thanks, Taylor
On Wed, Mar 12, 2025 at 1:06 AM Patrick Steinhardt <ps@pks.im> wrote: > > On Fri, Mar 07, 2025 at 03:48:40PM +0000, Elijah Newren via GitGitGadget wrote: > > diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c > > index d6f61359965..62834c30e9e 100644 > > --- a/merge-ort-wrappers.c > > +++ b/merge-ort-wrappers.c > > @@ -64,3 +68,63 @@ int merge_ort_recursive(struct merge_options *opt, > > > > return tmp.clean; > > } > > + > > +static struct commit *get_ref(struct repository *repo, > > + const struct object_id *oid, > > + const char *name) > > +{ > > + struct object *object; > > + > > + object = deref_tag(repo, parse_object(repo, oid), > > + name, strlen(name)); > > + if (!object) > > + return NULL; > > + if (object->type == OBJ_TREE) > > + return make_virtual_commit(repo, (struct tree*)object, name); > > + if (object->type != OBJ_COMMIT) > > + return NULL; > > + if (repo_parse_commit(repo, (struct commit *)object)) > > + return NULL; > > + return (struct commit *)object; > > +} > > This is an exact copy of the same function in "merge-recursive.c". > > > +int merge_ort_generic(struct merge_options *opt, > > + const struct object_id *head, > > + const struct object_id *merge, > > + int num_merge_bases, > > + const struct object_id *merge_bases, > > + struct commit **result) > > +{ > > + int clean; > > + struct lock_file lock = LOCK_INIT; > > + struct commit *head_commit = get_ref(opt->repo, head, opt->branch1); > > + struct commit *next_commit = get_ref(opt->repo, merge, opt->branch2); > > + struct commit_list *ca = NULL; > > + > > + if (merge_bases) { > > + int i; > > + for (i = 0; i < num_merge_bases; ++i) { > > + struct commit *base; > > + if (!(base = get_ref(opt->repo, &merge_bases[i], > > + oid_to_hex(&merge_bases[i])))) > > + return error(_("Could not parse object '%s'"), > > + oid_to_hex(&merge_bases[i])); > > + commit_list_insert(base, &ca); > > + } > > + } > > + > > + repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR); > > + clean = merge_ort_recursive(opt, head_commit, next_commit, ca, > > + result); > > + free_commit_list(ca); > > + if (clean < 0) { > > + rollback_lock_file(&lock); > > + return clean; > > + } > > + > > + if (write_locked_index(opt->repo->index, &lock, > > + COMMIT_LOCK | SKIP_IF_UNCHANGED)) > > + return error(_("Unable to write index.")); > > + > > + return clean ? 0 : 1; > > +} > > There are two differences here: > > - We use `error()` instead of the custom `err()` function that > "merge-recursive.c" uses. I'm happy to see us using standard error > reporting. > > - We don't have the check for `num_merge_bases == 1`. I have no idea > why we don't have it, and it's likely that other readers may be > puzzled in the same way. So this is something I'd expect to see > explained in the commit message. Yeah, it looks like when I was splitting commits, I had more of this explanation in a commit which was explaining differences in testcases. I'll copy the relevant bits here. Thanks for reading carefully. > > Other than that this function looks identical. > > > diff --git a/merge-ort.c b/merge-ort.c > > index 46e78c3ffa6..b4ff24403a1 100644 > > --- a/merge-ort.c > > +++ b/merge-ort.c > > @@ -5186,6 +5186,8 @@ static void merge_ort_internal(struct merge_options *opt, > > ancestor_name = "empty tree"; > > } else if (merge_bases) { > > ancestor_name = "merged common ancestors"; > > + } else if (opt->ancestor) { > > + ancestor_name = opt->ancestor; > > } else { > > strbuf_add_unique_abbrev(&merge_base_abbrev, > > &merged_merge_bases->object.oid, > > @@ -5275,8 +5277,13 @@ void merge_incore_recursive(struct merge_options *opt, > > { > > trace2_region_enter("merge", "incore_recursive", opt->repo); > > > > - /* We set the ancestor label based on the merge_bases */ > > - assert(opt->ancestor == NULL); > > + /* > > + * We set the ancestor label based on the merge_bases...but we > > + * allow one exception through so that builtin/am can override > > + * with its constructed fake ancestor. > > + */ > > + assert(opt->ancestor == NULL || > > + (merge_bases && !merge_bases->next)); > > > > trace2_region_enter("merge", "merge_start", opt->repo); > > merge_start(opt, result); > > These two hunks look related to my above observation that we don't have > the check for `num_merge_bases == 1`, as in "merge-recursive.c" we used > to set `opt->ancestor = "constructed merge base" if so. > > Patrick
diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c index d6f61359965..62834c30e9e 100644 --- a/merge-ort-wrappers.c +++ b/merge-ort-wrappers.c @@ -1,9 +1,13 @@ #include "git-compat-util.h" #include "gettext.h" #include "hash.h" +#include "hex.h" +#include "lockfile.h" #include "merge-ort.h" #include "merge-ort-wrappers.h" #include "read-cache-ll.h" +#include "repository.h" +#include "tag.h" #include "tree.h" #include "commit.h" @@ -64,3 +68,63 @@ int merge_ort_recursive(struct merge_options *opt, return tmp.clean; } + +static struct commit *get_ref(struct repository *repo, + const struct object_id *oid, + const char *name) +{ + struct object *object; + + object = deref_tag(repo, parse_object(repo, oid), + name, strlen(name)); + if (!object) + return NULL; + if (object->type == OBJ_TREE) + return make_virtual_commit(repo, (struct tree*)object, name); + if (object->type != OBJ_COMMIT) + return NULL; + if (repo_parse_commit(repo, (struct commit *)object)) + return NULL; + return (struct commit *)object; +} + +int merge_ort_generic(struct merge_options *opt, + const struct object_id *head, + const struct object_id *merge, + int num_merge_bases, + const struct object_id *merge_bases, + struct commit **result) +{ + int clean; + struct lock_file lock = LOCK_INIT; + struct commit *head_commit = get_ref(opt->repo, head, opt->branch1); + struct commit *next_commit = get_ref(opt->repo, merge, opt->branch2); + struct commit_list *ca = NULL; + + if (merge_bases) { + int i; + for (i = 0; i < num_merge_bases; ++i) { + struct commit *base; + if (!(base = get_ref(opt->repo, &merge_bases[i], + oid_to_hex(&merge_bases[i])))) + return error(_("Could not parse object '%s'"), + oid_to_hex(&merge_bases[i])); + commit_list_insert(base, &ca); + } + } + + repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR); + clean = merge_ort_recursive(opt, head_commit, next_commit, ca, + result); + free_commit_list(ca); + if (clean < 0) { + rollback_lock_file(&lock); + return clean; + } + + if (write_locked_index(opt->repo->index, &lock, + COMMIT_LOCK | SKIP_IF_UNCHANGED)) + return error(_("Unable to write index.")); + + return clean ? 0 : 1; +} diff --git a/merge-ort-wrappers.h b/merge-ort-wrappers.h index 90af1f69c55..aeffa1c87b4 100644 --- a/merge-ort-wrappers.h +++ b/merge-ort-wrappers.h @@ -22,4 +22,16 @@ int merge_ort_recursive(struct merge_options *opt, const struct commit_list *ancestors, struct commit **result); +/* + * rename-detecting three-way merge. num_merge_bases must be at least 1. + * Recursive ancestor consolidation will be performed if num_merge_bases > 1. + * Wrapper mimicking the old merge_recursive_generic() function. + */ +int merge_ort_generic(struct merge_options *opt, + const struct object_id *head, + const struct object_id *merge, + int num_merge_bases, + const struct object_id *merge_bases, + struct commit **result); + #endif diff --git a/merge-ort.c b/merge-ort.c index 46e78c3ffa6..b4ff24403a1 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -4878,9 +4878,9 @@ static inline void set_commit_tree(struct commit *c, struct tree *t) c->maybe_tree = t; } -static struct commit *make_virtual_commit(struct repository *repo, - struct tree *tree, - const char *comment) +struct commit *make_virtual_commit(struct repository *repo, + struct tree *tree, + const char *comment) { struct commit *commit = alloc_commit_node(repo); @@ -5186,6 +5186,8 @@ static void merge_ort_internal(struct merge_options *opt, ancestor_name = "empty tree"; } else if (merge_bases) { ancestor_name = "merged common ancestors"; + } else if (opt->ancestor) { + ancestor_name = opt->ancestor; } else { strbuf_add_unique_abbrev(&merge_base_abbrev, &merged_merge_bases->object.oid, @@ -5275,8 +5277,13 @@ void merge_incore_recursive(struct merge_options *opt, { trace2_region_enter("merge", "incore_recursive", opt->repo); - /* We set the ancestor label based on the merge_bases */ - assert(opt->ancestor == NULL); + /* + * We set the ancestor label based on the merge_bases...but we + * allow one exception through so that builtin/am can override + * with its constructed fake ancestor. + */ + assert(opt->ancestor == NULL || + (merge_bases && !merge_bases->next)); trace2_region_enter("merge", "merge_start", opt->repo); merge_start(opt, result); diff --git a/merge-ort.h b/merge-ort.h index 82f2b3222d2..b63bc5424e7 100644 --- a/merge-ort.h +++ b/merge-ort.h @@ -44,6 +44,11 @@ struct merge_result { unsigned _properly_initialized; }; +/* Mostly internal function also used by merge-ort-wrappers.c */ +struct commit *make_virtual_commit(struct repository *repo, + struct tree *tree, + const char *comment); + /* * rename-detecting three-way merge with recursive ancestor consolidation. * working tree and index are untouched.