From: Stefan Sperling <stsp@stsp.name>
Subject: Re: rebase suffers '?' conflicts
To: Christian Weisgerber <naddy@mips.inka.de>
Cc: gameoftrees@openbsd.org
Date: Thu, 13 Apr 2023 12:50:18 +0200

On Thu, Apr 13, 2023 at 12:13:45AM +0200, Christian Weisgerber wrote:
> Stefan Sperling:
> 
> > Can you try this? If this works then I'll try to add regress coverage
> > as well before committing it.
> 
> Yes!!  With that my reproducer script completes successfully.

Here is a more complete diff which implements the fix for rebase,
histedit, and merge. With an added safeguard for files which appear
in added status during the revert step but were not merged into the
work tree as part of the most recently rebased/edited/merged commit.
Deleting such files could result in data loss which is a big no-no
for a version control system.
It could be argued that such files should left in added status
rather than unversioned. But it seems cleaner to me to leave the
work tree behind without anything scheduled for commit.

Also adds test coverage.

OK?

-----------------------------------------------
 when aborting rebase/histedit/merge, unlink files added by merged changes
 
 Otherwise we leave unversioned files behind in the work tree which may
 interfere with new attempts to rebase or merge the changes again.
 
 Problem found by naddy@
 
diff 8cdd231889a848b735f84ed6772eab46c2512db9 a3a41f3a8c0cf88fc117c5edae388d564084f0cd
commit - 8cdd231889a848b735f84ed6772eab46c2512db9
commit + a3a41f3a8c0cf88fc117c5edae388d564084f0cd
blob - b08ea7c98c8952d224400390c47972f2e057ee4e
blob + cad03d0a864965a0acebeb1425c9c753c3d3449e
--- lib/worktree.c
+++ lib/worktree.c
@@ -4700,6 +4700,7 @@ struct revert_file_args {
 	void *patch_arg;
 	struct got_repository *repo;
 	int unlink_added_files;
+	struct got_pathlist_head *added_files_to_unlink;
 };
 
 static const struct got_error *
