On Sat, Dec 12, 2020 at 11:49:16AM -0500, Josh Rickmar wrote:
> On Sat, Dec 12, 2020 at 05:32:13PM +0100, Stefan Sperling wrote:
> > On Sat, Dec 12, 2020 at 04:01:56PM +0100, Stefan Sperling wrote:
> > > For now, this removes any attempts to clean up after histedit runs into an
> > > error. The user will now see why the histedit operation has failed, and 
> > > may decide whether to fix things up and continue or abort the operation.
> > > 
> > > Our tests seem happy with this.
> > 
> > Testing by jrick uncovered a related issue: While preparing a histedit
> > operation, we don't create a fully complete histedit tracking state.
> > The histedit "commit reference", which points at the commit currently
> > being edited, is not created during preparation. It is currently created
> > only once the histedit operation creates a new commit.
> > 
> > This leaves users unable to abort/continue the histedit operation after
> > saving an empty log message because the code which attempts to resume
> > the histedit operation complains about the missing reference.
> > 
> > This patch includes a fix for that problem as well. I plan to split
> > these changes into two commits.
> > 
> > ok?
> 
> ok jrick
> 
> histedit -m and -f are behaving properly now too with the other patch
> applied, so that is also ok.

As mentioned in my other post just now, this needs more work...

The tests below show that the patches we have so far are not enough.
While 'histedit -a' works after saving an empty log message,
'histedit -c' is not working and would require a further changes.

diff f0c928799728574319378cb66257edaaa1f2ad90 93bb851f5532efb7d2fbe407aed617a9bcb17db6
blob - 1999e16c89af6d8f4e46b5f62559e045f5c8f2fa
blob + 12048600c7f756a24151171a72ce050e96fcaedc
--- regress/cmdline/histedit.sh
+++ regress/cmdline/histedit.sh
@@ -1439,7 +1439,153 @@ test_histedit_fold_add_delete() {
 	test_done "$testroot" "$ret"
 }
 
+test_histedit_empty_logmsg_abort() {
+	local testroot=`test_init histedit_empty_logmsg_abort`
 
+	local orig_commit=`git_show_head $testroot/repo`
+
+	echo "modified alpha on master" > $testroot/repo/alpha
+	git_commit $testroot/repo -m "committing to alpha on master"
+	local old_commit1=`git_show_head $testroot/repo`
+
+	echo "modified zeta on master" > $testroot/repo/epsilon/zeta
+	git_commit $testroot/repo -m "committing to zeta on master"
+	local old_commit2=`git_show_head $testroot/repo`
+
+	got checkout -c $orig_commit $testroot/repo $testroot/wt > /dev/null
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	cat > $testroot/editor.sh <<EOF
+#!/bin/sh
+sed -i 'd' "\$1"
+EOF
+	chmod +x $testroot/editor.sh
+
+	(cd $testroot/wt && env EDITOR="$testroot/editor.sh" \
+		VISUAL="$testroot/editor.sh" got histedit -m \
+		> $testroot/stdout 2> $testroot/stderr)
+	
+	echo -n > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "got: commit message cannot be empty, aborting" \
+		> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got histedit -a > $testroot/stdout)
+
+	echo "Switching work tree to refs/heads/master" \
+		> $testroot/stdout.expected
+	echo "Histedit of refs/heads/master aborted" \
+		>> $testroot/stdout.expected
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+	test_done "$testroot" "$ret"
+}
+
+test_histedit_empty_logmsg_continue() {
+	local testroot=`test_init histedit_empty_logmsg_continue`
+
+	local orig_commit=`git_show_head $testroot/repo`
+
+	echo "modified alpha on master" > $testroot/repo/alpha
+	git_commit $testroot/repo -m "committing to alpha on master"
+	local old_commit1=`git_show_head $testroot/repo`
+
+	echo "modified zeta on master" > $testroot/repo/epsilon/zeta
+	git_commit $testroot/repo -m "committing to zeta on master"
+	local old_commit2=`git_show_head $testroot/repo`
+
+	got checkout -c $orig_commit $testroot/repo $testroot/wt > /dev/null
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	cat > $testroot/editor.sh <<EOF
+#!/bin/sh
+sed -i 'd' "\$1"
+EOF
+	chmod +x $testroot/editor.sh
+
+	(cd $testroot/wt && env EDITOR="$testroot/editor.sh" \
+		VISUAL="$testroot/editor.sh" got histedit -m \
+		> $testroot/stdout 2> $testroot/stderr)
+	
+	echo -n > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "got: commit message cannot be empty, aborting" \
+		> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	set -x
+	cat > $testroot/editor.sh <<EOF
+#!/bin/sh
+sed -i 's/.*/my log new message/' "\$1"
+EOF
+	chmod +x $testroot/editor.sh
+
+	(cd $testroot/wt && env EDITOR="$testroot/editor.sh" \
+		VISUAL="$testroot/editor.sh" got histedit -m -c \
+		> $testroot/stdout)
+
+	local new_commit1=`git_show_parent_commit $testroot/repo`
+	local new_commit2=`git_show_head $testroot/repo`
+
+	local short_new_commit1=`trim_obj_id 28 $new_commit1`
+	local short_new_commit2=`trim_obj_id 28 $new_commit2`
+
+	echo "$short_old_commit1 -> $short_new_commit1: my new log message" \
+		> $testroot/stdout.expected
+	echo "G  alpha" >> $testroot/stdout.expected
+	echo -n "$short_old_commit2 -> $short_new_commit2: " \
+		>> $testroot/stdout.expected
+	echo "committing to zeta on master" >> $testroot/stdout.expected
+	echo "Switching work tree to refs/heads/master" \
+		>> $testroot/stdout.expected
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+	test_done "$testroot" "$ret"
+}
+
 test_parseargs "$@"
 run_test test_histedit_no_op
 run_test test_histedit_swap
@@ -1456,3 +1602,5 @@ run_test test_histedit_fold_last_commit_swap
 run_test test_histedit_split_commit
 run_test test_histedit_duplicate_commit_in_script
 run_test test_histedit_fold_add_delete
+run_test test_histedit_empty_logmsg_abort
+run_test test_histedit_empty_logmsg_continue