commit 8641a33271eb0cef324fbeb4440485de8f433134
from: Stefan Sperling <stsp@stsp.name>
via: Thomas Adam <thomas@xteddy.org>
date: Fri Apr 14 16:27:57 2023 UTC

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 + ok naddy@

commit - 4d0b6596a3fd608f26c1ff6c5e445601160ba99f
commit + 8641a33271eb0cef324fbeb4440485de8f433134
blob - 93158be8199310738edca95f736970edafb1c23f
blob + aa1a140fd4731f9766992eb2ba3bac8928d901dd
--- lib/worktree.c
+++ lib/worktree.c
@@ -4697,6 +4697,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 *
@@ -4999,16 +5000,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;
@@ -7324,7 +7342,134 @@ done:
 		err = unlockerr;
 	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("asprintf");
+				goto done;
+			}
+
+			err = got_path_skip_common_ancestor(&wt_path,
+			    path_prefix, abspath);
+			if (err)
+				goto done;
+			free(abspath);
+			abspath = NULL;
+		}
+
+		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,
@@ -7334,15 +7479,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)
@@ -7390,7 +7564,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)
@@ -7403,14 +7578,19 @@ sync:
 	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)
@@ -7706,14 +7886,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)
@@ -7752,7 +7967,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)
@@ -7765,9 +7981,14 @@ sync:
 	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)
@@ -8445,12 +8666,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)
@@ -8477,6 +8728,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)
@@ -8490,12 +8742,15 @@ sync:
 		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
@@ -672,10 +672,15 @@ test_merge_abort() {
 		test_done "$testroot" "$ret"
 		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