@@ -5002,16 +5003,33 @@ revert_file(void *arg, unsigned char status, unsigned 
 			goto done;
 		got_fileindex_entry_remove(a->fileindex, ie);
 		if (a->unlink_added_files) {
-			if (asprintf(&ondisk_path, "%s/%s",
-			    got_worktree_get_root_path(a->worktree),
-			    relpath) == -1) {
-				err = got_error_from_errno("asprintf");
-				goto done;
+			int do_unlink = a->added_files_to_unlink ? 0 : 1;
+
+			if (a->added_files_to_unlink) {
+				struct got_pathlist_entry *pe;
+
+				TAILQ_FOREACH(pe, a->added_files_to_unlink,
+				    entry) {
+					if (got_path_cmp(pe->path, relpath,
+					    pe->path_len, strlen(relpath)))
+						continue;
+					do_unlink = 1;
+					break;
+				}
 			}
-			if (unlink(ondisk_path) == -1) {
-				err = got_error_from_errno2("unlink",
-				    ondisk_path);
-				break;
+
+			if (do_unlink) {
+				if (asprintf(&ondisk_path, "%s/%s",
+				    got_worktree_get_root_path(a->worktree),
+				    relpath) == -1) {
+					err = got_error_from_errno("asprintf");
+					goto done;
+				}
+				if (unlink(ondisk_path) == -1) {
+					err = got_error_from_errno2("unlink",
+					    ondisk_path);
+					break;
+				}
 			}
 		}
 		break;
@@ -7328,6 +7346,131 @@ const struct got_error *
 	return err;
 }
 
+static const struct got_error *
+get_paths_changed_between_commits(struct got_pathlist_head *paths,
+    struct got_object_id *id1, struct got_object_id *id2,
+    struct got_repository *repo)
+{
+	const struct got_error		*err;
+	struct got_commit_object	*commit1 = NULL, *commit2 = NULL;
+	struct got_tree_object		*tree1 = NULL, *tree2 = NULL;
+
+	if (id1) {
+		err = got_object_open_as_commit(&commit1, repo, id1);
+		if (err)
+			goto done;
+
+		err = got_object_open_as_tree(&tree1, repo,
+		    got_object_commit_get_tree_id(commit1));
+		if (err)
+			goto done;
+	}
+
+	if (id2) {
+		err = got_object_open_as_commit(&commit2, repo, id2);
+		if (err)
+			goto done;
+
+		err = got_object_open_as_tree(&tree2, repo,
+		    got_object_commit_get_tree_id(commit2));
+		if (err)
+			goto done;
+	}
+
+	err = got_diff_tree(tree1, tree2, NULL, NULL, -1, -1, "", "", repo,
+	    got_diff_tree_collect_changed_paths, paths, 0);
+	if (err)
+		goto done;
+done:
+	if (commit1)
+		got_object_commit_close(commit1);
+	if (commit2)
+		got_object_commit_close(commit2);
+	if (tree1)
+		got_object_tree_close(tree1);
+	if (tree2)
+		got_object_tree_close(tree2);
+	return err;
+}
+
+static const struct got_error *
+get_paths_added_between_commits(struct got_pathlist_head *added_paths,
+    struct got_object_id *id1, struct got_object_id *id2,
+    const char *path_prefix, struct got_repository *repo)
+{
+	const struct got_error *err;
+	struct got_pathlist_head merged_paths;
+	struct got_pathlist_entry *pe;
+	char *abspath = NULL, *wt_path = NULL;
+
+	TAILQ_INIT(&merged_paths);
+
+	err = get_paths_changed_between_commits(&merged_paths, id1, id2, repo);
+	if (err)
+		goto done;
+
+	TAILQ_FOREACH(pe, &merged_paths, entry) {
+		struct got_diff_changed_path *change = pe->data;
+
+		if (change->status != GOT_STATUS_ADD)
+			continue;
+
+		if (got_path_is_root_dir(path_prefix)) {
+			wt_path = strdup(pe->path);
+			if (wt_path == NULL) {
+				err = got_error_from_errno("strdup");
+				goto done;
+			}
+		} else {
+			if (asprintf(&abspath, "/%s", pe->path) == -1) {
+				err = got_error_from_errno("strdup");
+				goto done;
+			}
+
+			err = got_path_skip_common_ancestor(&wt_path,
+			    path_prefix, abspath);
+			if (err)
+				goto done;
+		}
+
+		err = got_pathlist_append(added_paths, wt_path, NULL);
+		if (err)
+			goto done;
+		wt_path = NULL;
+	}
+
+done:
+	got_pathlist_free(&merged_paths, GOT_PATHLIST_FREE_ALL);
+	free(abspath);
+	free(wt_path);
+	return err;
+}
+
+static const struct got_error *
+get_paths_added_in_commit(struct got_pathlist_head *added_paths,
+    struct got_object_id *id, const char *path_prefix,
+    struct got_repository *repo)
+{
+	const struct got_error		*err;
+	struct got_commit_object	*commit = NULL;
+	struct got_object_qid		*pid;
+
+	err = got_object_open_as_commit(&commit, repo, id);
+	if (err)
+		goto done;
+
+	pid = STAILQ_FIRST(got_object_commit_get_parent_ids(commit));
+
+	err = get_paths_added_between_commits(added_paths,
+	    pid ? &pid->id : NULL, id, path_prefix, repo);
+	if (err)
+		goto done;
+done:
+	if (commit)
+		got_object_commit_close(commit);
+	return err;
+}
+
 const struct got_error *
 got_worktree_rebase_abort(struct got_worktree *worktree,
     struct got_fileindex *fileindex, struct got_repository *repo,
@@ -7337,15 +7480,44 @@ got_worktree_rebase_abort(struct got_worktree *worktre
 	const struct got_error *err, *unlockerr, *sync_err;
 	struct got_reference *resolved = NULL;
 	struct got_object_id *commit_id = NULL;
+	struct got_object_id *merged_commit_id = NULL;
 	struct got_commit_object *commit = NULL;
 	char *fileindex_path = NULL;
+	char *commit_ref_name = NULL;
+	struct got_reference *commit_ref = NULL;
 	struct revert_file_args rfa;
 	struct got_object_id *tree_id = NULL;
+	struct got_pathlist_head added_paths;
 
+	TAILQ_INIT(&added_paths);
+
 	err = lock_worktree(worktree, LOCK_EX);
 	if (err)
 		return err;
 
+	err = get_rebase_commit_ref_name(&commit_ref_name, worktree);
+	if (err)
+		goto done;
+
+	err = got_ref_open(&commit_ref, repo, commit_ref_name, 0);
+	if (err)
+		goto done;
+
+	err = got_ref_resolve(&merged_commit_id, repo, commit_ref);
+	if (err)
+		goto done;
+
+	/*
+	 * Determine which files in added status can be safely removed
+	 * from disk while reverting changes in the work tree.
+	 * We want to avoid deleting unrelated files which were added by
+	 * the user for conflict resolution purposes.
+	 */
+	err = get_paths_added_in_commit(&added_paths, merged_commit_id,
+	    got_worktree_get_path_prefix(worktree), repo);
+	if (err)
+		goto done;
+
 	err = got_ref_open(&resolved, repo,
 	    got_ref_get_symref_target(new_base_branch), 0);
 	if (err)
@@ -7393,7 +7565,8 @@ got_worktree_rebase_abort(struct got_worktree *worktre
 	rfa.patch_cb = NULL;
 	rfa.patch_arg = NULL;
 	rfa.repo = repo;
-	rfa.unlink_added_files = 0;
+	rfa.unlink_added_files = 1;
+	rfa.added_files_to_unlink = &added_paths;
 	err = worktree_status(worktree, "", fileindex, repo,
 	    revert_file, &rfa, NULL, NULL, 1, 0);
 	if (err)
@@ -7406,14 +7579,19 @@ done:
 	if (sync_err && err == NULL)
 		err = sync_err;
 done:
+	got_pathlist_free(&added_paths, GOT_PATHLIST_FREE_PATH);
 	got_ref_close(resolved);
 	free(tree_id);
 	free(commit_id);
+	free(merged_commit_id);
 	if (commit)
 		got_object_commit_close(commit);
 	if (fileindex)
 		got_fileindex_free(fileindex);
 	free(fileindex_path);
+	free(commit_ref_name);
+	if (commit_ref)
+		got_ref_close(commit_ref);
 
 	unlockerr = lock_worktree(worktree, LOCK_SH);
 	if (unlockerr && err == NULL)
@@ -7709,14 +7887,49 @@ got_worktree_histedit_abort(struct got_worktree *workt
 	const struct got_error *err, *unlockerr, *sync_err;
 	struct got_reference *resolved = NULL;
 	char *fileindex_path = NULL;
+	struct got_object_id *merged_commit_id = NULL;
 	struct got_commit_object *commit = NULL;
+	char *commit_ref_name = NULL;
+	struct got_reference *commit_ref = NULL;
 	struct got_object_id *tree_id = NULL;
 	struct revert_file_args rfa;
+	struct got_pathlist_head added_paths;
 
+	TAILQ_INIT(&added_paths);
+
 	err = lock_worktree(worktree, LOCK_EX);
 	if (err)
 		return err;
 
+	err = get_histedit_commit_ref_name(&commit_ref_name, worktree);
+	if (err)
+		goto done;
+
+	err = got_ref_open(&commit_ref, repo, commit_ref_name, 0);
+	if (err) {
+		if (err->code != GOT_ERR_NOT_REF)
+			goto done;
+		/* Can happen on early abort due to invalid histedit script. */
+		commit_ref = NULL;
+	}
+
+	if (commit_ref) {
+		err = got_ref_resolve(&merged_commit_id, repo, commit_ref);
+		if (err)
+			goto done;
+
+		/*
+		 * Determine which files in added status can be safely removed
+		 * from disk while reverting changes in the work tree.
+		 * We want to avoid deleting unrelated files added by the
+		 * user during conflict resolution or during histedit -e.
+		 */
+		err = get_paths_added_in_commit(&added_paths, merged_commit_id,
+		    got_worktree_get_path_prefix(worktree), repo);
+		if (err)
+			goto done;
+	}
+
 	err = got_ref_open(&resolved, repo,
 	    got_ref_get_symref_target(branch), 0);
 	if (err)
@@ -7755,7 +7968,8 @@ got_worktree_histedit_abort(struct got_worktree *workt
 	rfa.patch_cb = NULL;
 	rfa.patch_arg = NULL;
 	rfa.repo = repo;
-	rfa.unlink_added_files = 0;
+	rfa.unlink_added_files = 1;
+	rfa.added_files_to_unlink = &added_paths;
 	err = worktree_status(worktree, "", fileindex, repo,
 	    revert_file, &rfa, NULL, NULL, 1, 0);
 	if (err)
@@ -7768,9 +7982,14 @@ done:
 	if (sync_err && err == NULL)
 		err = sync_err;
 done:
-	got_ref_close(resolved);
+	if (resolved)
+		got_ref_close(resolved);
+	if (commit_ref)
+		got_ref_close(commit_ref);
+	free(merged_commit_id);
 	free(tree_id);
 	free(fileindex_path);
+	free(commit_ref_name);
 
 	unlockerr = lock_worktree(worktree, LOCK_SH);
 	if (unlockerr && err == NULL)
@@ -8448,12 +8667,42 @@ got_worktree_merge_abort(struct got_worktree *worktree
     got_worktree_checkout_cb progress_cb, void *progress_arg)
 {
 	const struct got_error *err, *unlockerr, *sync_err;
-	struct got_object_id *commit_id = NULL;
 	struct got_commit_object *commit = NULL;
 	char *fileindex_path = NULL;
 	struct revert_file_args rfa;
+	char *commit_ref_name = NULL;
+	struct got_reference *commit_ref = NULL;
+	struct got_object_id *merged_commit_id = NULL;
 	struct got_object_id *tree_id = NULL;
+	struct got_pathlist_head added_paths;
 
+	TAILQ_INIT(&added_paths);
+
+	err = get_merge_commit_ref_name(&commit_ref_name, worktree);
+	if (err)
+		goto done;
+
+	err = got_ref_open(&commit_ref, repo, commit_ref_name, 0);
+	if (err)
+		goto done;
+
+	err = got_ref_resolve(&merged_commit_id, repo, commit_ref);
+	if (err)
+		goto done;
+
+	/*
+	 * Determine which files in added status can be safely removed
+	 * from disk while reverting changes in the work tree.
+	 * We want to avoid deleting unrelated files which were added by
+	 * the user for conflict resolution purposes.
+	 */
+	err = get_paths_added_between_commits(&added_paths,
+	    got_worktree_get_base_commit_id(worktree), merged_commit_id,
+	    got_worktree_get_path_prefix(worktree), repo);
+	if (err)
+		goto done;
+
+
 	err = got_object_open_as_commit(&commit, repo,
 	    worktree->base_commit_id);
 	if (err)
@@ -8480,6 +8729,7 @@ got_worktree_merge_abort(struct got_worktree *worktree
 	rfa.patch_arg = NULL;
 	rfa.repo = repo;
 	rfa.unlink_added_files = 1;
+	rfa.added_files_to_unlink = &added_paths;
 	err = worktree_status(worktree, "", fileindex, repo,
 	    revert_file, &rfa, NULL, NULL, 1, 0);
 	if (err)
@@ -8493,12 +8743,15 @@ done:
 		err = sync_err;
 done:
 	free(tree_id);
-	free(commit_id);
+	free(merged_commit_id);
 	if (commit)
 		got_object_commit_close(commit);
 	if (fileindex)
 		got_fileindex_free(fileindex);
 	free(fileindex_path);
+	if (commit_ref)
+		got_ref_close(commit_ref);
+	free(commit_ref_name);
 
 	unlockerr = lock_worktree(worktree, LOCK_SH);
 	if (unlockerr && err == NULL)
blob - efdeafbe917d9f2c945e958629fc06b7bd9af2ba
blob + 6080151a8a3e5c278b52bd1288edfdf4aa4383fb
--- regress/cmdline/histedit.sh
+++ regress/cmdline/histedit.sh
@@ -902,20 +902,15 @@ test_histedit_abort() {
 		fi
 	done
 
-	echo "new file on master" > $testroot/content.expected
-	cat $testroot/wt/epsilon/new > $testroot/content
-	cmp -s $testroot/content.expected $testroot/content
-	ret=$?
-	if [ $ret -ne 0 ]; then
-		diff -u $testroot/content.expected $testroot/content
-		test_done "$testroot" "$ret"
+	if [ -e $testroot/wt/epsilon/new ]; then
+		echo "removed file new still exists on disk" >&2
+		test_done "$testroot" "1"
 		return 1
 	fi
 
 	(cd $testroot/wt && got status > $testroot/stdout)
 
-	echo "?  epsilon/new" > $testroot/stdout.expected
-	echo "?  unversioned-file" >> $testroot/stdout.expected
+	echo "?  unversioned-file" > $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret=$?
 	if [ $ret -ne 0 ]; then
blob - 876eced1d6b84caf0d92277204c67399f2876875
blob + 9093dd5b9fb524b14d2d17d4fe754da0ec8ef9f4
--- regress/cmdline/merge.sh
+++ regress/cmdline/merge.sh
@@ -673,9 +673,14 @@ test_merge_abort() {
 		return 1
 	fi
 
+	# unrelated added file added during conflict resolution
+	touch $testroot/wt/added-file
+	(cd $testroot/wt && got add added-file > /dev/null)
+
 	(cd $testroot/wt && got status > $testroot/stdout)
 
-	echo "C  alpha" > $testroot/stdout.expected
+	echo "A  added-file" > $testroot/stdout.expected
+	echo "C  alpha" >> $testroot/stdout.expected
 	echo "D  beta" >> $testroot/stdout.expected
 	echo "A  epsilon/new" >> $testroot/stdout.expected
 	echo "M  gamma/delta" >> $testroot/stdout.expected
@@ -697,11 +702,13 @@ test_merge_abort() {
 		return 1
 	fi
 
-	echo "R  alpha" > $testroot/stdout.expected
+	echo "R  added-file" > $testroot/stdout.expected
+	echo "R  alpha" >> $testroot/stdout.expected
 	echo "R  beta" >> $testroot/stdout.expected
 	echo "R  epsilon/new" >> $testroot/stdout.expected
 	echo "R  gamma/delta" >> $testroot/stdout.expected
 	echo "R  symlink" >> $testroot/stdout.expected
+	echo "G  added-file" >> $testroot/stdout.expected
 	echo "Merge of refs/heads/newbranch aborted" \
 		>> $testroot/stdout.expected
 
@@ -757,7 +764,8 @@ test_merge_abort() {
 
 	(cd $testroot/wt && got status > $testroot/stdout)
 
-	echo "?  unversioned-file" > $testroot/stdout.expected
+	echo "?  added-file" > $testroot/stdout.expected
+	echo "?  unversioned-file" >> $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret=$?
 	if [ $ret -ne 0 ]; then
blob - bb762cd05b066923e9538c4bdf31982707dba134
blob + 1d339f654c90ee75c99eac9cc5a7f39e8b6275ce
--- regress/cmdline/rebase.sh
+++ regress/cmdline/rebase.sh
@@ -419,7 +419,10 @@ test_rebase_abort() {
 	local short_orig_commit1=`trim_obj_id 28 $orig_commit1`
 
 	echo "modified alpha on branch" > $testroot/repo/alpha
-	git_commit $testroot/repo -m "committing to alpha on newbranch"
+	echo "new file on branch" > $testroot/repo/epsilon/new
+	(cd $testroot/repo && git add epsilon/new)
+	git_commit $testroot/repo \
+		-m "changing alpha and adding new on newbranch"
 	local orig_commit2=`git_show_head $testroot/repo`
 	local short_orig_commit2=`trim_obj_id 28 $orig_commit2`
 
@@ -450,10 +453,12 @@ test_rebase_abort() {
 		>> $testroot/stdout.expected
 	echo ": committing to beta on newbranch" >> $testroot/stdout.expected
 	echo "C  alpha" >> $testroot/stdout.expected
+	echo "A  epsilon/new" >> $testroot/stdout.expected
 	echo "Files with new merge conflicts: 1" >> $testroot/stdout.expected
 	echo -n "$short_orig_commit2 -> merge conflict" \
 		>> $testroot/stdout.expected
-	echo ": committing to alpha on newbranch" >> $testroot/stdout.expected
+	echo ": changing alpha and adding new on newbranch" \
+		>> $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret=$?
 	if [ $ret -ne 0 ]; then
@@ -490,9 +495,15 @@ test_rebase_abort() {
 		return 1
 	fi
 
+	# unrelated file in work tree added during conflict resolution
+	touch $testroot/wt/added-file
+	(cd $testroot/wt && got add added-file > /dev/null)
+
 	(cd $testroot/wt && got status > $testroot/stdout)
 
-	echo "C  alpha" > $testroot/stdout.expected
+	echo "A  added-file" > $testroot/stdout.expected
+	echo "C  alpha" >> $testroot/stdout.expected
+	echo "A  epsilon/new" >> $testroot/stdout.expected
 	echo "?  unversioned-file" >> $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret=$?
@@ -508,7 +519,10 @@ test_rebase_abort() {
 
 	echo "Switching work tree to refs/heads/master" \
 		> $testroot/stdout.expected
+	echo 'R  added-file' >> $testroot/stdout.expected
 	echo 'R  alpha' >> $testroot/stdout.expected
+	echo 'R  epsilon/new' >> $testroot/stdout.expected
+	echo 'G  added-file' >> $testroot/stdout.expected
 	echo 'U  beta' >> $testroot/stdout.expected
 	echo "Rebase of refs/heads/newbranch aborted" \
 		>> $testroot/stdout.expected
@@ -556,7 +570,8 @@ test_rebase_abort() {
 	fi
 
 	(cd $testroot/wt && got status > $testroot/stdout)
-	echo "?  unversioned-file" > $testroot/stdout.expected
+	echo "?  added-file" > $testroot/stdout.expected
+	echo "?  unversioned-file" >> $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret=$?
 	if [ $ret -ne 0 ]; then