"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
fix histedit -e bug
To:
gameoftrees@openbsd.org
Date:
Tue, 11 Jun 2024 14:46:37 +0200

Download raw body.

Thread
  • Stefan Sperling:

    fix histedit -e bug

histedit -e has a bug if all changes are reverted after a merge conflict.

Running histedit -c after reverting conflicts will correctly report a no-op
change, indicating the commit will be dropped. However, the commit is not
marked as skipped in histedit meta data. The next time histedit -c runs
the same commit will be merged again, and conflict again.
This results in a revert + histedit -c cycle that can only be broken out
of with histedit -a.
 
Fix and regression test below -- undo the fix to see the test fail.

OK?


 M  got/got.c                    |    2+  8-
 M  regress/cmdline/histedit.sh  |  148+  0-

2 files changed, 150 insertions(+), 8 deletions(-)

diff 41873b17dbf689280a2f59f9e7187205a5b358f2 d653d34493f9baf49b0a216aa1edbea3b0b695f9
commit - 41873b17dbf689280a2f59f9e7187205a5b358f2
commit + d653d34493f9baf49b0a216aa1edbea3b0b695f9
blob - 724bdeb9f769210987e3c67785f9758c2dc5b3cb
blob + 128d0458991aa5cef9680a2b0ca92d02b65c4caf
--- got/got.c
+++ got/got.c
@@ -13223,16 +13223,10 @@ cmd_histedit(int argc, char *argv[])
 					if (error)
 						goto done;
 				} else {
-					error = got_object_open_as_commit(
-					    &commit, repo, hle->commit_id);
+					error = histedit_skip_commit(hle,
+					    worktree, repo);
 					if (error)
 						goto done;
-					error = show_histedit_progress(commit,
-					    hle, NULL);
-					got_object_commit_close(commit);
-					commit = NULL;
-					if (error)
-						goto done;
 				}
 			}
 			continue;
blob - b0adfd36a468c7760119682e77e3c30152401592
blob + 758167f8b5e07885c9944d76d57660a8d0a7afc1
--- regress/cmdline/histedit.sh
+++ regress/cmdline/histedit.sh
@@ -2611,6 +2611,153 @@ test_histedit_drop_only() {
 	test_done "$testroot" "$ret"
 }
 
+test_histedit_conflict_revert() {
+	local testroot=`test_init histedit_conflict_revert`
+	local orig_commit=`git_show_head $testroot/repo`
+
+	echo "first change of alpha" > $testroot/repo/alpha
+	git_commit $testroot/repo -m "committing changes"
+	local old_commit1=`git_show_head $testroot/repo`
+	local short_old_commit1=`trim_obj_id 28 $old_commit1`
+
+	echo "second change of alpha" > $testroot/repo/alpha
+	git_commit $testroot/repo -m "committing changes"
+	local old_commit2=`git_show_head $testroot/repo`
+	local short_old_commit2=`trim_obj_id 28 $old_commit2`
+
+	echo "third change of alpha" > $testroot/repo/alpha
+	git_commit $testroot/repo -m "committing changes"
+	local old_commit3=`git_show_head $testroot/repo`
+	local short_old_commit3=`trim_obj_id 28 $old_commit3`
+
+	got checkout -c $orig_commit $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "edit $old_commit1" > $testroot/histedit-script
+	echo "edit $old_commit2" >> $testroot/histedit-script
+	echo "pick $old_commit3" >> $testroot/histedit-script
+
+	(cd $testroot/wt && got histedit -F $testroot/histedit-script \
+		> $testroot/stdout)
+
+	echo "G  alpha" > $testroot/stdout.expected
+	echo "Stopping histedit for amending commit $old_commit1" \
+		>> $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "tweaked first change of alpha" > $testroot/wt/alpha
+
+	cat > $testroot/editor.sh <<EOF
+#!/bin/sh
+ed -s "\$1" <<-EOF
+	,s/.*/committing changes/
+	w
+	EOF
+EOF
+	chmod +x $testroot/editor.sh
+
+	(cd $testroot/wt && env EDITOR="$testroot/editor.sh" \
+		VISUAL="$testroot/editor.sh" \
+		got histedit -c > $testroot/stdout 2> $testroot/stderr)
+
+	local new_commit1=$(cd $testroot/wt && got info | \
+		grep 'work tree base commit:' | cut -d ' ' -f5)
+	local short_new_commit1=`trim_obj_id 28 $new_commit1`
+
+	echo "$short_old_commit1 -> $short_new_commit1: committing changes" \
+		> $testroot/stdout.expected
+	echo "C  alpha" >> $testroot/stdout.expected
+	echo "Files with new merge conflicts: 1" >> $testroot/stdout.expected
+	echo "$short_old_commit2 -> merge conflict: committing changes" \
+		>> $testroot/stdout.expected
+
+	echo "got: conflicts must be resolved before histedit can continue" \
+		> $testroot/stderr.expected
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got revert alpha > /dev/null)
+	(cd $testroot/wt && env EDITOR="$testroot/editor.sh" \
+		VISUAL="$testroot/editor.sh" \
+		got histedit -c > $testroot/stdout 2> $testroot/stderr)
+
+	echo "$short_old_commit2 -> no-op change: committing changes" \
+		> $testroot/stdout.expected
+	echo "C  alpha" >> $testroot/stdout.expected
+	echo "Files with new merge conflicts: 1" >> $testroot/stdout.expected
+	echo "$short_old_commit3 -> merge conflict: committing changes" \
+		>> $testroot/stdout.expected
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got revert alpha > /dev/null)
+	(cd $testroot/wt && env EDITOR="$testroot/editor.sh" \
+		VISUAL="$testroot/editor.sh" \
+		got histedit -c > $testroot/stdout 2> $testroot/stderr)
+
+	echo "$short_old_commit3 -> no-op change: committing changes" \
+		> $testroot/stdout.expected
+	echo "Switching work tree to refs/heads/master" \
+		>> $testroot/stdout.expected
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo -n > $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	test_done "$testroot" "$ret"
+}
+
 test_parseargs "$@"
 run_test test_histedit_no_op
 run_test test_histedit_swap
@@ -2638,3 +2785,4 @@ run_test test_histedit_resets_committer
 run_test test_histedit_umask
 run_test test_histedit_mesg_filemode_change
 run_test test_histedit_drop_only
+run_test test_histedit_conflict_revert