* [PATCH 00/10] merge-ort: add more handling of basic conflict types
@ 2020-12-18 5:51 Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge Elijah Newren via GitGitGadget
` (11 more replies)
0 siblings, 12 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-12-18 5:51 UTC (permalink / raw)
To: git; +Cc: Elijah Newren
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge
2020-12-18 5:51 [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren via GitGitGadget
@ 2020-12-18 5:51 ` Elijah Newren via GitGitGadget
2020-12-30 14:06 ` Derrick Stolee
2020-12-18 5:51 ` [PATCH 02/10] merge-ort: handle directory/file conflicts that remain Elijah Newren via GitGitGadget
` (10 subsequent siblings)
11 siblings, 1 reply; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-12-18 5:51 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 02/10] merge-ort: handle directory/file conflicts that remain
2020-12-18 5:51 [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge Elijah Newren via GitGitGadget
@ 2020-12-18 5:51 ` Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 03/10] merge-ort: implement unique_path() helper Elijah Newren via GitGitGadget
` (9 subsequent siblings)
11 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-12-18 5:51 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 03/10] merge-ort: implement unique_path() helper
2020-12-18 5:51 [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 02/10] merge-ort: handle directory/file conflicts that remain Elijah Newren via GitGitGadget
@ 2020-12-18 5:51 ` Elijah Newren via GitGitGadget
2020-12-30 14:16 ` Derrick Stolee
2020-12-18 5:51 ` [PATCH 04/10] merge-ort: handle book-keeping around two- and three-way content merge Elijah Newren via GitGitGadget
` (8 subsequent siblings)
11 siblings, 1 reply; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-12-18 5:51 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 04/10] merge-ort: handle book-keeping around two- and three-way content merge
2020-12-18 5:51 [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren via GitGitGadget
` (2 preceding siblings ...)
2020-12-18 5:51 ` [PATCH 03/10] merge-ort: implement unique_path() helper Elijah Newren via GitGitGadget
@ 2020-12-18 5:51 ` Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 05/10] merge-ort: flesh out implementation of handle_content_merge() Elijah Newren via GitGitGadget
` (7 subsequent siblings)
11 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-12-18 5:51 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 05/10] merge-ort: flesh out implementation of handle_content_merge()
2020-12-18 5:51 [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren via GitGitGadget
` (3 preceding siblings ...)
2020-12-18 5:51 ` [PATCH 04/10] merge-ort: handle book-keeping around two- and three-way content merge Elijah Newren via GitGitGadget
@ 2020-12-18 5:51 ` Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 06/10] merge-ort: copy and adapt merge_3way() from merge-recursive.c Elijah Newren via GitGitGadget
` (6 subsequent siblings)
11 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-12-18 5:51 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 06/10] merge-ort: copy and adapt merge_3way() from merge-recursive.c
2020-12-18 5:51 [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren via GitGitGadget
` (4 preceding siblings ...)
2020-12-18 5:51 ` [PATCH 05/10] merge-ort: flesh out implementation of handle_content_merge() Elijah Newren via GitGitGadget
@ 2020-12-18 5:51 ` Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 07/10] merge-ort: copy and adapt merge_submodule() " Elijah Newren via GitGitGadget
` (5 subsequent siblings)
11 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-12-18 5:51 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 07/10] merge-ort: copy and adapt merge_submodule() from merge-recursive.c
2020-12-18 5:51 [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren via GitGitGadget
` (5 preceding siblings ...)
2020-12-18 5:51 ` [PATCH 06/10] merge-ort: copy and adapt merge_3way() from merge-recursive.c Elijah Newren via GitGitGadget
@ 2020-12-18 5:51 ` Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 08/10] merge-ort: implement format_commit() Elijah Newren via GitGitGadget
` (4 subsequent siblings)
11 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-12-18 5:51 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 08/10] merge-ort: implement format_commit()
2020-12-18 5:51 [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren via GitGitGadget
` (6 preceding siblings ...)
2020-12-18 5:51 ` [PATCH 07/10] merge-ort: copy and adapt merge_submodule() " Elijah Newren via GitGitGadget
@ 2020-12-18 5:51 ` Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 09/10] merge-ort: copy find_first_merges() implementation from merge-recursive.c Elijah Newren via GitGitGadget
` (3 subsequent siblings)
11 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-12-18 5:51 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 09/10] merge-ort: copy find_first_merges() implementation from merge-recursive.c
2020-12-18 5:51 [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren via GitGitGadget
` (7 preceding siblings ...)
2020-12-18 5:51 ` [PATCH 08/10] merge-ort: implement format_commit() Elijah Newren via GitGitGadget
@ 2020-12-18 5:51 ` Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 10/10] merge-ort: add handling for different types of files at same path Elijah Newren via GitGitGadget
` (2 subsequent siblings)
11 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-12-18 5:51 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 10/10] merge-ort: add handling for different types of files at same path
2020-12-18 5:51 [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren via GitGitGadget
` (8 preceding siblings ...)
2020-12-18 5:51 ` [PATCH 09/10] merge-ort: copy find_first_merges() implementation from merge-recursive.c Elijah Newren via GitGitGadget
@ 2020-12-18 5:51 ` Elijah Newren via GitGitGadget
2020-12-29 0:44 ` [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
11 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-12-18 5:51 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 00/10] merge-ort: add more handling of basic conflict types
2020-12-18 5:51 [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren via GitGitGadget
` (9 preceding siblings ...)
2020-12-18 5:51 ` [PATCH 10/10] merge-ort: add handling for different types of files at same path Elijah Newren via GitGitGadget
@ 2020-12-29 0:44 ` Elijah Newren
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
11 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren @ 2020-12-29 0:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge
2020-12-18 5:51 ` [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge Elijah Newren via GitGitGadget
@ 2020-12-30 14:06 ` Derrick Stolee
2020-12-30 15:13 ` Elijah Newren
0 siblings, 1 reply; 37+ messages in thread
From: Derrick Stolee @ 2020-12-30 14:06 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Elijah Newren
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 03/10] merge-ort: implement unique_path() helper
2020-12-18 5:51 ` [PATCH 03/10] merge-ort: implement unique_path() helper Elijah Newren via GitGitGadget
@ 2020-12-30 14:16 ` Derrick Stolee
2020-12-30 15:10 ` Elijah Newren
0 siblings, 1 reply; 37+ messages in thread
From: Derrick Stolee @ 2020-12-30 14:16 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Elijah Newren
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 03/10] merge-ort: implement unique_path() helper
2020-12-30 14:16 ` Derrick Stolee
@ 2020-12-30 15:10 ` Elijah Newren
2020-12-31 11:19 ` Derrick Stolee
0 siblings, 1 reply; 37+ messages in thread
From: Elijah Newren @ 2020-12-30 15:10 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Elijah Newren via GitGitGadget, Git Mailing List
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.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge
2020-12-30 14:06 ` Derrick Stolee
@ 2020-12-30 15:13 ` Elijah Newren
2020-12-31 11:17 ` Derrick Stolee
0 siblings, 1 reply; 37+ messages in thread
From: Elijah Newren @ 2020-12-30 15:13 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Elijah Newren via GitGitGadget, Git Mailing List
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.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge
2020-12-30 15:13 ` Elijah Newren
@ 2020-12-31 11:17 ` Derrick Stolee
2020-12-31 17:13 ` Elijah Newren
0 siblings, 1 reply; 37+ messages in thread
From: Derrick Stolee @ 2020-12-31 11:17 UTC (permalink / raw)
To: Elijah Newren; +Cc: Elijah Newren via GitGitGadget, Git Mailing List
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 03/10] merge-ort: implement unique_path() helper
2020-12-30 15:10 ` Elijah Newren
@ 2020-12-31 11:19 ` Derrick Stolee
0 siblings, 0 replies; 37+ messages in thread
From: Derrick Stolee @ 2020-12-31 11:19 UTC (permalink / raw)
To: Elijah Newren; +Cc: Elijah Newren via GitGitGadget, Git Mailing List
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge
2020-12-31 11:17 ` Derrick Stolee
@ 2020-12-31 17:13 ` Elijah Newren
0 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren @ 2020-12-31 17:13 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Elijah Newren via GitGitGadget, Git Mailing List
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.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 00/10] merge-ort: add more handling of basic conflict types
2020-12-18 5:51 [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren via GitGitGadget
` (10 preceding siblings ...)
2020-12-29 0:44 ` [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren
@ 2021-01-01 2:34 ` Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 01/10] merge-ort: handle D/F conflict where directory disappears due to merge Elijah Newren via GitGitGadget
` (10 more replies)
11 siblings, 11 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-01-01 2:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Derrick Stolee, Elijah Newren
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 01/10] merge-ort: handle D/F conflict where directory disappears due to merge
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
@ 2021-01-01 2:34 ` Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 02/10] merge-ort: handle directory/file conflicts that remain Elijah Newren via GitGitGadget
` (9 subsequent siblings)
10 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-01-01 2:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Derrick Stolee, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 02/10] merge-ort: handle directory/file conflicts that remain
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 01/10] merge-ort: handle D/F conflict where directory disappears due to merge Elijah Newren via GitGitGadget
@ 2021-01-01 2:34 ` Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 03/10] merge-ort: implement unique_path() helper Elijah Newren via GitGitGadget
` (8 subsequent siblings)
10 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-01-01 2:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Derrick Stolee, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 03/10] merge-ort: implement unique_path() helper
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 01/10] merge-ort: handle D/F conflict where directory disappears due to merge Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 02/10] merge-ort: handle directory/file conflicts that remain Elijah Newren via GitGitGadget
@ 2021-01-01 2:34 ` Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 04/10] merge-ort: handle book-keeping around two- and three-way content merge Elijah Newren via GitGitGadget
` (7 subsequent siblings)
10 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-01-01 2:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Derrick Stolee, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 04/10] merge-ort: handle book-keeping around two- and three-way content merge
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
` (2 preceding siblings ...)
2021-01-01 2:34 ` [PATCH v2 03/10] merge-ort: implement unique_path() helper Elijah Newren via GitGitGadget
@ 2021-01-01 2:34 ` Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 05/10] merge-ort: flesh out implementation of handle_content_merge() Elijah Newren via GitGitGadget
` (6 subsequent siblings)
10 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-01-01 2:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Derrick Stolee, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 05/10] merge-ort: flesh out implementation of handle_content_merge()
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
` (3 preceding siblings ...)
2021-01-01 2:34 ` [PATCH v2 04/10] merge-ort: handle book-keeping around two- and three-way content merge Elijah Newren via GitGitGadget
@ 2021-01-01 2:34 ` Elijah Newren via GitGitGadget
2021-03-04 16:28 ` A merge-ort TODO comment, and how to test merge-ort? Ævar Arnfjörð Bjarmason
2021-01-01 2:34 ` [PATCH v2 06/10] merge-ort: copy and adapt merge_3way() from merge-recursive.c Elijah Newren via GitGitGadget
` (5 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-01-01 2:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Derrick Stolee, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 06/10] merge-ort: copy and adapt merge_3way() from merge-recursive.c
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
` (4 preceding siblings ...)
2021-01-01 2:34 ` [PATCH v2 05/10] merge-ort: flesh out implementation of handle_content_merge() Elijah Newren via GitGitGadget
@ 2021-01-01 2:34 ` Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 07/10] merge-ort: copy and adapt merge_submodule() " Elijah Newren via GitGitGadget
` (4 subsequent siblings)
10 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-01-01 2:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Derrick Stolee, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 07/10] merge-ort: copy and adapt merge_submodule() from merge-recursive.c
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
` (5 preceding siblings ...)
2021-01-01 2:34 ` [PATCH v2 06/10] merge-ort: copy and adapt merge_3way() from merge-recursive.c Elijah Newren via GitGitGadget
@ 2021-01-01 2:34 ` Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 08/10] merge-ort: implement format_commit() Elijah Newren via GitGitGadget
` (3 subsequent siblings)
10 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-01-01 2:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Derrick Stolee, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 08/10] merge-ort: implement format_commit()
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
` (6 preceding siblings ...)
2021-01-01 2:34 ` [PATCH v2 07/10] merge-ort: copy and adapt merge_submodule() " Elijah Newren via GitGitGadget
@ 2021-01-01 2:34 ` Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 09/10] merge-ort: copy find_first_merges() implementation from merge-recursive.c Elijah Newren via GitGitGadget
` (2 subsequent siblings)
10 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-01-01 2:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Derrick Stolee, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 09/10] merge-ort: copy find_first_merges() implementation from merge-recursive.c
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
` (7 preceding siblings ...)
2021-01-01 2:34 ` [PATCH v2 08/10] merge-ort: implement format_commit() Elijah Newren via GitGitGadget
@ 2021-01-01 2:34 ` Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 10/10] merge-ort: add handling for different types of files at same path Elijah Newren via GitGitGadget
2021-01-05 14:23 ` [PATCH v2 00/10] merge-ort: add more handling of basic conflict types Derrick Stolee
10 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-01-01 2:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Derrick Stolee, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 10/10] merge-ort: add handling for different types of files at same path
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
` (8 preceding siblings ...)
2021-01-01 2:34 ` [PATCH v2 09/10] merge-ort: copy find_first_merges() implementation from merge-recursive.c Elijah Newren via GitGitGadget
@ 2021-01-01 2:34 ` Elijah Newren via GitGitGadget
2021-01-05 14:23 ` [PATCH v2 00/10] merge-ort: add more handling of basic conflict types Derrick Stolee
10 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-01-01 2:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Derrick Stolee, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH v2 00/10] merge-ort: add more handling of basic conflict types
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
` (9 preceding siblings ...)
2021-01-01 2:34 ` [PATCH v2 10/10] merge-ort: add handling for different types of files at same path Elijah Newren via GitGitGadget
@ 2021-01-05 14:23 ` Derrick Stolee
2021-01-06 19:20 ` Elijah Newren
10 siblings, 1 reply; 37+ messages in thread
From: Derrick Stolee @ 2021-01-05 14:23 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Elijah Newren
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 00/10] merge-ort: add more handling of basic conflict types
2021-01-05 14:23 ` [PATCH v2 00/10] merge-ort: add more handling of basic conflict types Derrick Stolee
@ 2021-01-06 19:20 ` Elijah Newren
0 siblings, 0 replies; 37+ messages in thread
From: Elijah Newren @ 2021-01-06 19:20 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Elijah Newren via GitGitGadget, Git Mailing List
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...
^ permalink raw reply [flat|nested] 37+ messages in thread
* A merge-ort TODO comment, and how to test merge-ort?
2021-01-01 2:34 ` [PATCH v2 05/10] merge-ort: flesh out implementation of handle_content_merge() Elijah Newren via GitGitGadget
@ 2021-03-04 16:28 ` Ævar Arnfjörð Bjarmason
2021-03-04 19:43 ` Elijah Newren
0 siblings, 1 reply; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-04 16:28 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Derrick Stolee, Elijah Newren
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...
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: A merge-ort TODO comment, and how to test merge-ort?
2021-03-04 16:28 ` A merge-ort TODO comment, and how to test merge-ort? Ævar Arnfjörð Bjarmason
@ 2021-03-04 19:43 ` Elijah Newren
2021-03-04 21:29 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 37+ messages in thread
From: Elijah Newren @ 2021-03-04 19:43 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Elijah Newren via GitGitGadget, Git Mailing List, Derrick Stolee,
Jonathan Nieder
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...
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: A merge-ort TODO comment, and how to test merge-ort?
2021-03-04 19:43 ` Elijah Newren
@ 2021-03-04 21:29 ` Ævar Arnfjörð Bjarmason
2021-03-04 22:45 ` Elijah Newren
0 siblings, 1 reply; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-04 21:29 UTC (permalink / raw)
To: Elijah Newren
Cc: Elijah Newren via GitGitGadget, Git Mailing List, Derrick Stolee,
Jonathan Nieder
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...
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: A merge-ort TODO comment, and how to test merge-ort?
2021-03-04 21:29 ` Ævar Arnfjörð Bjarmason
@ 2021-03-04 22:45 ` Elijah Newren
2021-03-08 14:49 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 37+ messages in thread
From: Elijah Newren @ 2021-03-04 22:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Elijah Newren via GitGitGadget, Git Mailing List, Derrick Stolee,
Jonathan Nieder
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.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: A merge-ort TODO comment, and how to test merge-ort?
2021-03-04 22:45 ` Elijah Newren
@ 2021-03-08 14:49 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-08 14:49 UTC (permalink / raw)
To: Elijah Newren
Cc: Elijah Newren via GitGitGadget, Git Mailing List, Derrick Stolee,
Jonathan Nieder
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 :)
^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2021-03-08 14:50 UTC | newest]
Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-18 5:51 [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 01/10] merge-ort: handle D/F conflict where directory disappears due to merge Elijah Newren via GitGitGadget
2020-12-30 14:06 ` Derrick Stolee
2020-12-30 15:13 ` Elijah Newren
2020-12-31 11:17 ` Derrick Stolee
2020-12-31 17:13 ` Elijah Newren
2020-12-18 5:51 ` [PATCH 02/10] merge-ort: handle directory/file conflicts that remain Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 03/10] merge-ort: implement unique_path() helper Elijah Newren via GitGitGadget
2020-12-30 14:16 ` Derrick Stolee
2020-12-30 15:10 ` Elijah Newren
2020-12-31 11:19 ` Derrick Stolee
2020-12-18 5:51 ` [PATCH 04/10] merge-ort: handle book-keeping around two- and three-way content merge Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 05/10] merge-ort: flesh out implementation of handle_content_merge() Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 06/10] merge-ort: copy and adapt merge_3way() from merge-recursive.c Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 07/10] merge-ort: copy and adapt merge_submodule() " Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 08/10] merge-ort: implement format_commit() Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 09/10] merge-ort: copy find_first_merges() implementation from merge-recursive.c Elijah Newren via GitGitGadget
2020-12-18 5:51 ` [PATCH 10/10] merge-ort: add handling for different types of files at same path Elijah Newren via GitGitGadget
2020-12-29 0:44 ` [PATCH 00/10] merge-ort: add more handling of basic conflict types Elijah Newren
2021-01-01 2:34 ` [PATCH v2 " Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 01/10] merge-ort: handle D/F conflict where directory disappears due to merge Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 02/10] merge-ort: handle directory/file conflicts that remain Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 03/10] merge-ort: implement unique_path() helper Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 04/10] merge-ort: handle book-keeping around two- and three-way content merge Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 05/10] merge-ort: flesh out implementation of handle_content_merge() Elijah Newren via GitGitGadget
2021-03-04 16:28 ` A merge-ort TODO comment, and how to test merge-ort? Ævar Arnfjörð Bjarmason
2021-03-04 19:43 ` Elijah Newren
2021-03-04 21:29 ` Ævar Arnfjörð Bjarmason
2021-03-04 22:45 ` Elijah Newren
2021-03-08 14:49 ` Ævar Arnfjörð Bjarmason
2021-01-01 2:34 ` [PATCH v2 06/10] merge-ort: copy and adapt merge_3way() from merge-recursive.c Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 07/10] merge-ort: copy and adapt merge_submodule() " Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 08/10] merge-ort: implement format_commit() Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 09/10] merge-ort: copy find_first_merges() implementation from merge-recursive.c Elijah Newren via GitGitGadget
2021-01-01 2:34 ` [PATCH v2 10/10] merge-ort: add handling for different types of files at same path Elijah Newren via GitGitGadget
2021-01-05 14:23 ` [PATCH v2 00/10] merge-ort: add more handling of basic conflict types Derrick Stolee
2021-01-06 19:20 ` Elijah Newren
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).