This series depends on en/merge-ort-2 (it does not depend on en/merge-ort-3 or en/merge-ort-recursive). This series adds handling of additional basic conflict types (directory/file conflicts, three-way content merging, very basic submodule divergence reconciliation, and different filetypes). This series drops the number of test failures under GIT_TEST_MERGE_ALGORITHM=ort by 211 (from 1448 to 1237). Further, if en/merge-tests, en/merge-ort-3, en/merge-ort-recursive, and this series are all merged down (in any order), then collectively they drop the number of test failure under GIT_TEST_MERGE_ALGORITHM=ort from 1448 down to 60. Elijah Newren (10): merge-ort: handle D/F conflict where directory disappears due to merge merge-ort: handle directory/file conflicts that remain merge-ort: implement unique_path() helper merge-ort: handle book-keeping around two- and three-way content merge merge-ort: flesh out implementation of handle_content_merge() merge-ort: copy and adapt merge_3way() from merge-recursive.c merge-ort: copy and adapt merge_submodule() from merge-recursive.c merge-ort: implement format_commit() merge-ort: copy find_first_merges() implementation from merge-recursive.c merge-ort: add handling for different types of files at same path merge-ort.c | 671 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 653 insertions(+), 18 deletions(-) base-commit: c5a6f65527aa3b6f5d7cf25437a88d8727ab0646 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-815%2Fnewren%2Fort-conflict-handling-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-815/newren/ort-conflict-handling-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/815 -- gitgitgadget
From: Elijah Newren <newren@gmail.com> When one side has a directory at a given path and the other side of history has a file at the path, but the merge resolves the directory away (e.g. because no path within that directory was modified and the other side deleted it, or because renaming moved all the files elsewhere), then we don't actually have a conflict anymore. We just need to clear away any information related to the relevant directory, and then the subsequent process_entry() handling can handle the given path. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index 414e7b7eeac..f9a79eb25e6 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -976,14 +976,35 @@ static void process_entry(struct merge_options *opt, assert(ci->df_conflict); } - if (ci->df_conflict) { + if (ci->df_conflict && ci->merged.result.mode == 0) { + int i; + + /* + * directory no longer in the way, but we do have a file we + * need to place here so we need to clean away the "directory + * merges to nothing" result. + */ + ci->df_conflict = 0; + assert(ci->filemask != 0); + ci->merged.clean = 0; + ci->merged.is_null = 0; + /* and we want to zero out any directory-related entries */ + ci->match_mask = (ci->match_mask & ~ci->dirmask); + ci->dirmask = 0; + for (i = MERGE_BASE; i <= MERGE_SIDE2; i++) { + if (ci->filemask & (1 << i)) + continue; + ci->stages[i].mode = 0; + oidcpy(&ci->stages[i].oid, &null_oid); + } + } else if (ci->df_conflict && ci->merged.result.mode != 0) { die("Not yet implemented."); } /* * NOTE: Below there is a long switch-like if-elseif-elseif... block * which the code goes through even for the df_conflict cases - * above. Well, it will once we don't die-not-implemented above. + * above. */ if (ci->match_mask) { ci->merged.clean = 1; -- gitgitgadget
From: Elijah Newren <newren@gmail.com> When a directory/file conflict remains, we can leave the directory where it is, but need to move the information about the file to a different pathname. After moving the file to a different pathname, we allow subsequent process_entry() logic to handle any additional details that might be relevant. This depends on a new helper function, unique_path(), that dies with an unimplemented error currently but will be implemented in a subsequent commit. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-) diff --git a/merge-ort.c b/merge-ort.c index f9a79eb25e6..d300a02810e 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -343,6 +343,13 @@ static void path_msg(struct merge_options *opt, strbuf_addch(sb, '\n'); } +static char *unique_path(struct strmap *existing_paths, + const char *path, + const char *branch) +{ + die("Not yet implemented."); +} + /*** Function Grouping: functions related to collect_merge_info() ***/ static void setup_path_info(struct merge_options *opt, @@ -962,6 +969,8 @@ static void process_entry(struct merge_options *opt, struct conflict_info *ci, struct directory_versions *dir_metadata) { + int df_file_index = 0; + VERIFY_CI(ci); assert(ci->filemask >= 0 && ci->filemask <= 7); /* ci->match_mask == 7 was handled in collect_merge_info_callback() */ @@ -998,7 +1007,80 @@ static void process_entry(struct merge_options *opt, oidcpy(&ci->stages[i].oid, &null_oid); } } else if (ci->df_conflict && ci->merged.result.mode != 0) { - die("Not yet implemented."); + /* + * This started out as a D/F conflict, and the entries in + * the competing directory were not removed by the merge as + * evidenced by write_completed_directory() writing a value + * to ci->merged.result.mode. + */ + struct conflict_info *new_ci; + const char *branch; + const char *old_path = path; + int i; + + assert(ci->merged.result.mode == S_IFDIR); + + /* + * If filemask is 1, we can just ignore the file as having + * been deleted on both sides. We do not want to overwrite + * ci->merged.result, since it stores the tree for all the + * files under it. + */ + if (ci->filemask == 1) { + ci->filemask = 0; + return; + } + + /* + * This file still exists on at least one side, and we want + * the directory to remain here, so we need to move this + * path to some new location. + */ + new_ci = xcalloc(1, sizeof(*new_ci)); + /* We don't really want new_ci->merged.result copied, but it'll + * be overwritten below so it doesn't matter. We also don't + * want any directory mode/oid values copied, but we'll zero + * those out immediately. We do want the rest of ci copied. + */ + memcpy(new_ci, ci, sizeof(*ci)); + new_ci->match_mask = (new_ci->match_mask & ~new_ci->dirmask); + new_ci->dirmask = 0; + for (i = MERGE_BASE; i <= MERGE_SIDE2; i++) { + if (new_ci->filemask & (1 << i)) + continue; + /* zero out any entries related to directories */ + new_ci->stages[i].mode = 0; + oidcpy(&new_ci->stages[i].oid, &null_oid); + } + + /* + * Find out which side this file came from; note that we + * cannot just use ci->filemask, because renames could cause + * the filemask to go back to 7. So we use dirmask, then + * pick the opposite side's index. + */ + df_file_index = (ci->dirmask & (1 << 1)) ? 2 : 1; + branch = (df_file_index == 1) ? opt->branch1 : opt->branch2; + path = unique_path(&opt->priv->paths, path, branch); + strmap_put(&opt->priv->paths, path, new_ci); + + path_msg(opt, path, 0, + _("CONFLICT (file/directory): directory in the way " + "of %s from %s; moving it to %s instead."), + old_path, branch, path); + + /* + * Zero out the filemask for the old ci. At this point, ci + * was just an entry for a directory, so we don't need to + * do anything more with it. + */ + ci->filemask = 0; + + /* + * Now note that we're working on the new entry (path was + * updated above. + */ + ci = new_ci; } /* -- gitgitgadget
From: Elijah Newren <newren@gmail.com> Implement unique_path(), based on the one from merge-recursive.c. It is simplified, however, due to: (1) using strmaps, and (2) the fact that merge-ort lets the checkout codepath handle possible collisions with the working tree means that other code locations don't have to. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/merge-ort.c b/merge-ort.c index d300a02810e..1adc27a11bc 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -343,11 +343,34 @@ static void path_msg(struct merge_options *opt, strbuf_addch(sb, '\n'); } +/* add a string to a strbuf, but converting "/" to "_" */ +static void add_flattened_path(struct strbuf *out, const char *s) +{ + size_t i = out->len; + strbuf_addstr(out, s); + for (; i < out->len; i++) + if (out->buf[i] == '/') + out->buf[i] = '_'; +} + static char *unique_path(struct strmap *existing_paths, const char *path, const char *branch) { - die("Not yet implemented."); + struct strbuf newpath = STRBUF_INIT; + int suffix = 0; + size_t base_len; + + strbuf_addf(&newpath, "%s~", path); + add_flattened_path(&newpath, branch); + + base_len = newpath.len; + while (strmap_contains(existing_paths, newpath.buf)) { + strbuf_setlen(&newpath, base_len); + strbuf_addf(&newpath, "_%d", suffix++); + } + + return strbuf_detach(&newpath, NULL); } /*** Function Grouping: functions related to collect_merge_info() ***/ -- gitgitgadget
From: Elijah Newren <newren@gmail.com> In addition to the content merge (which will go in a subsequent commit), we need to worry about conflict messages, placing results in higher order stages in case of a df_conflict, and making sure the results are placed in ci->merged.result so that they will show up in the working tree. Take care of all that external book-keeping, moving the simplistic just-take-HEAD code into the barebones handle_content_merge() function for now. Subsequent commits will flesh out handle_content_merge(). Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 52 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index 1adc27a11bc..47e230fe341 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -640,7 +640,15 @@ static int handle_content_merge(struct merge_options *opt, const int extra_marker_size, struct version_info *result) { - die("Not yet implemented"); + int clean = 0; + /* + * TODO: Needs a two-way or three-way content merge, but we're + * just being lazy and copying the version from HEAD and + * leaving it as conflicted. + */ + result->mode = a->mode; + oidcpy(&result->oid, &a->oid); + return clean; } /*** Function Grouping: functions related to detect_and_process_renames(), *** @@ -1138,16 +1146,38 @@ static void process_entry(struct merge_options *opt, */ die("Not yet implemented."); } else if (ci->filemask >= 6) { - /* - * TODO: Needs a two-way or three-way content merge, but we're - * just being lazy and copying the version from HEAD and - * leaving it as conflicted. - */ - ci->merged.clean = 0; - ci->merged.result.mode = ci->stages[1].mode; - oidcpy(&ci->merged.result.oid, &ci->stages[1].oid); - /* When we fix above, we'll call handle_content_merge() */ - (void)handle_content_merge; + /* Need a two-way or three-way content merge */ + struct version_info merged_file; + unsigned clean_merge; + struct version_info *o = &ci->stages[0]; + struct version_info *a = &ci->stages[1]; + struct version_info *b = &ci->stages[2]; + + clean_merge = handle_content_merge(opt, path, o, a, b, + ci->pathnames, + opt->priv->call_depth * 2, + &merged_file); + ci->merged.clean = clean_merge && + !ci->df_conflict && !ci->path_conflict; + ci->merged.result.mode = merged_file.mode; + ci->merged.is_null = (merged_file.mode == 0); + oidcpy(&ci->merged.result.oid, &merged_file.oid); + if (clean_merge && ci->df_conflict) { + assert(df_file_index == 1 || df_file_index == 2); + ci->filemask = 1 << df_file_index; + ci->stages[df_file_index].mode = merged_file.mode; + oidcpy(&ci->stages[df_file_index].oid, &merged_file.oid); + } + if (!clean_merge) { + const char *reason = _("content"); + if (ci->filemask == 6) + reason = _("add/add"); + if (S_ISGITLINK(merged_file.mode)) + reason = _("submodule"); + path_msg(opt, path, 0, + _("CONFLICT (%s): Merge conflict in %s"), + reason, path); + } } else if (ci->filemask == 3 || ci->filemask == 5) { /* Modify/delete */ const char *modify_branch, *delete_branch; -- gitgitgadget
From: Elijah Newren <newren@gmail.com> This implementation is based heavily on merge_mode_and_contents() from merge-recursive.c, though it has some fixes for recursive merges (i.e. when call_depth > 0), and has a number of changes throughout based on slight differences in data structures and in how the functions are called. It is, however, based on two new helper functions -- merge_3way() and merge_submodule -- for which we only provide die-not-implemented stubs at this point. Future commits will add implementations of these functions. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 143 insertions(+), 6 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index 47e230fe341..2cfb7ffa3b0 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -631,6 +631,28 @@ static int collect_merge_info(struct merge_options *opt, /*** Function Grouping: functions related to threeway content merges ***/ +static int merge_submodule(struct merge_options *opt, + const char *path, + const struct object_id *o, + const struct object_id *a, + const struct object_id *b, + struct object_id *result) +{ + die("Not yet implemented."); +} + +static int merge_3way(struct merge_options *opt, + const char *path, + const struct object_id *o, + const struct object_id *a, + const struct object_id *b, + const char *pathnames[3], + const int extra_marker_size, + mmbuffer_t *result_buf) +{ + die("Not yet implemented."); +} + static int handle_content_merge(struct merge_options *opt, const char *path, const struct version_info *o, @@ -640,14 +662,129 @@ static int handle_content_merge(struct merge_options *opt, const int extra_marker_size, struct version_info *result) { - int clean = 0; /* - * TODO: Needs a two-way or three-way content merge, but we're - * just being lazy and copying the version from HEAD and - * leaving it as conflicted. + * path is the target location where we want to put the file, and + * is used to determine any normalization rules in ll_merge. + * + * The normal case is that path and all entries in pathnames are + * identical, though renames can affect which path we got one of + * the three blobs to merge on various sides of history. + * + * extra_marker_size is the amount to extend conflict markers in + * ll_merge; this is neeed if we have content merges of content + * merges, which happens for example with rename/rename(2to1) and + * rename/add conflicts. */ - result->mode = a->mode; - oidcpy(&result->oid, &a->oid); + unsigned clean = 1; + + /* + * handle_content_merge() needs both files to be of the same type, i.e. + * both files OR both submodules OR both symlinks. Conflicting types + * needs to be handled elsewhere. + */ + assert((S_IFMT & a->mode) == (S_IFMT & b->mode)); + + /* Merge modes */ + if (a->mode == b->mode || a->mode == o->mode) + result->mode = b->mode; + else { + /* must be the 100644/100755 case */ + assert(S_ISREG(a->mode)); + result->mode = a->mode; + clean = (b->mode == o->mode); + /* + * FIXME: If opt->priv->call_depth && !clean, then we really + * should not make result->mode match either a->mode or + * b->mode; that causes t6036 "check conflicting mode for + * regular file" to fail. It would be best to use some other + * mode, but we'll confuse all kinds of stuff if we use one + * where S_ISREG(result->mode) isn't true, and if we use + * something like 0100666, then tree-walk.c's calls to + * canon_mode() will just normalize that to 100644 for us and + * thus not solve anything. + * + * Figure out if there's some kind of way we can work around + * this... + */ + } + + /* + * Trivial oid merge. + * + * Note: While one might assume that the next four lines would + * be unnecessary due to the fact that match_mask is often + * setup and already handled, renames don't always take care + * of that. + */ + if (oideq(&a->oid, &b->oid) || oideq(&a->oid, &o->oid)) + oidcpy(&result->oid, &b->oid); + else if (oideq(&b->oid, &o->oid)) + oidcpy(&result->oid, &a->oid); + + /* Remaining rules depend on file vs. submodule vs. symlink. */ + else if (S_ISREG(a->mode)) { + mmbuffer_t result_buf; + int ret = 0, merge_status; + int two_way; + + /* + * If 'o' is different type, treat it as null so we do a + * two-way merge. + */ + two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode)); + + merge_status = merge_3way(opt, path, + two_way ? &null_oid : &o->oid, + &a->oid, &b->oid, + pathnames, extra_marker_size, + &result_buf); + + if ((merge_status < 0) || !result_buf.ptr) + ret = err(opt, _("Failed to execute internal merge")); + + if (!ret && + write_object_file(result_buf.ptr, result_buf.size, + blob_type, &result->oid)) + ret = err(opt, _("Unable to add %s to database"), + path); + + free(result_buf.ptr); + if (ret) + return -1; + clean &= (merge_status == 0); + path_msg(opt, path, 1, _("Auto-merging %s"), path); + } else if (S_ISGITLINK(a->mode)) { + int two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode)); + clean = merge_submodule(opt, pathnames[0], + two_way ? &null_oid : &o->oid, + &a->oid, &b->oid, &result->oid); + if (opt->priv->call_depth && two_way && !clean) { + result->mode = o->mode; + oidcpy(&result->oid, &o->oid); + } + } else if (S_ISLNK(a->mode)) { + if (opt->priv->call_depth) { + clean = 0; + result->mode = o->mode; + oidcpy(&result->oid, &o->oid); + } else { + switch (opt->recursive_variant) { + case MERGE_VARIANT_NORMAL: + clean = 0; + oidcpy(&result->oid, &a->oid); + break; + case MERGE_VARIANT_OURS: + oidcpy(&result->oid, &a->oid); + break; + case MERGE_VARIANT_THEIRS: + oidcpy(&result->oid, &b->oid); + break; + } + } + } else + BUG("unsupported object type in the tree: %06o for %s", + a->mode, path); + return clean; } -- gitgitgadget
From: Elijah Newren <newren@gmail.com> Take merge_3way() from merge-recursive.c and make slight adjustments based on different data structures (direct usage of object_id rather diff_filespec, separate pathnames which based on our careful interning of pathnames in opt->priv->paths can be compared with '!=' rather than 'strcmp'). Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/merge-ort.c b/merge-ort.c index 2cfb7ffa3b0..a59adb42aa6 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -23,6 +23,7 @@ #include "diff.h" #include "diffcore.h" #include "dir.h" +#include "ll-merge.h" #include "object-store.h" #include "strmap.h" #include "tree.h" @@ -650,7 +651,58 @@ static int merge_3way(struct merge_options *opt, const int extra_marker_size, mmbuffer_t *result_buf) { - die("Not yet implemented."); + mmfile_t orig, src1, src2; + struct ll_merge_options ll_opts = {0}; + char *base, *name1, *name2; + int merge_status; + + ll_opts.renormalize = opt->renormalize; + ll_opts.extra_marker_size = extra_marker_size; + ll_opts.xdl_opts = opt->xdl_opts; + + if (opt->priv->call_depth) { + ll_opts.virtual_ancestor = 1; + ll_opts.variant = 0; + } else { + switch (opt->recursive_variant) { + case MERGE_VARIANT_OURS: + ll_opts.variant = XDL_MERGE_FAVOR_OURS; + break; + case MERGE_VARIANT_THEIRS: + ll_opts.variant = XDL_MERGE_FAVOR_THEIRS; + break; + default: + ll_opts.variant = 0; + break; + } + } + + assert(pathnames[0] && pathnames[1] && pathnames[2] && opt->ancestor); + if (pathnames[0] == pathnames[1] && pathnames[1] == pathnames[2]) { + base = mkpathdup("%s", opt->ancestor); + name1 = mkpathdup("%s", opt->branch1); + name2 = mkpathdup("%s", opt->branch2); + } else { + base = mkpathdup("%s:%s", opt->ancestor, pathnames[0]); + name1 = mkpathdup("%s:%s", opt->branch1, pathnames[1]); + name2 = mkpathdup("%s:%s", opt->branch2, pathnames[2]); + } + + read_mmblob(&orig, o); + read_mmblob(&src1, a); + read_mmblob(&src2, b); + + merge_status = ll_merge(result_buf, path, &orig, base, + &src1, name1, &src2, name2, + opt->repo->index, &ll_opts); + + free(base); + free(name1); + free(name2); + free(orig.ptr); + free(src1.ptr); + free(src2.ptr); + return merge_status; } static int handle_content_merge(struct merge_options *opt, -- gitgitgadget
From: Elijah Newren <newren@gmail.com> Take merge_submodule() from merge-recursive.c and make slight adjustments, predominantly around deferring output using path_msg() instead of using merge-recursive's output() and show() functions. There's also a fix for recursive cases (when call_depth > 0) and a slight change to argument order for find_first_merges(). find_first_merges() and format_commit() are left unimplemented for now, but will be added by subsequent commits. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-) diff --git a/merge-ort.c b/merge-ort.c index a59adb42aa6..2dfab1858fc 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -26,6 +26,7 @@ #include "ll-merge.h" #include "object-store.h" #include "strmap.h" +#include "submodule.h" #include "tree.h" #include "unpack-trees.h" #include "xdiff-interface.h" @@ -323,6 +324,13 @@ static int err(struct merge_options *opt, const char *err, ...) return -1; } +static void format_commit(struct strbuf *sb, + int indent, + struct commit *commit) +{ + die("Not yet implemented."); +} + __attribute__((format (printf, 4, 5))) static void path_msg(struct merge_options *opt, const char *path, @@ -632,6 +640,15 @@ static int collect_merge_info(struct merge_options *opt, /*** Function Grouping: functions related to threeway content merges ***/ +static int find_first_merges(struct repository *repo, + const char *path, + struct commit *a, + struct commit *b, + struct object_array *result) +{ + die("Not yet implemented."); +} + static int merge_submodule(struct merge_options *opt, const char *path, const struct object_id *o, @@ -639,7 +656,114 @@ static int merge_submodule(struct merge_options *opt, const struct object_id *b, struct object_id *result) { - die("Not yet implemented."); + struct commit *commit_o, *commit_a, *commit_b; + int parent_count; + struct object_array merges; + struct strbuf sb = STRBUF_INIT; + + int i; + int search = !opt->priv->call_depth; + + /* store fallback answer in result in case we fail */ + oidcpy(result, opt->priv->call_depth ? o : a); + + /* we can not handle deletion conflicts */ + if (is_null_oid(o)) + return 0; + if (is_null_oid(a)) + return 0; + if (is_null_oid(b)) + return 0; + + if (add_submodule_odb(path)) { + path_msg(opt, path, 0, + _("Failed to merge submodule %s (not checked out)"), + path); + return 0; + } + + if (!(commit_o = lookup_commit_reference(opt->repo, o)) || + !(commit_a = lookup_commit_reference(opt->repo, a)) || + !(commit_b = lookup_commit_reference(opt->repo, b))) { + path_msg(opt, path, 0, + _("Failed to merge submodule %s (commits not present)"), + path); + return 0; + } + + /* check whether both changes are forward */ + if (!in_merge_bases(commit_o, commit_a) || + !in_merge_bases(commit_o, commit_b)) { + path_msg(opt, path, 0, + _("Failed to merge submodule %s " + "(commits don't follow merge-base)"), + path); + return 0; + } + + /* Case #1: a is contained in b or vice versa */ + if (in_merge_bases(commit_a, commit_b)) { + oidcpy(result, b); + path_msg(opt, path, 1, + _("Note: Fast-forwarding submodule %s to %s"), + path, oid_to_hex(b)); + return 1; + } + if (in_merge_bases(commit_b, commit_a)) { + oidcpy(result, a); + path_msg(opt, path, 1, + _("Note: Fast-forwarding submodule %s to %s"), + path, oid_to_hex(a)); + return 1; + } + + /* + * Case #2: There are one or more merges that contain a and b in + * the submodule. If there is only one, then present it as a + * suggestion to the user, but leave it marked unmerged so the + * user needs to confirm the resolution. + */ + + /* Skip the search if makes no sense to the calling context. */ + if (!search) + return 0; + + /* find commit which merges them */ + parent_count = find_first_merges(opt->repo, path, commit_a, commit_b, + &merges); + switch (parent_count) { + case 0: + path_msg(opt, path, 0, _("Failed to merge submodule %s"), path); + break; + + case 1: + format_commit(&sb, 4, + (struct commit *)merges.objects[0].item); + path_msg(opt, path, 0, + _("Failed to merge submodule %s, but a possible merge " + "resolution exists:\n%s\n"), + path, sb.buf); + path_msg(opt, path, 1, + _("If this is correct simply add it to the index " + "for example\n" + "by using:\n\n" + " git update-index --cacheinfo 160000 %s \"%s\"\n\n" + "which will accept this suggestion.\n"), + oid_to_hex(&merges.objects[0].item->oid), path); + strbuf_release(&sb); + break; + default: + for (i = 0; i < merges.nr; i++) + format_commit(&sb, 4, + (struct commit *)merges.objects[i].item); + path_msg(opt, path, 0, + _("Failed to merge submodule %s, but multiple " + "possible merges exist:\n%s"), path, sb.buf); + strbuf_release(&sb); + } + + object_array_clear(&merges); + return 0; } static int merge_3way(struct merge_options *opt, -- gitgitgadget
From: Elijah Newren <newren@gmail.com> This implementation is based on a mixture of print_commit() and output_commit_title() from merge-recursive.c so that it can be used to take over both functions. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/merge-ort.c b/merge-ort.c index 2dfab1858fc..bf704bcd34d 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -328,7 +328,19 @@ static void format_commit(struct strbuf *sb, int indent, struct commit *commit) { - die("Not yet implemented."); + struct merge_remote_desc *desc; + struct pretty_print_context ctx = {0}; + ctx.abbrev = DEFAULT_ABBREV; + + strbuf_addchars(sb, ' ', indent); + desc = merge_remote_util(commit); + if (desc) { + strbuf_addf(sb, "virtual %s\n", desc->name); + return; + } + + format_commit_message(commit, "%h %s", sb, &ctx); + strbuf_addch(sb, '\n'); } __attribute__((format (printf, 4, 5))) -- gitgitgadget
From: Elijah Newren <newren@gmail.com> Code is identical for the function body in the two files, the call signature is just slightly different in merge-ort than merge-recursive as noted a couple commits ago. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/merge-ort.c b/merge-ort.c index bf704bcd34d..203fa987e43 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -25,6 +25,7 @@ #include "dir.h" #include "ll-merge.h" #include "object-store.h" +#include "revision.h" #include "strmap.h" #include "submodule.h" #include "tree.h" @@ -658,7 +659,61 @@ static int find_first_merges(struct repository *repo, struct commit *b, struct object_array *result) { - die("Not yet implemented."); + int i, j; + struct object_array merges = OBJECT_ARRAY_INIT; + struct commit *commit; + int contains_another; + + char merged_revision[GIT_MAX_HEXSZ + 2]; + const char *rev_args[] = { "rev-list", "--merges", "--ancestry-path", + "--all", merged_revision, NULL }; + struct rev_info revs; + struct setup_revision_opt rev_opts; + + memset(result, 0, sizeof(struct object_array)); + memset(&rev_opts, 0, sizeof(rev_opts)); + + /* get all revisions that merge commit a */ + xsnprintf(merged_revision, sizeof(merged_revision), "^%s", + oid_to_hex(&a->object.oid)); + repo_init_revisions(repo, &revs, NULL); + rev_opts.submodule = path; + /* FIXME: can't handle linked worktrees in submodules yet */ + revs.single_worktree = path != NULL; + setup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts); + + /* save all revisions from the above list that contain b */ + if (prepare_revision_walk(&revs)) + die("revision walk setup failed"); + while ((commit = get_revision(&revs)) != NULL) { + struct object *o = &(commit->object); + if (in_merge_bases(b, commit)) + add_object_array(o, NULL, &merges); + } + reset_revision_walk(); + + /* Now we've got all merges that contain a and b. Prune all + * merges that contain another found merge and save them in + * result. + */ + for (i = 0; i < merges.nr; i++) { + struct commit *m1 = (struct commit *) merges.objects[i].item; + + contains_another = 0; + for (j = 0; j < merges.nr; j++) { + struct commit *m2 = (struct commit *) merges.objects[j].item; + if (i != j && in_merge_bases(m2, m1)) { + contains_another = 1; + break; + } + } + + if (!contains_another) + add_object_array(merges.objects[i].item, NULL, result); + } + + object_array_clear(&merges); + return result->nr; } static int merge_submodule(struct merge_options *opt, -- gitgitgadget
From: Elijah Newren <newren@gmail.com> Add some handling that explicitly considers collisions of the following types: * file/submodule * file/symlink * submodule/symlink Leaving them as conflicts at the same path are hard for users to resolve, so move one or both of them aside so that they each get their own path. Note that in the case of recursive handling (i.e. call_depth > 0), we can just use the merge base of the two merge bases as the merge result much like we do with modify/delete conflicts, binary files, conflicting submodule values, and so on. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 103 insertions(+), 4 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index 203fa987e43..afe721182e2 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -1521,10 +1521,109 @@ static void process_entry(struct merge_options *opt, } else if (ci->filemask >= 6 && (S_IFMT & ci->stages[1].mode) != (S_IFMT & ci->stages[2].mode)) { - /* - * Two different items from (file/submodule/symlink) - */ - die("Not yet implemented."); + /* Two different items from (file/submodule/symlink) */ + if (opt->priv->call_depth) { + /* Just use the version from the merge base */ + ci->merged.clean = 0; + oidcpy(&ci->merged.result.oid, &ci->stages[0].oid); + ci->merged.result.mode = ci->stages[0].mode; + ci->merged.is_null = (ci->merged.result.mode == 0); + } else { + /* Handle by renaming one or both to separate paths. */ + unsigned o_mode = ci->stages[0].mode; + unsigned a_mode = ci->stages[1].mode; + unsigned b_mode = ci->stages[2].mode; + struct conflict_info *new_ci; + const char *a_path = NULL, *b_path = NULL; + int rename_a = 0, rename_b = 0; + + new_ci = xmalloc(sizeof(*new_ci)); + + if (S_ISREG(a_mode)) + rename_a = 1; + else if (S_ISREG(b_mode)) + rename_b = 1; + else { + rename_a = 1; + rename_b = 1; + } + + path_msg(opt, path, 0, + _("CONFLICT (distinct types): %s had different " + "types on each side; renamed %s of them so " + "each can be recorded somewhere."), + path, + (rename_a && rename_b) ? _("both") : _("one")); + + ci->merged.clean = 0; + memcpy(new_ci, ci, sizeof(*new_ci)); + + /* Put b into new_ci, removing a from stages */ + new_ci->merged.result.mode = ci->stages[2].mode; + oidcpy(&new_ci->merged.result.oid, &ci->stages[2].oid); + new_ci->stages[1].mode = 0; + oidcpy(&new_ci->stages[1].oid, &null_oid); + new_ci->filemask = 5; + if ((S_IFMT & b_mode) != (S_IFMT & o_mode)) { + new_ci->stages[0].mode = 0; + oidcpy(&new_ci->stages[0].oid, &null_oid); + new_ci->filemask = 4; + } + + /* Leave only a in ci, fixing stages. */ + ci->merged.result.mode = ci->stages[1].mode; + oidcpy(&ci->merged.result.oid, &ci->stages[1].oid); + ci->stages[2].mode = 0; + oidcpy(&ci->stages[2].oid, &null_oid); + ci->filemask = 3; + if ((S_IFMT & a_mode) != (S_IFMT & o_mode)) { + ci->stages[0].mode = 0; + oidcpy(&ci->stages[0].oid, &null_oid); + ci->filemask = 2; + } + + /* Insert entries into opt->priv_paths */ + assert(rename_a || rename_b); + if (rename_a) { + a_path = unique_path(&opt->priv->paths, + path, opt->branch1); + strmap_put(&opt->priv->paths, a_path, ci); + } + + if (rename_b) + b_path = unique_path(&opt->priv->paths, + path, opt->branch2); + else + b_path = path; + strmap_put(&opt->priv->paths, b_path, new_ci); + + if (rename_a && rename_b) { + strmap_remove(&opt->priv->paths, path, 0); + /* + * We removed path from opt->priv->paths. path + * will also eventually need to be freed, but + * it may still be used by e.g. ci->pathnames. + * So, store it in another string-list for now. + */ + string_list_append(&opt->priv->paths_to_free, + path); + } + + /* + * Do special handling for b_path since process_entry() + * won't be called on it specially. + */ + strmap_put(&opt->priv->conflicted, b_path, new_ci); + record_entry_for_tree(dir_metadata, b_path, + &new_ci->merged); + + /* + * Remaining code for processing this entry should + * think in terms of processing a_path. + */ + if (a_path) + path = a_path; + } } else if (ci->filemask >= 6) { /* Need a two-way or three-way content merge */ struct version_info merged_file; -- gitgitgadget
Hi Junio, On Thu, Dec 17, 2020 at 9:51 PM Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote: > > This series depends on en/merge-ort-2 (it does not depend on en/merge-ort-3 > or en/merge-ort-recursive). > > This series adds handling of additional basic conflict types (directory/file I submitted this about a week and a half ago, and figured you might not have picked it up for 'seen' because of the -rc freeze and focus on the release. With 2.30.0 released now, would it be easier for you if I resent the series (with no changes) or is it easier to just pick this series up as it is? Thanks, Elijah > conflicts, three-way content merging, very basic submodule divergence > reconciliation, and different filetypes). This series drops the number of > test failures under GIT_TEST_MERGE_ALGORITHM=ort by 211 (from 1448 to 1237). > > Further, if en/merge-tests, en/merge-ort-3, en/merge-ort-recursive, and this > series are all merged down (in any order), then collectively they drop the > number of test failure under GIT_TEST_MERGE_ALGORITHM=ort from 1448 down to > 60. > > Elijah Newren (10): > merge-ort: handle D/F conflict where directory disappears due to merge > merge-ort: handle directory/file conflicts that remain > merge-ort: implement unique_path() helper > merge-ort: handle book-keeping around two- and three-way content merge > merge-ort: flesh out implementation of handle_content_merge() > merge-ort: copy and adapt merge_3way() from merge-recursive.c > merge-ort: copy and adapt merge_submodule() from merge-recursive.c > merge-ort: implement format_commit() > merge-ort: copy find_first_merges() implementation from > merge-recursive.c > merge-ort: add handling for different types of files at same path > > merge-ort.c | 671 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 653 insertions(+), 18 deletions(-) > > > base-commit: c5a6f65527aa3b6f5d7cf25437a88d8727ab0646 > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-815%2Fnewren%2Fort-conflict-handling-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-815/newren/ort-conflict-handling-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/815 > -- > gitgitgadget
On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:
> + } else if (ci->df_conflict && ci->merged.result.mode != 0) {
> die("Not yet implemented.");
> }
>
> /*
> * NOTE: Below there is a long switch-like if-elseif-elseif... block
> * which the code goes through even for the df_conflict cases
> - * above. Well, it will once we don't die-not-implemented above.
> + * above.
> */
This comment change might be a bit premature.
Thanks,
-Stolee
On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> Implement unique_path(), based on the one from merge-recursive.c. It is
> simplified, however, due to: (1) using strmaps, and (2) the fact that
> merge-ort lets the checkout codepath handle possible collisions with the
> working tree means that other code locations don't have to.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> merge-ort.c | 25 ++++++++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index d300a02810e..1adc27a11bc 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -343,11 +343,34 @@ static void path_msg(struct merge_options *opt,
> strbuf_addch(sb, '\n');
> }
>
> +/* add a string to a strbuf, but converting "/" to "_" */
> +static void add_flattened_path(struct strbuf *out, const char *s)
> +{
> + size_t i = out->len;
> + strbuf_addstr(out, s);
> + for (; i < out->len; i++)
> + if (out->buf[i] == '/')
> + out->buf[i] = '_';
> +}
> +
Thank you for pointing out that you based your code on merge-recursive.c.
I see that this implementation is identical to the one there. I question
whether this causes collisions in a problematic way, when "a/b/c" and
"a_b_c" both exist in a tree.
To avoid such a problem, we'd likely need to also expand "_" to "__" or
similar. This might never actually affect any users because of the
strange filename matches _and_ we need to be in a directory/file conflict.
And maybe it's not a hole at all? If it is, we can consider patching or
at least documenting the problem.
Thanks,
-Stolee
On Wed, Dec 30, 2020 at 6:16 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Implement unique_path(), based on the one from merge-recursive.c. It is
> > simplified, however, due to: (1) using strmaps, and (2) the fact that
> > merge-ort lets the checkout codepath handle possible collisions with the
> > working tree means that other code locations don't have to.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> > merge-ort.c | 25 ++++++++++++++++++++++++-
> > 1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/merge-ort.c b/merge-ort.c
> > index d300a02810e..1adc27a11bc 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -343,11 +343,34 @@ static void path_msg(struct merge_options *opt,
> > strbuf_addch(sb, '\n');
> > }
> >
> > +/* add a string to a strbuf, but converting "/" to "_" */
> > +static void add_flattened_path(struct strbuf *out, const char *s)
> > +{
> > + size_t i = out->len;
> > + strbuf_addstr(out, s);
> > + for (; i < out->len; i++)
> > + if (out->buf[i] == '/')
> > + out->buf[i] = '_';
> > +}
> > +
>
> Thank you for pointing out that you based your code on merge-recursive.c.
> I see that this implementation is identical to the one there. I question
> whether this causes collisions in a problematic way, when "a/b/c" and
> "a_b_c" both exist in a tree.
>
> To avoid such a problem, we'd likely need to also expand "_" to "__" or
> similar. This might never actually affect any users because of the
> strange filename matches _and_ we need to be in a directory/file conflict.
>
> And maybe it's not a hole at all? If it is, we can consider patching or
> at least documenting the problem.
add_flattened_path() can certainly result in a collision, regardless
of whether the char *s parameter has any '/' characters in it. For
example, if you are trying to get a unique path associated with
builtin/commit-graph.c due to changes from the 'next' branch side of
the merge, and builtin/commit-graph.c~next already exists, then you
have a collision. It's actually pretty rare that the parameter would
have any '/' characters at all, since it's pretty rare for me to see
folks (other than Junio) use hierarchical branch names. But if the
branch name were ds/line-log-on-bloom, then the provisional filename
would be builtin/commit-graph.c~ds_line-log-on-bloom. The '/' to '_'
conversion exists just to make sure our new file remains in the same
directory as where the conflict that caused us to need a new unique
path occurred.
But unique_path() does NOT end immediately after calling
add_flattened_path() and there is no collision possible in the return
from unique_path(), because it ends with a
while (strmap_contains(existing_paths, newpath.buf)) {
loop that modifies the resulting path until it finds one that doesn't
collide with an existing path.
On Wed, Dec 30, 2020 at 6:06 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:
> > + } else if (ci->df_conflict && ci->merged.result.mode != 0) {
> > die("Not yet implemented.");
> > }
> >
> > /*
> > * NOTE: Below there is a long switch-like if-elseif-elseif... block
> > * which the code goes through even for the df_conflict cases
> > - * above. Well, it will once we don't die-not-implemented above.
> > + * above.
> > */
>
> This comment change might be a bit premature.
Or perhaps it should have been squashed into an earlier series that
was already merged to next.
On 12/30/2020 10:13 AM, Elijah Newren wrote:
> On Wed, Dec 30, 2020 at 6:06 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:
>>> + } else if (ci->df_conflict && ci->merged.result.mode != 0) {
>>> die("Not yet implemented.");
>>> }
>>>
>>> /*
>>> * NOTE: Below there is a long switch-like if-elseif-elseif... block
>>> * which the code goes through even for the df_conflict cases
>>> - * above. Well, it will once we don't die-not-implemented above.
>>> + * above.
>>> */
>>
>> This comment change might be a bit premature.
>
> Or perhaps it should have been squashed into an earlier series that
> was already merged to next.
I think it works with the next patch, which removes the die() from the
if-elseif-elseif from immediately before the comment.
-Stolee
On 12/30/2020 10:10 AM, Elijah Newren wrote: > On Wed, Dec 30, 2020 at 6:16 AM Derrick Stolee <stolee@gmail.com> wrote: >> On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote: >>> From: Elijah Newren <newren@gmail.com> >>> >>> Implement unique_path(), based on the one from merge-recursive.c. It is >>> simplified, however, due to: (1) using strmaps, and (2) the fact that >>> merge-ort lets the checkout codepath handle possible collisions with the >>> working tree means that other code locations don't have to. >>> >>> Signed-off-by: Elijah Newren <newren@gmail.com> >>> --- >>> merge-ort.c | 25 ++++++++++++++++++++++++- >>> 1 file changed, 24 insertions(+), 1 deletion(-) >>> >>> diff --git a/merge-ort.c b/merge-ort.c >>> index d300a02810e..1adc27a11bc 100644 >>> --- a/merge-ort.c >>> +++ b/merge-ort.c >>> @@ -343,11 +343,34 @@ static void path_msg(struct merge_options *opt, >>> strbuf_addch(sb, '\n'); >>> } >>> >>> +/* add a string to a strbuf, but converting "/" to "_" */ >>> +static void add_flattened_path(struct strbuf *out, const char *s) >>> +{ >>> + size_t i = out->len; >>> + strbuf_addstr(out, s); >>> + for (; i < out->len; i++) >>> + if (out->buf[i] == '/') >>> + out->buf[i] = '_'; >>> +} >>> + >> >> Thank you for pointing out that you based your code on merge-recursive.c. >> I see that this implementation is identical to the one there. I question >> whether this causes collisions in a problematic way, when "a/b/c" and >> "a_b_c" both exist in a tree. >> >> To avoid such a problem, we'd likely need to also expand "_" to "__" or >> similar. This might never actually affect any users because of the >> strange filename matches _and_ we need to be in a directory/file conflict. >> >> And maybe it's not a hole at all? If it is, we can consider patching or >> at least documenting the problem. > > add_flattened_path() can certainly result in a collision, regardless > of whether the char *s parameter has any '/' characters in it. For > example, if you are trying to get a unique path associated with > builtin/commit-graph.c due to changes from the 'next' branch side of > the merge, and builtin/commit-graph.c~next already exists, then you > have a collision. It's actually pretty rare that the parameter would > have any '/' characters at all, since it's pretty rare for me to see > folks (other than Junio) use hierarchical branch names. But if the > branch name were ds/line-log-on-bloom, then the provisional filename > would be builtin/commit-graph.c~ds_line-log-on-bloom. The '/' to '_' > conversion exists just to make sure our new file remains in the same > directory as where the conflict that caused us to need a new unique > path occurred. Ah, I am misinterpreting which '/' characters are getting squashed. Thank you for fixing my confusion. > But unique_path() does NOT end immediately after calling > add_flattened_path() and there is no collision possible in the return > from unique_path(), because it ends with a > while (strmap_contains(existing_paths, newpath.buf)) { > loop that modifies the resulting path until it finds one that doesn't > collide with an existing path. And this extra care here is helpful. Thanks! -Stolee
On Thu, Dec 31, 2020 at 3:17 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 12/30/2020 10:13 AM, Elijah Newren wrote:
> > On Wed, Dec 30, 2020 at 6:06 AM Derrick Stolee <stolee@gmail.com> wrote:
> >>
> >> On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:
> >>> + } else if (ci->df_conflict && ci->merged.result.mode != 0) {
> >>> die("Not yet implemented.");
> >>> }
> >>>
> >>> /*
> >>> * NOTE: Below there is a long switch-like if-elseif-elseif... block
> >>> * which the code goes through even for the df_conflict cases
> >>> - * above. Well, it will once we don't die-not-implemented above.
> >>> + * above.
> >>> */
> >>
> >> This comment change might be a bit premature.
> >
> > Or perhaps it should have been squashed into an earlier series that
> > was already merged to next.
>
> I think it works with the next patch, which removes the die() from the
> if-elseif-elseif from immediately before the comment.
Oh, right, it's been long enough that I forgot the details and then I
read the patch backwards thinking it was adding the message. Yeah, it
should go with the next patch. I'll fix it up.
This series depends on en/merge-ort-2 (it does not depend on en/merge-ort-3 or en/merge-ort-recursive). This series adds handling of additional basic conflict types (directory/file conflicts, three-way content merging, very basic submodule divergence reconciliation, and different filetypes). This series drops the number of test failures under GIT_TEST_MERGE_ALGORITHM=ort by 211 (from 1448 to 1237). Further, if en/merge-tests, en/merge-ort-3, en/merge-ort-recursive, and this series are all merged down (in any order), then collectively they drop the number of test failure under GIT_TEST_MERGE_ALGORITHM=ort from 1448 down to 60. Changes since v1: * Wait to remove comment about a die-not-implemented code block until the commit where we actually remove it (spotted by Stollee) Elijah Newren (10): merge-ort: handle D/F conflict where directory disappears due to merge merge-ort: handle directory/file conflicts that remain merge-ort: implement unique_path() helper merge-ort: handle book-keeping around two- and three-way content merge merge-ort: flesh out implementation of handle_content_merge() merge-ort: copy and adapt merge_3way() from merge-recursive.c merge-ort: copy and adapt merge_submodule() from merge-recursive.c merge-ort: implement format_commit() merge-ort: copy find_first_merges() implementation from merge-recursive.c merge-ort: add handling for different types of files at same path merge-ort.c | 671 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 653 insertions(+), 18 deletions(-) base-commit: c5a6f65527aa3b6f5d7cf25437a88d8727ab0646 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-815%2Fnewren%2Fort-conflict-handling-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-815/newren/ort-conflict-handling-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/815 Range-diff vs v1: 1: 382a009c18e ! 1: 1869e497482 merge-ort: handle D/F conflict where directory disappears due to merge @@ merge-ort.c: static void process_entry(struct merge_options *opt, die("Not yet implemented."); } - /* - * NOTE: Below there is a long switch-like if-elseif-elseif... block - * which the code goes through even for the df_conflict cases -- * above. Well, it will once we don't die-not-implemented above. -+ * above. - */ - if (ci->match_mask) { - ci->merged.clean = 1; 2: 46953226ba8 ! 2: 54f9be41a8a merge-ort: handle directory/file conflicts that remain @@ merge-ort.c: static void process_entry(struct merge_options *opt, } /* + * NOTE: Below there is a long switch-like if-elseif-elseif... block + * which the code goes through even for the df_conflict cases +- * above. Well, it will once we don't die-not-implemented above. ++ * above. + */ + if (ci->match_mask) { + ci->merged.clean = 1; 3: 6ac555b3c0f = 3: 63fed5e49a7 merge-ort: implement unique_path() helper 4: 4c641ec19d5 = 4: d0fab13c78a merge-ort: handle book-keeping around two- and three-way content merge 5: 0e7321e67f8 = 5: 69129a20edc merge-ort: flesh out implementation of handle_content_merge() 6: 611141f24af = 6: d1cc76ac620 merge-ort: copy and adapt merge_3way() from merge-recursive.c 7: 4696f6c2d95 = 7: 2ddf6ece9d0 merge-ort: copy and adapt merge_submodule() from merge-recursive.c 8: a640cc0effc = 8: b0bfada5d81 merge-ort: implement format_commit() 9: b898876b119 = 9: 334cc7c65a6 merge-ort: copy find_first_merges() implementation from merge-recursive.c 10: 0a5778df253 = 10: 34eb647df40 merge-ort: add handling for different types of files at same path -- gitgitgadget
From: Elijah Newren <newren@gmail.com> When one side has a directory at a given path and the other side of history has a file at the path, but the merge resolves the directory away (e.g. because no path within that directory was modified and the other side deleted it, or because renaming moved all the files elsewhere), then we don't actually have a conflict anymore. We just need to clear away any information related to the relevant directory, and then the subsequent process_entry() handling can handle the given path. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/merge-ort.c b/merge-ort.c index 414e7b7eeac..dd90987ae3d 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -976,7 +976,28 @@ static void process_entry(struct merge_options *opt, assert(ci->df_conflict); } - if (ci->df_conflict) { + if (ci->df_conflict && ci->merged.result.mode == 0) { + int i; + + /* + * directory no longer in the way, but we do have a file we + * need to place here so we need to clean away the "directory + * merges to nothing" result. + */ + ci->df_conflict = 0; + assert(ci->filemask != 0); + ci->merged.clean = 0; + ci->merged.is_null = 0; + /* and we want to zero out any directory-related entries */ + ci->match_mask = (ci->match_mask & ~ci->dirmask); + ci->dirmask = 0; + for (i = MERGE_BASE; i <= MERGE_SIDE2; i++) { + if (ci->filemask & (1 << i)) + continue; + ci->stages[i].mode = 0; + oidcpy(&ci->stages[i].oid, &null_oid); + } + } else if (ci->df_conflict && ci->merged.result.mode != 0) { die("Not yet implemented."); } -- gitgitgadget
From: Elijah Newren <newren@gmail.com> When a directory/file conflict remains, we can leave the directory where it is, but need to move the information about the file to a different pathname. After moving the file to a different pathname, we allow subsequent process_entry() logic to handle any additional details that might be relevant. This depends on a new helper function, unique_path(), that dies with an unimplemented error currently but will be implemented in a subsequent commit. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 84 insertions(+), 2 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index dd90987ae3d..d300a02810e 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -343,6 +343,13 @@ static void path_msg(struct merge_options *opt, strbuf_addch(sb, '\n'); } +static char *unique_path(struct strmap *existing_paths, + const char *path, + const char *branch) +{ + die("Not yet implemented."); +} + /*** Function Grouping: functions related to collect_merge_info() ***/ static void setup_path_info(struct merge_options *opt, @@ -962,6 +969,8 @@ static void process_entry(struct merge_options *opt, struct conflict_info *ci, struct directory_versions *dir_metadata) { + int df_file_index = 0; + VERIFY_CI(ci); assert(ci->filemask >= 0 && ci->filemask <= 7); /* ci->match_mask == 7 was handled in collect_merge_info_callback() */ @@ -998,13 +1007,86 @@ static void process_entry(struct merge_options *opt, oidcpy(&ci->stages[i].oid, &null_oid); } } else if (ci->df_conflict && ci->merged.result.mode != 0) { - die("Not yet implemented."); + /* + * This started out as a D/F conflict, and the entries in + * the competing directory were not removed by the merge as + * evidenced by write_completed_directory() writing a value + * to ci->merged.result.mode. + */ + struct conflict_info *new_ci; + const char *branch; + const char *old_path = path; + int i; + + assert(ci->merged.result.mode == S_IFDIR); + + /* + * If filemask is 1, we can just ignore the file as having + * been deleted on both sides. We do not want to overwrite + * ci->merged.result, since it stores the tree for all the + * files under it. + */ + if (ci->filemask == 1) { + ci->filemask = 0; + return; + } + + /* + * This file still exists on at least one side, and we want + * the directory to remain here, so we need to move this + * path to some new location. + */ + new_ci = xcalloc(1, sizeof(*new_ci)); + /* We don't really want new_ci->merged.result copied, but it'll + * be overwritten below so it doesn't matter. We also don't + * want any directory mode/oid values copied, but we'll zero + * those out immediately. We do want the rest of ci copied. + */ + memcpy(new_ci, ci, sizeof(*ci)); + new_ci->match_mask = (new_ci->match_mask & ~new_ci->dirmask); + new_ci->dirmask = 0; + for (i = MERGE_BASE; i <= MERGE_SIDE2; i++) { + if (new_ci->filemask & (1 << i)) + continue; + /* zero out any entries related to directories */ + new_ci->stages[i].mode = 0; + oidcpy(&new_ci->stages[i].oid, &null_oid); + } + + /* + * Find out which side this file came from; note that we + * cannot just use ci->filemask, because renames could cause + * the filemask to go back to 7. So we use dirmask, then + * pick the opposite side's index. + */ + df_file_index = (ci->dirmask & (1 << 1)) ? 2 : 1; + branch = (df_file_index == 1) ? opt->branch1 : opt->branch2; + path = unique_path(&opt->priv->paths, path, branch); + strmap_put(&opt->priv->paths, path, new_ci); + + path_msg(opt, path, 0, + _("CONFLICT (file/directory): directory in the way " + "of %s from %s; moving it to %s instead."), + old_path, branch, path); + + /* + * Zero out the filemask for the old ci. At this point, ci + * was just an entry for a directory, so we don't need to + * do anything more with it. + */ + ci->filemask = 0; + + /* + * Now note that we're working on the new entry (path was + * updated above. + */ + ci = new_ci; } /* * NOTE: Below there is a long switch-like if-elseif-elseif... block * which the code goes through even for the df_conflict cases - * above. Well, it will once we don't die-not-implemented above. + * above. */ if (ci->match_mask) { ci->merged.clean = 1; -- gitgitgadget
From: Elijah Newren <newren@gmail.com> Implement unique_path(), based on the one from merge-recursive.c. It is simplified, however, due to: (1) using strmaps, and (2) the fact that merge-ort lets the checkout codepath handle possible collisions with the working tree means that other code locations don't have to. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/merge-ort.c b/merge-ort.c index d300a02810e..1adc27a11bc 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -343,11 +343,34 @@ static void path_msg(struct merge_options *opt, strbuf_addch(sb, '\n'); } +/* add a string to a strbuf, but converting "/" to "_" */ +static void add_flattened_path(struct strbuf *out, const char *s) +{ + size_t i = out->len; + strbuf_addstr(out, s); + for (; i < out->len; i++) + if (out->buf[i] == '/') + out->buf[i] = '_'; +} + static char *unique_path(struct strmap *existing_paths, const char *path, const char *branch) { - die("Not yet implemented."); + struct strbuf newpath = STRBUF_INIT; + int suffix = 0; + size_t base_len; + + strbuf_addf(&newpath, "%s~", path); + add_flattened_path(&newpath, branch); + + base_len = newpath.len; + while (strmap_contains(existing_paths, newpath.buf)) { + strbuf_setlen(&newpath, base_len); + strbuf_addf(&newpath, "_%d", suffix++); + } + + return strbuf_detach(&newpath, NULL); } /*** Function Grouping: functions related to collect_merge_info() ***/ -- gitgitgadget
From: Elijah Newren <newren@gmail.com> In addition to the content merge (which will go in a subsequent commit), we need to worry about conflict messages, placing results in higher order stages in case of a df_conflict, and making sure the results are placed in ci->merged.result so that they will show up in the working tree. Take care of all that external book-keeping, moving the simplistic just-take-HEAD code into the barebones handle_content_merge() function for now. Subsequent commits will flesh out handle_content_merge(). Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 52 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index 1adc27a11bc..47e230fe341 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -640,7 +640,15 @@ static int handle_content_merge(struct merge_options *opt, const int extra_marker_size, struct version_info *result) { - die("Not yet implemented"); + int clean = 0; + /* + * TODO: Needs a two-way or three-way content merge, but we're + * just being lazy and copying the version from HEAD and + * leaving it as conflicted. + */ + result->mode = a->mode; + oidcpy(&result->oid, &a->oid); + return clean; } /*** Function Grouping: functions related to detect_and_process_renames(), *** @@ -1138,16 +1146,38 @@ static void process_entry(struct merge_options *opt, */ die("Not yet implemented."); } else if (ci->filemask >= 6) { - /* - * TODO: Needs a two-way or three-way content merge, but we're - * just being lazy and copying the version from HEAD and - * leaving it as conflicted. - */ - ci->merged.clean = 0; - ci->merged.result.mode = ci->stages[1].mode; - oidcpy(&ci->merged.result.oid, &ci->stages[1].oid); - /* When we fix above, we'll call handle_content_merge() */ - (void)handle_content_merge; + /* Need a two-way or three-way content merge */ + struct version_info merged_file; + unsigned clean_merge; + struct version_info *o = &ci->stages[0]; + struct version_info *a = &ci->stages[1]; + struct version_info *b = &ci->stages[2]; + + clean_merge = handle_content_merge(opt, path, o, a, b, + ci->pathnames, + opt->priv->call_depth * 2, + &merged_file); + ci->merged.clean = clean_merge && + !ci->df_conflict && !ci->path_conflict; + ci->merged.result.mode = merged_file.mode; + ci->merged.is_null = (merged_file.mode == 0); + oidcpy(&ci->merged.result.oid, &merged_file.oid); + if (clean_merge && ci->df_conflict) { + assert(df_file_index == 1 || df_file_index == 2); + ci->filemask = 1 << df_file_index; + ci->stages[df_file_index].mode = merged_file.mode; + oidcpy(&ci->stages[df_file_index].oid, &merged_file.oid); + } + if (!clean_merge) { + const char *reason = _("content"); + if (ci->filemask == 6) + reason = _("add/add"); + if (S_ISGITLINK(merged_file.mode)) + reason = _("submodule"); + path_msg(opt, path, 0, + _("CONFLICT (%s): Merge conflict in %s"), + reason, path); + } } else if (ci->filemask == 3 || ci->filemask == 5) { /* Modify/delete */ const char *modify_branch, *delete_branch; -- gitgitgadget
From: Elijah Newren <newren@gmail.com> This implementation is based heavily on merge_mode_and_contents() from merge-recursive.c, though it has some fixes for recursive merges (i.e. when call_depth > 0), and has a number of changes throughout based on slight differences in data structures and in how the functions are called. It is, however, based on two new helper functions -- merge_3way() and merge_submodule -- for which we only provide die-not-implemented stubs at this point. Future commits will add implementations of these functions. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 143 insertions(+), 6 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index 47e230fe341..2cfb7ffa3b0 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -631,6 +631,28 @@ static int collect_merge_info(struct merge_options *opt, /*** Function Grouping: functions related to threeway content merges ***/ +static int merge_submodule(struct merge_options *opt, + const char *path, + const struct object_id *o, + const struct object_id *a, + const struct object_id *b, + struct object_id *result) +{ + die("Not yet implemented."); +} + +static int merge_3way(struct merge_options *opt, + const char *path, + const struct object_id *o, + const struct object_id *a, + const struct object_id *b, + const char *pathnames[3], + const int extra_marker_size, + mmbuffer_t *result_buf) +{ + die("Not yet implemented."); +} + static int handle_content_merge(struct merge_options *opt, const char *path, const struct version_info *o, @@ -640,14 +662,129 @@ static int handle_content_merge(struct merge_options *opt, const int extra_marker_size, struct version_info *result) { - int clean = 0; /* - * TODO: Needs a two-way or three-way content merge, but we're - * just being lazy and copying the version from HEAD and - * leaving it as conflicted. + * path is the target location where we want to put the file, and + * is used to determine any normalization rules in ll_merge. + * + * The normal case is that path and all entries in pathnames are + * identical, though renames can affect which path we got one of + * the three blobs to merge on various sides of history. + * + * extra_marker_size is the amount to extend conflict markers in + * ll_merge; this is neeed if we have content merges of content + * merges, which happens for example with rename/rename(2to1) and + * rename/add conflicts. */ - result->mode = a->mode; - oidcpy(&result->oid, &a->oid); + unsigned clean = 1; + + /* + * handle_content_merge() needs both files to be of the same type, i.e. + * both files OR both submodules OR both symlinks. Conflicting types + * needs to be handled elsewhere. + */ + assert((S_IFMT & a->mode) == (S_IFMT & b->mode)); + + /* Merge modes */ + if (a->mode == b->mode || a->mode == o->mode) + result->mode = b->mode; + else { + /* must be the 100644/100755 case */ + assert(S_ISREG(a->mode)); + result->mode = a->mode; + clean = (b->mode == o->mode); + /* + * FIXME: If opt->priv->call_depth && !clean, then we really + * should not make result->mode match either a->mode or + * b->mode; that causes t6036 "check conflicting mode for + * regular file" to fail. It would be best to use some other + * mode, but we'll confuse all kinds of stuff if we use one + * where S_ISREG(result->mode) isn't true, and if we use + * something like 0100666, then tree-walk.c's calls to + * canon_mode() will just normalize that to 100644 for us and + * thus not solve anything. + * + * Figure out if there's some kind of way we can work around + * this... + */ + } + + /* + * Trivial oid merge. + * + * Note: While one might assume that the next four lines would + * be unnecessary due to the fact that match_mask is often + * setup and already handled, renames don't always take care + * of that. + */ + if (oideq(&a->oid, &b->oid) || oideq(&a->oid, &o->oid)) + oidcpy(&result->oid, &b->oid); + else if (oideq(&b->oid, &o->oid)) + oidcpy(&result->oid, &a->oid); + + /* Remaining rules depend on file vs. submodule vs. symlink. */ + else if (S_ISREG(a->mode)) { + mmbuffer_t result_buf; + int ret = 0, merge_status; + int two_way; + + /* + * If 'o' is different type, treat it as null so we do a + * two-way merge. + */ + two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode)); + + merge_status = merge_3way(opt, path, + two_way ? &null_oid : &o->oid, + &a->oid, &b->oid, + pathnames, extra_marker_size, + &result_buf); + + if ((merge_status < 0) || !result_buf.ptr) + ret = err(opt, _("Failed to execute internal merge")); + + if (!ret && + write_object_file(result_buf.ptr, result_buf.size, + blob_type, &result->oid)) + ret = err(opt, _("Unable to add %s to database"), + path); + + free(result_buf.ptr); + if (ret) + return -1; + clean &= (merge_status == 0); + path_msg(opt, path, 1, _("Auto-merging %s"), path); + } else if (S_ISGITLINK(a->mode)) { + int two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode)); + clean = merge_submodule(opt, pathnames[0], + two_way ? &null_oid : &o->oid, + &a->oid, &b->oid, &result->oid); + if (opt->priv->call_depth && two_way && !clean) { + result->mode = o->mode; + oidcpy(&result->oid, &o->oid); + } + } else if (S_ISLNK(a->mode)) { + if (opt->priv->call_depth) { + clean = 0; + result->mode = o->mode; + oidcpy(&result->oid, &o->oid); + } else { + switch (opt->recursive_variant) { + case MERGE_VARIANT_NORMAL: + clean = 0; + oidcpy(&result->oid, &a->oid); + break; + case MERGE_VARIANT_OURS: + oidcpy(&result->oid, &a->oid); + break; + case MERGE_VARIANT_THEIRS: + oidcpy(&result->oid, &b->oid); + break; + } + } + } else + BUG("unsupported object type in the tree: %06o for %s", + a->mode, path); + return clean; } -- gitgitgadget
From: Elijah Newren <newren@gmail.com> Take merge_3way() from merge-recursive.c and make slight adjustments based on different data structures (direct usage of object_id rather diff_filespec, separate pathnames which based on our careful interning of pathnames in opt->priv->paths can be compared with '!=' rather than 'strcmp'). Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/merge-ort.c b/merge-ort.c index 2cfb7ffa3b0..a59adb42aa6 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -23,6 +23,7 @@ #include "diff.h" #include "diffcore.h" #include "dir.h" +#include "ll-merge.h" #include "object-store.h" #include "strmap.h" #include "tree.h" @@ -650,7 +651,58 @@ static int merge_3way(struct merge_options *opt, const int extra_marker_size, mmbuffer_t *result_buf) { - die("Not yet implemented."); + mmfile_t orig, src1, src2; + struct ll_merge_options ll_opts = {0}; + char *base, *name1, *name2; + int merge_status; + + ll_opts.renormalize = opt->renormalize; + ll_opts.extra_marker_size = extra_marker_size; + ll_opts.xdl_opts = opt->xdl_opts; + + if (opt->priv->call_depth) { + ll_opts.virtual_ancestor = 1; + ll_opts.variant = 0; + } else { + switch (opt->recursive_variant) { + case MERGE_VARIANT_OURS: + ll_opts.variant = XDL_MERGE_FAVOR_OURS; + break; + case MERGE_VARIANT_THEIRS: + ll_opts.variant = XDL_MERGE_FAVOR_THEIRS; + break; + default: + ll_opts.variant = 0; + break; + } + } + + assert(pathnames[0] && pathnames[1] && pathnames[2] && opt->ancestor); + if (pathnames[0] == pathnames[1] && pathnames[1] == pathnames[2]) { + base = mkpathdup("%s", opt->ancestor); + name1 = mkpathdup("%s", opt->branch1); + name2 = mkpathdup("%s", opt->branch2); + } else { + base = mkpathdup("%s:%s", opt->ancestor, pathnames[0]); + name1 = mkpathdup("%s:%s", opt->branch1, pathnames[1]); + name2 = mkpathdup("%s:%s", opt->branch2, pathnames[2]); + } + + read_mmblob(&orig, o); + read_mmblob(&src1, a); + read_mmblob(&src2, b); + + merge_status = ll_merge(result_buf, path, &orig, base, + &src1, name1, &src2, name2, + opt->repo->index, &ll_opts); + + free(base); + free(name1); + free(name2); + free(orig.ptr); + free(src1.ptr); + free(src2.ptr); + return merge_status; } static int handle_content_merge(struct merge_options *opt, -- gitgitgadget
From: Elijah Newren <newren@gmail.com> Take merge_submodule() from merge-recursive.c and make slight adjustments, predominantly around deferring output using path_msg() instead of using merge-recursive's output() and show() functions. There's also a fix for recursive cases (when call_depth > 0) and a slight change to argument order for find_first_merges(). find_first_merges() and format_commit() are left unimplemented for now, but will be added by subsequent commits. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-) diff --git a/merge-ort.c b/merge-ort.c index a59adb42aa6..2dfab1858fc 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -26,6 +26,7 @@ #include "ll-merge.h" #include "object-store.h" #include "strmap.h" +#include "submodule.h" #include "tree.h" #include "unpack-trees.h" #include "xdiff-interface.h" @@ -323,6 +324,13 @@ static int err(struct merge_options *opt, const char *err, ...) return -1; } +static void format_commit(struct strbuf *sb, + int indent, + struct commit *commit) +{ + die("Not yet implemented."); +} + __attribute__((format (printf, 4, 5))) static void path_msg(struct merge_options *opt, const char *path, @@ -632,6 +640,15 @@ static int collect_merge_info(struct merge_options *opt, /*** Function Grouping: functions related to threeway content merges ***/ +static int find_first_merges(struct repository *repo, + const char *path, + struct commit *a, + struct commit *b, + struct object_array *result) +{ + die("Not yet implemented."); +} + static int merge_submodule(struct merge_options *opt, const char *path, const struct object_id *o, @@ -639,7 +656,114 @@ static int merge_submodule(struct merge_options *opt, const struct object_id *b, struct object_id *result) { - die("Not yet implemented."); + struct commit *commit_o, *commit_a, *commit_b; + int parent_count; + struct object_array merges; + struct strbuf sb = STRBUF_INIT; + + int i; + int search = !opt->priv->call_depth; + + /* store fallback answer in result in case we fail */ + oidcpy(result, opt->priv->call_depth ? o : a); + + /* we can not handle deletion conflicts */ + if (is_null_oid(o)) + return 0; + if (is_null_oid(a)) + return 0; + if (is_null_oid(b)) + return 0; + + if (add_submodule_odb(path)) { + path_msg(opt, path, 0, + _("Failed to merge submodule %s (not checked out)"), + path); + return 0; + } + + if (!(commit_o = lookup_commit_reference(opt->repo, o)) || + !(commit_a = lookup_commit_reference(opt->repo, a)) || + !(commit_b = lookup_commit_reference(opt->repo, b))) { + path_msg(opt, path, 0, + _("Failed to merge submodule %s (commits not present)"), + path); + return 0; + } + + /* check whether both changes are forward */ + if (!in_merge_bases(commit_o, commit_a) || + !in_merge_bases(commit_o, commit_b)) { + path_msg(opt, path, 0, + _("Failed to merge submodule %s " + "(commits don't follow merge-base)"), + path); + return 0; + } + + /* Case #1: a is contained in b or vice versa */ + if (in_merge_bases(commit_a, commit_b)) { + oidcpy(result, b); + path_msg(opt, path, 1, + _("Note: Fast-forwarding submodule %s to %s"), + path, oid_to_hex(b)); + return 1; + } + if (in_merge_bases(commit_b, commit_a)) { + oidcpy(result, a); + path_msg(opt, path, 1, + _("Note: Fast-forwarding submodule %s to %s"), + path, oid_to_hex(a)); + return 1; + } + + /* + * Case #2: There are one or more merges that contain a and b in + * the submodule. If there is only one, then present it as a + * suggestion to the user, but leave it marked unmerged so the + * user needs to confirm the resolution. + */ + + /* Skip the search if makes no sense to the calling context. */ + if (!search) + return 0; + + /* find commit which merges them */ + parent_count = find_first_merges(opt->repo, path, commit_a, commit_b, + &merges); + switch (parent_count) { + case 0: + path_msg(opt, path, 0, _("Failed to merge submodule %s"), path); + break; + + case 1: + format_commit(&sb, 4, + (struct commit *)merges.objects[0].item); + path_msg(opt, path, 0, + _("Failed to merge submodule %s, but a possible merge " + "resolution exists:\n%s\n"), + path, sb.buf); + path_msg(opt, path, 1, + _("If this is correct simply add it to the index " + "for example\n" + "by using:\n\n" + " git update-index --cacheinfo 160000 %s \"%s\"\n\n" + "which will accept this suggestion.\n"), + oid_to_hex(&merges.objects[0].item->oid), path); + strbuf_release(&sb); + break; + default: + for (i = 0; i < merges.nr; i++) + format_commit(&sb, 4, + (struct commit *)merges.objects[i].item); + path_msg(opt, path, 0, + _("Failed to merge submodule %s, but multiple " + "possible merges exist:\n%s"), path, sb.buf); + strbuf_release(&sb); + } + + object_array_clear(&merges); + return 0; } static int merge_3way(struct merge_options *opt, -- gitgitgadget
From: Elijah Newren <newren@gmail.com> This implementation is based on a mixture of print_commit() and output_commit_title() from merge-recursive.c so that it can be used to take over both functions. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/merge-ort.c b/merge-ort.c index 2dfab1858fc..bf704bcd34d 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -328,7 +328,19 @@ static void format_commit(struct strbuf *sb, int indent, struct commit *commit) { - die("Not yet implemented."); + struct merge_remote_desc *desc; + struct pretty_print_context ctx = {0}; + ctx.abbrev = DEFAULT_ABBREV; + + strbuf_addchars(sb, ' ', indent); + desc = merge_remote_util(commit); + if (desc) { + strbuf_addf(sb, "virtual %s\n", desc->name); + return; + } + + format_commit_message(commit, "%h %s", sb, &ctx); + strbuf_addch(sb, '\n'); } __attribute__((format (printf, 4, 5))) -- gitgitgadget
From: Elijah Newren <newren@gmail.com> Code is identical for the function body in the two files, the call signature is just slightly different in merge-ort than merge-recursive as noted a couple commits ago. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/merge-ort.c b/merge-ort.c index bf704bcd34d..203fa987e43 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -25,6 +25,7 @@ #include "dir.h" #include "ll-merge.h" #include "object-store.h" +#include "revision.h" #include "strmap.h" #include "submodule.h" #include "tree.h" @@ -658,7 +659,61 @@ static int find_first_merges(struct repository *repo, struct commit *b, struct object_array *result) { - die("Not yet implemented."); + int i, j; + struct object_array merges = OBJECT_ARRAY_INIT; + struct commit *commit; + int contains_another; + + char merged_revision[GIT_MAX_HEXSZ + 2]; + const char *rev_args[] = { "rev-list", "--merges", "--ancestry-path", + "--all", merged_revision, NULL }; + struct rev_info revs; + struct setup_revision_opt rev_opts; + + memset(result, 0, sizeof(struct object_array)); + memset(&rev_opts, 0, sizeof(rev_opts)); + + /* get all revisions that merge commit a */ + xsnprintf(merged_revision, sizeof(merged_revision), "^%s", + oid_to_hex(&a->object.oid)); + repo_init_revisions(repo, &revs, NULL); + rev_opts.submodule = path; + /* FIXME: can't handle linked worktrees in submodules yet */ + revs.single_worktree = path != NULL; + setup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts); + + /* save all revisions from the above list that contain b */ + if (prepare_revision_walk(&revs)) + die("revision walk setup failed"); + while ((commit = get_revision(&revs)) != NULL) { + struct object *o = &(commit->object); + if (in_merge_bases(b, commit)) + add_object_array(o, NULL, &merges); + } + reset_revision_walk(); + + /* Now we've got all merges that contain a and b. Prune all + * merges that contain another found merge and save them in + * result. + */ + for (i = 0; i < merges.nr; i++) { + struct commit *m1 = (struct commit *) merges.objects[i].item; + + contains_another = 0; + for (j = 0; j < merges.nr; j++) { + struct commit *m2 = (struct commit *) merges.objects[j].item; + if (i != j && in_merge_bases(m2, m1)) { + contains_another = 1; + break; + } + } + + if (!contains_another) + add_object_array(merges.objects[i].item, NULL, result); + } + + object_array_clear(&merges); + return result->nr; } static int merge_submodule(struct merge_options *opt, -- gitgitgadget
From: Elijah Newren <newren@gmail.com> Add some handling that explicitly considers collisions of the following types: * file/submodule * file/symlink * submodule/symlink Leaving them as conflicts at the same path are hard for users to resolve, so move one or both of them aside so that they each get their own path. Note that in the case of recursive handling (i.e. call_depth > 0), we can just use the merge base of the two merge bases as the merge result much like we do with modify/delete conflicts, binary files, conflicting submodule values, and so on. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 103 insertions(+), 4 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index 203fa987e43..afe721182e2 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -1521,10 +1521,109 @@ static void process_entry(struct merge_options *opt, } else if (ci->filemask >= 6 && (S_IFMT & ci->stages[1].mode) != (S_IFMT & ci->stages[2].mode)) { - /* - * Two different items from (file/submodule/symlink) - */ - die("Not yet implemented."); + /* Two different items from (file/submodule/symlink) */ + if (opt->priv->call_depth) { + /* Just use the version from the merge base */ + ci->merged.clean = 0; + oidcpy(&ci->merged.result.oid, &ci->stages[0].oid); + ci->merged.result.mode = ci->stages[0].mode; + ci->merged.is_null = (ci->merged.result.mode == 0); + } else { + /* Handle by renaming one or both to separate paths. */ + unsigned o_mode = ci->stages[0].mode; + unsigned a_mode = ci->stages[1].mode; + unsigned b_mode = ci->stages[2].mode; + struct conflict_info *new_ci; + const char *a_path = NULL, *b_path = NULL; + int rename_a = 0, rename_b = 0; + + new_ci = xmalloc(sizeof(*new_ci)); + + if (S_ISREG(a_mode)) + rename_a = 1; + else if (S_ISREG(b_mode)) + rename_b = 1; + else { + rename_a = 1; + rename_b = 1; + } + + path_msg(opt, path, 0, + _("CONFLICT (distinct types): %s had different " + "types on each side; renamed %s of them so " + "each can be recorded somewhere."), + path, + (rename_a && rename_b) ? _("both") : _("one")); + + ci->merged.clean = 0; + memcpy(new_ci, ci, sizeof(*new_ci)); + + /* Put b into new_ci, removing a from stages */ + new_ci->merged.result.mode = ci->stages[2].mode; + oidcpy(&new_ci->merged.result.oid, &ci->stages[2].oid); + new_ci->stages[1].mode = 0; + oidcpy(&new_ci->stages[1].oid, &null_oid); + new_ci->filemask = 5; + if ((S_IFMT & b_mode) != (S_IFMT & o_mode)) { + new_ci->stages[0].mode = 0; + oidcpy(&new_ci->stages[0].oid, &null_oid); + new_ci->filemask = 4; + } + + /* Leave only a in ci, fixing stages. */ + ci->merged.result.mode = ci->stages[1].mode; + oidcpy(&ci->merged.result.oid, &ci->stages[1].oid); + ci->stages[2].mode = 0; + oidcpy(&ci->stages[2].oid, &null_oid); + ci->filemask = 3; + if ((S_IFMT & a_mode) != (S_IFMT & o_mode)) { + ci->stages[0].mode = 0; + oidcpy(&ci->stages[0].oid, &null_oid); + ci->filemask = 2; + } + + /* Insert entries into opt->priv_paths */ + assert(rename_a || rename_b); + if (rename_a) { + a_path = unique_path(&opt->priv->paths, + path, opt->branch1); + strmap_put(&opt->priv->paths, a_path, ci); + } + + if (rename_b) + b_path = unique_path(&opt->priv->paths, + path, opt->branch2); + else + b_path = path; + strmap_put(&opt->priv->paths, b_path, new_ci); + + if (rename_a && rename_b) { + strmap_remove(&opt->priv->paths, path, 0); + /* + * We removed path from opt->priv->paths. path + * will also eventually need to be freed, but + * it may still be used by e.g. ci->pathnames. + * So, store it in another string-list for now. + */ + string_list_append(&opt->priv->paths_to_free, + path); + } + + /* + * Do special handling for b_path since process_entry() + * won't be called on it specially. + */ + strmap_put(&opt->priv->conflicted, b_path, new_ci); + record_entry_for_tree(dir_metadata, b_path, + &new_ci->merged); + + /* + * Remaining code for processing this entry should + * think in terms of processing a_path. + */ + if (a_path) + path = a_path; + } } else if (ci->filemask >= 6) { /* Need a two-way or three-way content merge */ struct version_info merged_file; -- gitgitgadget
On 12/31/2020 9:34 PM, Elijah Newren via GitGitGadget wrote:
> This series depends on en/merge-ort-2 (it does not depend on en/merge-ort-3
> or en/merge-ort-recursive).
>
> This series adds handling of additional basic conflict types (directory/file
> conflicts, three-way content merging, very basic submodule divergence
> reconciliation, and different filetypes). This series drops the number of
> test failures under GIT_TEST_MERGE_ALGORITHM=ort by 211 (from 1448 to 1237).
>
> Further, if en/merge-tests, en/merge-ort-3, en/merge-ort-recursive, and this
> series are all merged down (in any order), then collectively they drop the
> number of test failure under GIT_TEST_MERGE_ALGORITHM=ort from 1448 down to
> 60.
I finished reading the rest of the patches. They are well-organized to
present the merging logic in small chunks.
While this is a very dense series, the proof is in the test cases. I
look forward to testing the ort algorithm in CI builds and start enabling
it in my repositories.
Your patch organization will help if there are bugs that we won't catch
until we can enable this merge algorithm more widely, as we can blame to
these well-crafted patches to assist with debugging.
Thanks,
-Stolee
On Tue, Jan 5, 2021 at 6:23 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 12/31/2020 9:34 PM, Elijah Newren via GitGitGadget wrote:
> > This series depends on en/merge-ort-2 (it does not depend on en/merge-ort-3
> > or en/merge-ort-recursive).
> >
> > This series adds handling of additional basic conflict types (directory/file
> > conflicts, three-way content merging, very basic submodule divergence
> > reconciliation, and different filetypes). This series drops the number of
> > test failures under GIT_TEST_MERGE_ALGORITHM=ort by 211 (from 1448 to 1237).
> >
> > Further, if en/merge-tests, en/merge-ort-3, en/merge-ort-recursive, and this
> > series are all merged down (in any order), then collectively they drop the
> > number of test failure under GIT_TEST_MERGE_ALGORITHM=ort from 1448 down to
> > 60.
>
> I finished reading the rest of the patches. They are well-organized to
> present the merging logic in small chunks.
>
> While this is a very dense series, the proof is in the test cases. I
> look forward to testing the ort algorithm in CI builds and start enabling
> it in my repositories.
>
> Your patch organization will help if there are bugs that we won't catch
> until we can enable this merge algorithm more widely, as we can blame to
> these well-crafted patches to assist with debugging.
Thanks for taking a look, as always.
Your comments reminded me that I intended to dig into your code
coverage reports and try to figure out if there are parts of
merge-ort.[ch] (and diffcore-rename.c) that are uncovered by any
testcases. I bet there are some. There were certainly many in
merge-recursive, and while I've extended the testsuite coverage over
the years, I never did it with the aid of a code coverage tool...
On Fri, Jan 01 2021, Elijah Newren via GitGitGadget wrote:
> + else {
> + /* must be the 100644/100755 case */
> + assert(S_ISREG(a->mode));
> + result->mode = a->mode;
> + clean = (b->mode == o->mode);
> + /*
> + * FIXME: If opt->priv->call_depth && !clean, then we really
> + * should not make result->mode match either a->mode or
> + * b->mode; that causes t6036 "check conflicting mode for
> + * regular file" to fail. It would be best to use some other
> + * mode, but we'll confuse all kinds of stuff if we use one
> + * where S_ISREG(result->mode) isn't true, and if we use
> + * something like 0100666, then tree-walk.c's calls to
> + * canon_mode() will just normalize that to 100644 for us and
> + * thus not solve anything.
> + *
> + * Figure out if there's some kind of way we can work around
> + * this...
> + */
So if tree-walk.c didn't call canon_mode() you would do:
if (opt->priv->call_depth && !clean)
result->mode = 0100666;
else
result->mode = a->mode;
I haven't looked at this bit closer, but that doesn't make the test
referenced here pass.
I'm refactoring tree-walk.h to do that in a WIP series, and ran into
this.
As an aside, how does one run the merge-ort tests in such a way as
they'll pass on master now? There's now a bunch of failures with
GIT_TEST_MERGE_ALGORITHM=ort, well, just for t*merge*.sh:
t6409-merge-subtree.sh (Wstat: 256 Tests: 12 Failed: 1)
Failed test: 12
Non-zero exit status: 1
t6418-merge-text-auto.sh (Wstat: 256 Tests: 10 Failed: 3)
Failed tests: 4-5, 10
Non-zero exit status: 1
t6437-submodule-merge.sh (Wstat: 0 Tests: 18 Failed: 0)
TODO passed: 13, 17
t6423-merge-rename-directories.sh (Wstat: 256 Tests: 68 Failed: 4)
Failed tests: 7, 53, 55, 59
Non-zero exit status: 1
And both test_expect_merge_algorithm and what seems to be a common
pattern of e.g.:
t6400-merge-df.sh: if test "$GIT_TEST_MERGE_ALGORITHM" = ort
t6400-merge-df.sh- then
t6400-merge-df.sh- test 0 -eq $(git ls-files -o | wc -l)
t6400-merge-df.sh- else
t6400-merge-df.sh- test 1 -eq $(git ls-files -o | wc -l)
t6400-merge-df.sh- fi &&
Will not run tests on both backends, I was expecting to find something
so we can the test N times for the backends, and declared if things were
to be skipped on ort or whatever.
I understand that this is still WIP code, but it would be nice to have
it in a state where one can confidently touch merge-ort.c when changing
some API or whatever and have it be tested by default.
Or maybe that's the case, and I've missed how it's happening...
Hi Ævar, On Thu, Mar 4, 2021 at 8:28 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Fri, Jan 01 2021, Elijah Newren via GitGitGadget wrote: > > > + else { > > + /* must be the 100644/100755 case */ > > + assert(S_ISREG(a->mode)); > > + result->mode = a->mode; > > + clean = (b->mode == o->mode); > > + /* > > + * FIXME: If opt->priv->call_depth && !clean, then we really > > + * should not make result->mode match either a->mode or > > + * b->mode; that causes t6036 "check conflicting mode for > > + * regular file" to fail. It would be best to use some other > > + * mode, but we'll confuse all kinds of stuff if we use one > > + * where S_ISREG(result->mode) isn't true, and if we use > > + * something like 0100666, then tree-walk.c's calls to > > + * canon_mode() will just normalize that to 100644 for us and > > + * thus not solve anything. > > + * > > + * Figure out if there's some kind of way we can work around > > + * this... > > + */ > > So if tree-walk.c didn't call canon_mode() you would do: > > if (opt->priv->call_depth && !clean) > result->mode = 0100666; > else > result->mode = a->mode; > > I haven't looked at this bit closer, but that doesn't make the test > referenced here pass. > > I'm refactoring tree-walk.h to do that in a WIP series, and ran into > this. Interesting. Yeah, there might be more steps to make that particular test work, but I couldn't go any further due to canon_mode(). It's a testcase that has always failed under merge-recursive, and which I was resigned to always have fail under merge-ort too; I suspect it's enough of a corner case that no one but me ever really cared before. (And I didn't hit it in the wild or know anyone that did, I just learned of it by trying to clean up merge-recursive.) > As an aside, how does one run the merge-ort tests in such a way as > they'll pass on master now? There's now a bunch of failures with > GIT_TEST_MERGE_ALGORITHM=ort, well, just for t*merge*.sh: > > t6409-merge-subtree.sh (Wstat: 256 Tests: 12 Failed: 1) > Failed test: 12 > Non-zero exit status: 1 > t6418-merge-text-auto.sh (Wstat: 256 Tests: 10 Failed: 3) > Failed tests: 4-5, 10 > Non-zero exit status: 1 > t6437-submodule-merge.sh (Wstat: 0 Tests: 18 Failed: 0) > TODO passed: 13, 17 > t6423-merge-rename-directories.sh (Wstat: 256 Tests: 68 Failed: 4) > Failed tests: 7, 53, 55, 59 > Non-zero exit status: 1 Right, I've been sending merge-ort upstream as fast as possible since last September or so, but there's only so much reviewer bandwidth so I've been forced to hold back on dozens of patches. Currently there are 8 test failures (all shown in your output here -- 1 in t6409, 3 in t6418, and 4 in t6423), and 12 TODO passed (only two of which you show here). I was forced to switch my ordering of sending patches upstream late last year due to an intern project that was planned to do significant work within diffcore-rename; I was worried about major conflicts, so I needed to get the diffcore-rename changes upstream earlier. That's still in-process. By the way, if you'd like to help accelerate the merge-ort work; it's almost entirely review bound. https://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/ still has no comments, then I have optimization series 10-14 to send (viewable up at https://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch), then I have other fixes -- mostly for the testsuite (viewable at https://github.com/newren/git/tree/ort-remainder), then I need to fix up the TODO passed submodule tests. Due to how the submodule testing framework functions, I can't just make a simple s/test_expect_failure/test_expect_success/ -- the tests are structured a bit funny and the tests are themselves buggy in some cases. I talked with jrnieder about it a little bit, just need to spend more time on it. But it hasn't been critical because the rest of the code was so far away from finally landing anyway. Finally, and optionally, comes the --remerge-diff and --remerge-diff-only options to log/show (viewable at https://github.com/newren/git/tree/remerge-diff, but these patches need to both be cleaned up and rebased on ort-remainder). > And both test_expect_merge_algorithm and what seems to be a common > pattern of e.g.: > > t6400-merge-df.sh: if test "$GIT_TEST_MERGE_ALGORITHM" = ort > t6400-merge-df.sh- then > t6400-merge-df.sh- test 0 -eq $(git ls-files -o | wc -l) > t6400-merge-df.sh- else > t6400-merge-df.sh- test 1 -eq $(git ls-files -o | wc -l) > t6400-merge-df.sh- fi && > > Will not run tests on both backends, I was expecting to find something > so we can the test N times for the backends, and declared if things were > to be skipped on ort or whatever. Yeah, multiple ways of testing were discussed mid last year. There were lots of tradeoffs. I think the thing that pushed in this direction is that we're not just aiming to add another optional merge backend, we're aiming to replace merge-recursive entirely. Since merge tests appear all throughout the code base, many as rebase or cherry-pick or revert or stash tests...or just as simple setup tests, we want all of those tested with the new backend. Trying to duplicate all those tests in any way other than just re-running the testsuite with a different knob would require huge changes to hundreds (thousands?) of testfiles and conflict with nearly every other topic. So I made an environment variable that would choose which backend to use, but with the downside of having to re-run the testsuite again. > I understand that this is still WIP code, but it would be nice to have > it in a state where one can confidently touch merge-ort.c when changing > some API or whatever and have it be tested by default. Thanks for proactively checking. To make it easier for you, I'll see if I can submit a series later today that mostly completes the merge-ort implementation; the t6423 testcases won't be fixed until "Optimization batch 12" lands, and I might not be able to fix the "TODO passed" submodule tests in this series, but the rest of the stuff can be fixed with about 10-12 patches. I had been worried about overloading the list with too many patches at once, but since it sounds like you're willing to review these particular patches... :-) > Or maybe that's the case, and I've missed how it's happening...
On Thu, Mar 04 2021, Elijah Newren wrote: > Hi Ævar, > > On Thu, Mar 4, 2021 at 8:28 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> >> On Fri, Jan 01 2021, Elijah Newren via GitGitGadget wrote: >> >> > + else { >> > + /* must be the 100644/100755 case */ >> > + assert(S_ISREG(a->mode)); >> > + result->mode = a->mode; >> > + clean = (b->mode == o->mode); >> > + /* >> > + * FIXME: If opt->priv->call_depth && !clean, then we really >> > + * should not make result->mode match either a->mode or >> > + * b->mode; that causes t6036 "check conflicting mode for >> > + * regular file" to fail. It would be best to use some other >> > + * mode, but we'll confuse all kinds of stuff if we use one >> > + * where S_ISREG(result->mode) isn't true, and if we use >> > + * something like 0100666, then tree-walk.c's calls to >> > + * canon_mode() will just normalize that to 100644 for us and >> > + * thus not solve anything. >> > + * >> > + * Figure out if there's some kind of way we can work around >> > + * this... >> > + */ >> >> So if tree-walk.c didn't call canon_mode() you would do: >> >> if (opt->priv->call_depth && !clean) >> result->mode = 0100666; >> else >> result->mode = a->mode; >> >> I haven't looked at this bit closer, but that doesn't make the test >> referenced here pass. >> >> I'm refactoring tree-walk.h to do that in a WIP series, and ran into >> this. > > Interesting. Yeah, there might be more steps to make that particular > test work, but I couldn't go any further due to canon_mode(). It's a > testcase that has always failed under merge-recursive, and which I was > resigned to always have fail under merge-ort too; I suspect it's > enough of a corner case that no one but me ever really cared before. > (And I didn't hit it in the wild or know anyone that did, I just > learned of it by trying to clean up merge-recursive.) I'll send those patches out sooner than later, but as a quick question, for merges / writing new files to the index/trees etc. we basically: 1. sanitize the mode with canon_mode() 2. write it to a new object, either index or TREE object I've been trying to refactor things so those things have canon_mode() as close to the time of writing as possible. Well, mostly to replace the whole S_*(mode) macros all over the place with checks against "enum object_type", which is what most of it wants anyway. Do you think the merge logic generally wants to operate on the "raw" mode bits (including what may not even pass fsck checks), or the sanitized canon_mode()? >> As an aside, how does one run the merge-ort tests in such a way as >> they'll pass on master now? There's now a bunch of failures with >> GIT_TEST_MERGE_ALGORITHM=ort, well, just for t*merge*.sh: >> >> t6409-merge-subtree.sh (Wstat: 256 Tests: 12 Failed: 1) >> Failed test: 12 >> Non-zero exit status: 1 >> t6418-merge-text-auto.sh (Wstat: 256 Tests: 10 Failed: 3) >> Failed tests: 4-5, 10 >> Non-zero exit status: 1 >> t6437-submodule-merge.sh (Wstat: 0 Tests: 18 Failed: 0) >> TODO passed: 13, 17 >> t6423-merge-rename-directories.sh (Wstat: 256 Tests: 68 Failed: 4) >> Failed tests: 7, 53, 55, 59 >> Non-zero exit status: 1 > > Right, I've been sending merge-ort upstream as fast as possible since > last September or so, but there's only so much reviewer bandwidth so > I've been forced to hold back on dozens of patches. > > Currently there are 8 test failures (all shown in your output here -- > 1 in t6409, 3 in t6418, and 4 in t6423), and 12 TODO passed (only two > of which you show here). I was forced to switch my ordering of > sending patches upstream late last year due to an intern project that > was planned to do significant work within diffcore-rename; I was > worried about major conflicts, so I needed to get the diffcore-rename > changes upstream earlier. That's still in-process. > > By the way, if you'd like to help accelerate the merge-ort work; it's > almost entirely review bound. > https://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/ > still has no comments, then I have optimization series 10-14 to send > (viewable up at > https://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch), > then I have other fixes -- mostly for the testsuite (viewable at > https://github.com/newren/git/tree/ort-remainder), then I need to fix > up the TODO passed submodule tests. Due to how the submodule testing > framework functions, I can't just make a simple > s/test_expect_failure/test_expect_success/ -- the tests are structured > a bit funny and the tests are themselves buggy in some cases. I > talked with jrnieder about it a little bit, just need to spend more > time on it. But it hasn't been critical because the rest of the code > was so far away from finally landing anyway. Finally, and optionally, > comes the --remerge-diff and --remerge-diff-only options to log/show > (viewable at https://github.com/newren/git/tree/remerge-diff, but > these patches need to both be cleaned up and rebased on > ort-remainder). Maybe something like this with a bit of test prereq sprinkle on top? :) diff --git a/t/t6423-merge-rename-directories-TARGET-ort.sh b/t/t6423-merge-rename-directories-TARGET-ort.sh new file mode 120000 index 00000000000..6bf750c4036 --- /dev/null +++ b/t/t6423-merge-rename-directories-TARGET-ort.sh @@ -0,0 +1 @@ +t6423-merge-rename-directories.sh \ No newline at end of file diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh index 5d3b711fe68..1e571384223 100755 --- a/t/t6423-merge-rename-directories.sh +++ b/t/t6423-merge-rename-directories.sh @@ -3,2 +3,4 @@ test_description="recursive merge with directory renames" +# TARGET-ort: GIT_TEST_MERGE_ALGORITHM=ort && export GIT_TEST_MERGE_ALGORITHM=ort + # includes checking of many corner cases, with a similar methodology to: diff --git a/t/test-lib.sh b/t/test-lib.sh index d3f6af6a654..8d5da7e0ba9 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -247,2 +247,11 @@ TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}" TEST_NAME="$(basename "$0" .sh)" +if test -L "$0" +then + target=$(echo "$0" | grep -o "TARGET-[^.]*") + if test -n "$target" + then + to_eval=$(grep "^# $target: " "$0" | sed 's/.*://') + eval $to_eval + fi +fi TEST_NUMBER="${TEST_NAME%%-*}" That implementation's a bit of a hack, and requires SYMLINK (could be changed), but now I can: ./t6423-merge-rename-directories-TARGET-ort.sh ./t6423-merge-rename-directories.sh And run the whole thing with/without ort in one go. Then with a trivial symlink-in-a-loop $(git grep -l MERGE_ALGORITHM) loop on top I've got ort and non-ort merge tests all in one go on a WIP topic. >> And both test_expect_merge_algorithm and what seems to be a common >> pattern of e.g.: >> >> t6400-merge-df.sh: if test "$GIT_TEST_MERGE_ALGORITHM" = ort >> t6400-merge-df.sh- then >> t6400-merge-df.sh- test 0 -eq $(git ls-files -o | wc -l) >> t6400-merge-df.sh- else >> t6400-merge-df.sh- test 1 -eq $(git ls-files -o | wc -l) >> t6400-merge-df.sh- fi && >> >> Will not run tests on both backends, I was expecting to find something >> so we can the test N times for the backends, and declared if things were >> to be skipped on ort or whatever. > > Yeah, multiple ways of testing were discussed mid last year. There > were lots of tradeoffs. I think the thing that pushed in this > direction is that we're not just aiming to add another optional merge > backend, we're aiming to replace merge-recursive entirely. Since > merge tests appear all throughout the code base, many as rebase or > cherry-pick or revert or stash tests...or just as simple setup tests, > we want all of those tested with the new backend. Trying to duplicate > all those tests in any way other than just re-running the testsuite > with a different knob would require huge changes to hundreds > (thousands?) of testfiles and conflict with nearly every other topic. > So I made an environment variable that would choose which backend to > use, but with the downside of having to re-run the testsuite again. > >> I understand that this is still WIP code, but it would be nice to have >> it in a state where one can confidently touch merge-ort.c when changing >> some API or whatever and have it be tested by default. > > Thanks for proactively checking. To make it easier for you, I'll see > if I can submit a series later today that mostly completes the > merge-ort implementation; the t6423 testcases won't be fixed until > "Optimization batch 12" lands, and I might not be able to fix the > "TODO passed" submodule tests in this series, but the rest of the > stuff can be fixed with about 10-12 patches. I had been worried about > overloading the list with too many patches at once, but since it > sounds like you're willing to review these particular patches... :-) *nod* Also, I'll try to get to reviewing some of it, thanks for all your work. B.t.w., if the original E-Mail sounded like complaining that wasn't the intent. I just thought I'd shoot off a quick message about if I missed something about the in-flight status / testing of the ort work...
On Thu, Mar 4, 2021 at 1:29 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Thu, Mar 04 2021, Elijah Newren wrote: > > > Hi Ævar, > > > > On Thu, Mar 4, 2021 at 8:28 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > >> > >> On Fri, Jan 01 2021, Elijah Newren via GitGitGadget wrote: > >> > >> > + else { > >> > + /* must be the 100644/100755 case */ > >> > + assert(S_ISREG(a->mode)); > >> > + result->mode = a->mode; > >> > + clean = (b->mode == o->mode); > >> > + /* > >> > + * FIXME: If opt->priv->call_depth && !clean, then we really > >> > + * should not make result->mode match either a->mode or > >> > + * b->mode; that causes t6036 "check conflicting mode for > >> > + * regular file" to fail. It would be best to use some other > >> > + * mode, but we'll confuse all kinds of stuff if we use one > >> > + * where S_ISREG(result->mode) isn't true, and if we use > >> > + * something like 0100666, then tree-walk.c's calls to > >> > + * canon_mode() will just normalize that to 100644 for us and > >> > + * thus not solve anything. > >> > + * > >> > + * Figure out if there's some kind of way we can work around > >> > + * this... > >> > + */ > >> > >> So if tree-walk.c didn't call canon_mode() you would do: > >> > >> if (opt->priv->call_depth && !clean) > >> result->mode = 0100666; > >> else > >> result->mode = a->mode; > >> > >> I haven't looked at this bit closer, but that doesn't make the test > >> referenced here pass. > >> > >> I'm refactoring tree-walk.h to do that in a WIP series, and ran into > >> this. > > > > Interesting. Yeah, there might be more steps to make that particular > > test work, but I couldn't go any further due to canon_mode(). It's a > > testcase that has always failed under merge-recursive, and which I was > > resigned to always have fail under merge-ort too; I suspect it's > > enough of a corner case that no one but me ever really cared before. > > (And I didn't hit it in the wild or know anyone that did, I just > > learned of it by trying to clean up merge-recursive.) > > I'll send those patches out sooner than later, but as a quick question, > for merges / writing new files to the index/trees etc. we basically: > > 1. sanitize the mode with canon_mode() > 2. write it to a new object, either index or TREE object > > I've been trying to refactor things so those things have canon_mode() as > close to the time of writing as possible. > > Well, mostly to replace the whole S_*(mode) macros all over the place > with checks against "enum object_type", which is what most of it wants > anyway. > > Do you think the merge logic generally wants to operate on the "raw" > mode bits (including what may not even pass fsck checks), or the > sanitized canon_mode()? This one little special case is the only one when it'd care about the raw mode bits. I'm worried that making the code work on raw mode bits wouldn't be trivial. In general mode differences between different sides or the mode base is a conflict as well, so I'd need to add code around it's-not-really-a-conflict if canon_mode() on both modes map to the same value. > >> As an aside, how does one run the merge-ort tests in such a way as > >> they'll pass on master now? There's now a bunch of failures with > >> GIT_TEST_MERGE_ALGORITHM=ort, well, just for t*merge*.sh: > >> > >> t6409-merge-subtree.sh (Wstat: 256 Tests: 12 Failed: 1) > >> Failed test: 12 > >> Non-zero exit status: 1 > >> t6418-merge-text-auto.sh (Wstat: 256 Tests: 10 Failed: 3) > >> Failed tests: 4-5, 10 > >> Non-zero exit status: 1 > >> t6437-submodule-merge.sh (Wstat: 0 Tests: 18 Failed: 0) > >> TODO passed: 13, 17 > >> t6423-merge-rename-directories.sh (Wstat: 256 Tests: 68 Failed: 4) > >> Failed tests: 7, 53, 55, 59 > >> Non-zero exit status: 1 > > > > Right, I've been sending merge-ort upstream as fast as possible since > > last September or so, but there's only so much reviewer bandwidth so > > I've been forced to hold back on dozens of patches. > > > > Currently there are 8 test failures (all shown in your output here -- > > 1 in t6409, 3 in t6418, and 4 in t6423), and 12 TODO passed (only two > > of which you show here). I was forced to switch my ordering of > > sending patches upstream late last year due to an intern project that > > was planned to do significant work within diffcore-rename; I was > > worried about major conflicts, so I needed to get the diffcore-rename > > changes upstream earlier. That's still in-process. > > > > By the way, if you'd like to help accelerate the merge-ort work; it's > > almost entirely review bound. > > https://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/ > > still has no comments, then I have optimization series 10-14 to send > > (viewable up at > > https://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch), > > then I have other fixes -- mostly for the testsuite (viewable at > > https://github.com/newren/git/tree/ort-remainder), then I need to fix > > up the TODO passed submodule tests. Due to how the submodule testing > > framework functions, I can't just make a simple > > s/test_expect_failure/test_expect_success/ -- the tests are structured > > a bit funny and the tests are themselves buggy in some cases. I > > talked with jrnieder about it a little bit, just need to spend more > > time on it. But it hasn't been critical because the rest of the code > > was so far away from finally landing anyway. Finally, and optionally, > > comes the --remerge-diff and --remerge-diff-only options to log/show > > (viewable at https://github.com/newren/git/tree/remerge-diff, but > > these patches need to both be cleaned up and rebased on > > ort-remainder). > > Maybe something like this with a bit of test prereq sprinkle on top? :) > > diff --git a/t/t6423-merge-rename-directories-TARGET-ort.sh b/t/t6423-merge-rename-directories-TARGET-ort.sh > new file mode 120000 > index 00000000000..6bf750c4036 > --- /dev/null > +++ b/t/t6423-merge-rename-directories-TARGET-ort.sh > @@ -0,0 +1 @@ > +t6423-merge-rename-directories.sh > \ No newline at end of file > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh > index 5d3b711fe68..1e571384223 100755 > --- a/t/t6423-merge-rename-directories.sh > +++ b/t/t6423-merge-rename-directories.sh > @@ -3,2 +3,4 @@ > test_description="recursive merge with directory renames" > +# TARGET-ort: GIT_TEST_MERGE_ALGORITHM=ort && export GIT_TEST_MERGE_ALGORITHM=ort To test my understanding of your proposal, would you need to add this line to hundreds of files? > + > # includes checking of many corner cases, with a similar methodology to: > diff --git a/t/test-lib.sh b/t/test-lib.sh > index d3f6af6a654..8d5da7e0ba9 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -247,2 +247,11 @@ TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}" > TEST_NAME="$(basename "$0" .sh)" > +if test -L "$0" > +then > + target=$(echo "$0" | grep -o "TARGET-[^.]*") > + if test -n "$target" > + then > + to_eval=$(grep "^# $target: " "$0" | sed 's/.*://') > + eval $to_eval > + fi > +fi > TEST_NUMBER="${TEST_NAME%%-*}" > > That implementation's a bit of a hack, and requires SYMLINK (could be > changed), but now I can: > > ./t6423-merge-rename-directories-TARGET-ort.sh > ./t6423-merge-rename-directories.sh > > And run the whole thing with/without ort in one go. > > Then with a trivial symlink-in-a-loop $(git grep -l MERGE_ALGORITHM) > loop on top I've got ort and non-ort merge tests all in one go on a WIP > topic. Do you need a symlink for each file as well, thus hundreds of symlinks? Your idea is a quick way to get testing, that's much better than duplicating all the files. I'm still a bit worried that it'd encourage people to only add it to the "most important" or "most obvious" test files, which goes somewhat counter to testing merge-ort as a full replacement of merge-recursive. While developing merge-ort, I'd sometimes see failures outside t6*, even when everything under t6* passed. For example, t3[45]* and t76*. > >> And both test_expect_merge_algorithm and what seems to be a common > >> pattern of e.g.: > >> > >> t6400-merge-df.sh: if test "$GIT_TEST_MERGE_ALGORITHM" = ort > >> t6400-merge-df.sh- then > >> t6400-merge-df.sh- test 0 -eq $(git ls-files -o | wc -l) > >> t6400-merge-df.sh- else > >> t6400-merge-df.sh- test 1 -eq $(git ls-files -o | wc -l) > >> t6400-merge-df.sh- fi && > >> > >> Will not run tests on both backends, I was expecting to find something > >> so we can the test N times for the backends, and declared if things were > >> to be skipped on ort or whatever. > > > > Yeah, multiple ways of testing were discussed mid last year. There > > were lots of tradeoffs. I think the thing that pushed in this > > direction is that we're not just aiming to add another optional merge > > backend, we're aiming to replace merge-recursive entirely. Since > > merge tests appear all throughout the code base, many as rebase or > > cherry-pick or revert or stash tests...or just as simple setup tests, > > we want all of those tested with the new backend. Trying to duplicate > > all those tests in any way other than just re-running the testsuite > > with a different knob would require huge changes to hundreds > > (thousands?) of testfiles and conflict with nearly every other topic. > > So I made an environment variable that would choose which backend to > > use, but with the downside of having to re-run the testsuite again. > > > >> I understand that this is still WIP code, but it would be nice to have > >> it in a state where one can confidently touch merge-ort.c when changing > >> some API or whatever and have it be tested by default. > > > > Thanks for proactively checking. To make it easier for you, I'll see > > if I can submit a series later today that mostly completes the > > merge-ort implementation; the t6423 testcases won't be fixed until > > "Optimization batch 12" lands, and I might not be able to fix the > > "TODO passed" submodule tests in this series, but the rest of the > > stuff can be fixed with about 10-12 patches. I had been worried about > > overloading the list with too many patches at once, but since it > > sounds like you're willing to review these particular patches... :-) > > *nod* > > Also, I'll try to get to reviewing some of it, thanks for all your work. > > B.t.w., if the original E-Mail sounded like complaining that wasn't the > intent. I just thought I'd shoot off a quick message about if I missed > something about the in-flight status / testing of the ort work... Nope, didn't sound like complaining; thanks again for checking.
On Thu, Mar 04 2021, Elijah Newren wrote: > On Thu, Mar 4, 2021 at 1:29 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> >> >> On Thu, Mar 04 2021, Elijah Newren wrote: >> >> > Hi Ævar, >> > >> > On Thu, Mar 4, 2021 at 8:28 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> >> >> >> On Fri, Jan 01 2021, Elijah Newren via GitGitGadget wrote: >> >> >> >> > + else { >> >> > + /* must be the 100644/100755 case */ >> >> > + assert(S_ISREG(a->mode)); >> >> > + result->mode = a->mode; >> >> > + clean = (b->mode == o->mode); >> >> > + /* >> >> > + * FIXME: If opt->priv->call_depth && !clean, then we really >> >> > + * should not make result->mode match either a->mode or >> >> > + * b->mode; that causes t6036 "check conflicting mode for >> >> > + * regular file" to fail. It would be best to use some other >> >> > + * mode, but we'll confuse all kinds of stuff if we use one >> >> > + * where S_ISREG(result->mode) isn't true, and if we use >> >> > + * something like 0100666, then tree-walk.c's calls to >> >> > + * canon_mode() will just normalize that to 100644 for us and >> >> > + * thus not solve anything. >> >> > + * >> >> > + * Figure out if there's some kind of way we can work around >> >> > + * this... >> >> > + */ >> >> >> >> So if tree-walk.c didn't call canon_mode() you would do: >> >> >> >> if (opt->priv->call_depth && !clean) >> >> result->mode = 0100666; >> >> else >> >> result->mode = a->mode; >> >> >> >> I haven't looked at this bit closer, but that doesn't make the test >> >> referenced here pass. >> >> >> >> I'm refactoring tree-walk.h to do that in a WIP series, and ran into >> >> this. >> > >> > Interesting. Yeah, there might be more steps to make that particular >> > test work, but I couldn't go any further due to canon_mode(). It's a >> > testcase that has always failed under merge-recursive, and which I was >> > resigned to always have fail under merge-ort too; I suspect it's >> > enough of a corner case that no one but me ever really cared before. >> > (And I didn't hit it in the wild or know anyone that did, I just >> > learned of it by trying to clean up merge-recursive.) >> >> I'll send those patches out sooner than later, but as a quick question, >> for merges / writing new files to the index/trees etc. we basically: >> >> 1. sanitize the mode with canon_mode() >> 2. write it to a new object, either index or TREE object >> >> I've been trying to refactor things so those things have canon_mode() as >> close to the time of writing as possible. >> >> Well, mostly to replace the whole S_*(mode) macros all over the place >> with checks against "enum object_type", which is what most of it wants >> anyway. >> >> Do you think the merge logic generally wants to operate on the "raw" >> mode bits (including what may not even pass fsck checks), or the >> sanitized canon_mode()? > > This one little special case is the only one when it'd care about the > raw mode bits. I'm worried that making the code work on raw mode bits > wouldn't be trivial. In general mode differences between different > sides or the mode base is a conflict as well, so I'd need to add code > around it's-not-really-a-conflict if canon_mode() on both modes map to > the same value. It wasn't trivial, but let's see what you think of the end result, soon on-list. >> >> As an aside, how does one run the merge-ort tests in such a way as >> >> they'll pass on master now? There's now a bunch of failures with >> >> GIT_TEST_MERGE_ALGORITHM=ort, well, just for t*merge*.sh: >> >> >> >> t6409-merge-subtree.sh (Wstat: 256 Tests: 12 Failed: 1) >> >> Failed test: 12 >> >> Non-zero exit status: 1 >> >> t6418-merge-text-auto.sh (Wstat: 256 Tests: 10 Failed: 3) >> >> Failed tests: 4-5, 10 >> >> Non-zero exit status: 1 >> >> t6437-submodule-merge.sh (Wstat: 0 Tests: 18 Failed: 0) >> >> TODO passed: 13, 17 >> >> t6423-merge-rename-directories.sh (Wstat: 256 Tests: 68 Failed: 4) >> >> Failed tests: 7, 53, 55, 59 >> >> Non-zero exit status: 1 >> > >> > Right, I've been sending merge-ort upstream as fast as possible since >> > last September or so, but there's only so much reviewer bandwidth so >> > I've been forced to hold back on dozens of patches. >> > >> > Currently there are 8 test failures (all shown in your output here -- >> > 1 in t6409, 3 in t6418, and 4 in t6423), and 12 TODO passed (only two >> > of which you show here). I was forced to switch my ordering of >> > sending patches upstream late last year due to an intern project that >> > was planned to do significant work within diffcore-rename; I was >> > worried about major conflicts, so I needed to get the diffcore-rename >> > changes upstream earlier. That's still in-process. >> > >> > By the way, if you'd like to help accelerate the merge-ort work; it's >> > almost entirely review bound. >> > https://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/ >> > still has no comments, then I have optimization series 10-14 to send >> > (viewable up at >> > https://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch), >> > then I have other fixes -- mostly for the testsuite (viewable at >> > https://github.com/newren/git/tree/ort-remainder), then I need to fix >> > up the TODO passed submodule tests. Due to how the submodule testing >> > framework functions, I can't just make a simple >> > s/test_expect_failure/test_expect_success/ -- the tests are structured >> > a bit funny and the tests are themselves buggy in some cases. I >> > talked with jrnieder about it a little bit, just need to spend more >> > time on it. But it hasn't been critical because the rest of the code >> > was so far away from finally landing anyway. Finally, and optionally, >> > comes the --remerge-diff and --remerge-diff-only options to log/show >> > (viewable at https://github.com/newren/git/tree/remerge-diff, but >> > these patches need to both be cleaned up and rebased on >> > ort-remainder). >> >> Maybe something like this with a bit of test prereq sprinkle on top? :) >> >> diff --git a/t/t6423-merge-rename-directories-TARGET-ort.sh b/t/t6423-merge-rename-directories-TARGET-ort.sh >> new file mode 120000 >> index 00000000000..6bf750c4036 >> --- /dev/null >> +++ b/t/t6423-merge-rename-directories-TARGET-ort.sh >> @@ -0,0 +1 @@ >> +t6423-merge-rename-directories.sh >> \ No newline at end of file >> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh >> index 5d3b711fe68..1e571384223 100755 >> --- a/t/t6423-merge-rename-directories.sh >> +++ b/t/t6423-merge-rename-directories.sh >> @@ -3,2 +3,4 @@ >> test_description="recursive merge with directory renames" >> +# TARGET-ort: GIT_TEST_MERGE_ALGORITHM=ort && export GIT_TEST_MERGE_ALGORITHM=ort > > To test my understanding of your proposal, would you need to add this > line to hundreds of files? > >> + >> # includes checking of many corner cases, with a similar methodology to: >> diff --git a/t/test-lib.sh b/t/test-lib.sh >> index d3f6af6a654..8d5da7e0ba9 100644 >> --- a/t/test-lib.sh >> +++ b/t/test-lib.sh >> @@ -247,2 +247,11 @@ TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}" >> TEST_NAME="$(basename "$0" .sh)" >> +if test -L "$0" >> +then >> + target=$(echo "$0" | grep -o "TARGET-[^.]*") >> + if test -n "$target" >> + then >> + to_eval=$(grep "^# $target: " "$0" | sed 's/.*://') >> + eval $to_eval >> + fi >> +fi >> TEST_NUMBER="${TEST_NAME%%-*}" >> >> That implementation's a bit of a hack, and requires SYMLINK (could be >> changed), but now I can: >> >> ./t6423-merge-rename-directories-TARGET-ort.sh >> ./t6423-merge-rename-directories.sh >> >> And run the whole thing with/without ort in one go. >> >> Then with a trivial symlink-in-a-loop $(git grep -l MERGE_ALGORITHM) >> loop on top I've got ort and non-ort merge tests all in one go on a WIP >> topic. > > Do you need a symlink for each file as well, thus hundreds of symlinks? > > Your idea is a quick way to get testing, that's much better than > duplicating all the files. I'm still a bit worried that it'd > encourage people to only add it to the "most important" or "most > obvious" test files, which goes somewhat counter to testing merge-ort > as a full replacement of merge-recursive. While developing merge-ort, > I'd sometimes see failures outside t6*, even when everything under t6* > passed. For example, t3[45]* and t76*. Yes, this wouldn't make sense for merge-ort then. I was assuming that it was fairly isolated (at least mostly) to a few test files. That's mostly me not having read the ort traffic carefully, I'm embarrased to say that I managed to miss that it was a full "recursive" replacement, I thought it was (mostly) a new merge driver mode and we'd keep both. So nevermind :) I do think it's interesting to have something like this, but it's way out of scope for merge-ort work. E.g. we could start by making sure for all N tests in a file, we can run run each N times in a loop, i.e. individual --stress tests. That in itself would be a big undertaking, and would require e.g. having a "test_expect_success_setup" for tests that do one-off setup, which we'd skip. Then we could instrument the git_env_bool("GIT_TEST_*" with some replacement which logged if we ended up deciding something on whether that was true/false. And finally, log that with trace2, then for each test that encountered differing modes we'd run them for the N modes, or all combinations of modes (would quickly get expensive for things that touch a lot of things). Anyway, just take the above as rambling :) >> >> And both test_expect_merge_algorithm and what seems to be a common >> >> pattern of e.g.: >> >> >> >> t6400-merge-df.sh: if test "$GIT_TEST_MERGE_ALGORITHM" = ort >> >> t6400-merge-df.sh- then >> >> t6400-merge-df.sh- test 0 -eq $(git ls-files -o | wc -l) >> >> t6400-merge-df.sh- else >> >> t6400-merge-df.sh- test 1 -eq $(git ls-files -o | wc -l) >> >> t6400-merge-df.sh- fi && >> >> >> >> Will not run tests on both backends, I was expecting to find something >> >> so we can the test N times for the backends, and declared if things were >> >> to be skipped on ort or whatever. >> > >> > Yeah, multiple ways of testing were discussed mid last year. There >> > were lots of tradeoffs. I think the thing that pushed in this >> > direction is that we're not just aiming to add another optional merge >> > backend, we're aiming to replace merge-recursive entirely. Since >> > merge tests appear all throughout the code base, many as rebase or >> > cherry-pick or revert or stash tests...or just as simple setup tests, >> > we want all of those tested with the new backend. Trying to duplicate >> > all those tests in any way other than just re-running the testsuite >> > with a different knob would require huge changes to hundreds >> > (thousands?) of testfiles and conflict with nearly every other topic. >> > So I made an environment variable that would choose which backend to >> > use, but with the downside of having to re-run the testsuite again. >> > >> >> I understand that this is still WIP code, but it would be nice to have >> >> it in a state where one can confidently touch merge-ort.c when changing >> >> some API or whatever and have it be tested by default. >> > >> > Thanks for proactively checking. To make it easier for you, I'll see >> > if I can submit a series later today that mostly completes the >> > merge-ort implementation; the t6423 testcases won't be fixed until >> > "Optimization batch 12" lands, and I might not be able to fix the >> > "TODO passed" submodule tests in this series, but the rest of the >> > stuff can be fixed with about 10-12 patches. I had been worried about >> > overloading the list with too many patches at once, but since it >> > sounds like you're willing to review these particular patches... :-) >> >> *nod* >> >> Also, I'll try to get to reviewing some of it, thanks for all your work. >> >> B.t.w., if the original E-Mail sounded like complaining that wasn't the >> intent. I just thought I'd shoot off a quick message about if I missed >> something about the in-flight status / testing of the ort work... > > Nope, didn't sound like complaining; thanks again for checking. And thanks a lot for your good work on merge-ort & other things :)