On Thu, Sep 02, 2021 at 09:11:29PM +0200, Stefan Sperling wrote:
> On Thu, Sep 02, 2021 at 07:16:43PM +0200, Christian Weisgerber wrote:
> > It's still fairly slow, though.
> 
> Needs profiling to dig deeper.

I found a way to speed it up a bit further.

The current code checks merge pre-conditions for all files in the work tree.
We can instead only check pre-conditions on files which will be affected
by the merge. This can be done by running the tree diff twice, once for
checking pre-conditions, and then again for merging.
In general, this will be faster than iterating over all files in the work
tree and then diffing trees to do the merge. We use less stat(2) calls.
And thanks to the object cache the second tree diff run will have less
overhead than the first run.

As a side-effect this relaxes pre-condition checks a bit:
Conflicts on files which will not be affected by 'got cherrypick' will
now be ignored. I guess some would say that this is an improvement over
the current behaviour where cherrypick is disallowed if _any_ file in
the work tree contains conflicts. I would expect lingering merge conflicts
to be common in large disparate trees such as a ports tree checkout.

This patch doesn't touch the test suite, and our tests are still passing.
Ideally, when relaxing pre-conditions we should try to expand test coverage
to cover cases which will no longer be caught upfront and might result in
unexpected behaviour. Any ideas what we could test for? I have some ideas
but would like to collect other people's ideas, too.

diff 5546d4669ccce281a6f6f89ccf204e671ed0d3a9 /home/stsp/src/got
blob - 15441d50109add43dd64b4368c57680ec2884d50
file + got/got.1
--- got/got.1
+++ got/got.1
@@ -1615,7 +1615,7 @@ will refuse to run if certain preconditions are not me
 If the work tree contains multiple base commits it must first be updated
 to a single base commit with
 .Cm got update .
-If the work tree already contains files with merge conflicts, these
+If any relevant files already contain merge conflicts, these
 conflicts must be resolved first.
 .It Cm cy
 Short alias for
@@ -1659,7 +1659,7 @@ will refuse to run if certain preconditions are not me
 If the work tree contains multiple base commits it must first be updated
 to a single base commit with
 .Cm got update .
-If the work tree already contains files with merge conflicts, these
+If any relevant files already contain merge conflicts, these
 conflicts must be resolved first.
 .It Cm bo
 Short alias for
blob - 821074bee10bd2aec44fc5e2732eb33c5b157f6f
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -3120,22 +3120,35 @@ done:
 
 struct check_merge_ok_arg {
 	struct got_worktree *worktree;
+	struct got_fileindex *fileindex;
 	struct got_repository *repo;
 };
 
 static const struct got_error *
-check_merge_ok(void *arg, struct got_fileindex_entry *ie)
+check_merge_ok(void *arg, struct got_blob_object *blob1,
+    struct got_blob_object *blob2, struct got_object_id *id1,
+    struct got_object_id *id2, const char *path1, const char *path2,
+    mode_t mode1, mode_t mode2, struct got_repository *repo)
 {
 	const struct got_error *err = NULL;
 	struct check_merge_ok_arg *a = arg;
 	unsigned char status;
 	struct stat sb;
+	struct got_fileindex_entry *ie;
+	const char *path = path2 ? path2 : path1;
+	struct got_object_id *id = id2 ? id2 : id1;
 	char *ondisk_path;
 
+	if (id == NULL)
+		return NULL;
+
+	ie = got_fileindex_entry_get(a->fileindex, path, strlen(path));
+	if (ie == NULL)
+		return NULL;
+
 	/* Reject merges into a work tree with mixed base commits. */
 	if (got_fileindex_entry_has_commit(ie) &&
-	    memcmp(ie->commit_sha1, a->worktree->base_commit_id->sha1,
-	    SHA1_DIGEST_LENGTH))
+	    got_object_id_cmp(id, a->worktree->base_commit_id) == 0)
 		return got_error(GOT_ERR_MIXED_COMMITS);
 
 	if (asprintf(&ondisk_path, "%s/%s", a->worktree->root_path, ie->path)
@@ -3163,6 +3176,7 @@ merge_files(struct got_worktree *worktree, struct got_
 	const struct got_error *err = NULL, *sync_err;
 	struct got_object_id *tree_id1 = NULL, *tree_id2 = NULL;
 	struct got_tree_object *tree1 = NULL, *tree2 = NULL;
+	struct check_merge_ok_arg mok_arg;
 	struct merge_file_cb_arg arg;
 	char *label_orig = NULL;
 
@@ -3201,6 +3215,14 @@ merge_files(struct got_worktree *worktree, struct got_
 	if (err)
 		goto done;
 
+	mok_arg.worktree = worktree;
+	mok_arg.fileindex = fileindex;
+	mok_arg.repo = repo;
+	err = got_diff_tree(tree1, tree2, "", "", repo,
+	    check_merge_ok, &mok_arg, 0);
+	if (err)
+		goto done;
+
 	arg.worktree = worktree;
 	arg.fileindex = fileindex;
 	arg.progress_cb = progress_cb;
@@ -3231,7 +3253,6 @@ got_worktree_merge_files(struct got_worktree *worktree
 	const struct got_error *err, *unlockerr;
 	char *fileindex_path = NULL;
 	struct got_fileindex *fileindex = NULL;
-	struct check_merge_ok_arg mok_arg;
 
 	err = lock_worktree(worktree, LOCK_EX);
 	if (err)
@@ -3241,13 +3262,6 @@ got_worktree_merge_files(struct got_worktree *worktree
 	if (err)
 		goto done;
 
-	mok_arg.worktree = worktree;
-	mok_arg.repo = repo;
-	err = got_fileindex_for_each_entry_safe(fileindex, check_merge_ok,
-	    &mok_arg);
-	if (err)
-		goto done;
-
 	err = merge_files(worktree, fileindex, fileindex_path, commit_id1,
 	    commit_id2, repo, progress_cb, progress_arg, cancel_cb, cancel_arg);
 done: