Omar Polo <op@omarpolo.com> wrote:
> Stefan Sperling <stsp@stsp.name> wrote:
> > On Mon, Jul 11, 2022 at 09:12:48PM +0200, Omar Polo wrote:
> > > while talking on irc this issue pop up: there can be some strange
> > > interactions between the commands in the histedit script.
> > > 
> > > while none of this is really surprising, preventing `mesg' in some
> > > situations where the outcome is non-sensical.  In particular, a fold or
> > > drop followed by a mesg.  (in the first case the commit would be first
> > > reworded and then deleted, in the second case it would be reworded and
> > > then the new text discarded by fold.)
> > > 
> > > so, the only valid options are pick -> mesg and edit -> mesg (which is
> > > used by the regress test at least.)  It would be nice to prevent mesg ->
> > > mesg too maybe.
> > 
> > Indeed, two consecutive mesg lines should also be a syntax error.
> 
> on a second thought, here's a slightly different version that also
> catches the consecutive mesg (and adds a regress for it ;)
> 
> I tried also to add a sentence to the manpage, but suggestion for a
> better phrasing are welcome (or i can drop it and let you fix the manpage)

second thought of a second thought, i like this one more:

diff /home/op/w/got
commit - c5d51f20832122bdce117ecbbe7dd460627531cf
path + /home/op/w/got
blob - beb2b8ca9f072ebfef556186c6c61d244bd11d69
file + got/got.1
--- got/got.1
+++ got/got.1
@@ -2113,6 +2113,7 @@ argument provides a new single-line log message to use
 If the
 .Ar log-message
 argument is omitted, open an editor where a new log message can be written.
+Can only be used immediately after a pick or edit command.
 .El
 .Pp
 Every commit in the history being edited must be mentioned in the script.
blob - 77ee3290c2b299ba86499c5b196115d3f54e96b6
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -10699,7 +10699,7 @@ histedit_parse_list(struct got_histedit_list *histedit
 	char *line = NULL, *p, *end;
 	size_t i, size;
 	ssize_t len;
-	int lineno = 0;
+	int lineno = 0, lastcmd = -1;
 	const struct got_histedit_cmd *cmd;
 	struct got_object_id *commit_id = NULL;
 	struct got_histedit_list_entry *hle = NULL;
@@ -10743,7 +10743,8 @@ histedit_parse_list(struct got_histedit_list *histedit
 		while (isspace((unsigned char)p[0]))
 			p++;
 		if (cmd->code == GOT_HISTEDIT_MESG) {
-			if (hle == NULL || hle->logmsg != NULL) {
+			if (lastcmd != GOT_HISTEDIT_PICK &&
+			    lastcmd != GOT_HISTEDIT_EDIT) {
 				err = got_error(GOT_ERR_HISTEDIT_CMD);
 				break;
 			}
@@ -10760,6 +10761,7 @@ histedit_parse_list(struct got_histedit_list *histedit
 			}
 			free(line);
 			line = NULL;
+			lastcmd = cmd->code;
 			continue;
 		} else {
 			end = p;
@@ -10786,6 +10788,7 @@ histedit_parse_list(struct got_histedit_list *histedit
 		free(line);
 		line = NULL;
 		TAILQ_INSERT_TAIL(histedit_cmds, hle, entry);
+		lastcmd = cmd->code;
 	}
 
 	free(line);
blob - b15d00ef680f545b9c19d914b7c9d87cf23b1c8f
file + regress/cmdline/histedit.sh
--- regress/cmdline/histedit.sh
+++ regress/cmdline/histedit.sh
@@ -740,7 +740,6 @@ test_histedit_fold_last_commit() {
 
 	echo "pick $old_commit1" > $testroot/histedit-script
 	echo "fold $old_commit2" >> $testroot/histedit-script
-	echo "mesg committing folded changes" >> $testroot/histedit-script
 
 	(cd $testroot/wt && got histedit -F $testroot/histedit-script \
 		> $testroot/stdout 2> $testroot/stderr)
@@ -1973,6 +1972,125 @@ EOF
 	test_done "$testroot" $ret
 }
 
+test_histedit_mesg_invalid() {
+	local testroot=`test_init mesg_invalid`
+
+	local orig_commit=`git_show_head $testroot/repo`
+
+	echo "modified alpha on master" > $testroot/repo/alpha
+	(cd $testroot/repo && git rm -q beta)
+	echo "new file on master" > $testroot/repo/epsilon/new
+	(cd $testroot/repo && git add epsilon/new)
+	git_commit $testroot/repo -m 'committing changes'
+	local old_commit1=`git_show_head $testroot/repo`
+
+	echo "modified zeta on master" > $testroot/repo/epsilon/zeta
+	git_commit $testroot/repo -m 'committing to zeto on master'
+	local old_commit2=`git_show_head $testroot/repo`
+
+	got checkout -c $orig_commit $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+	fi
+
+	# try with a leading mesg
+
+	echo "mesg something something" > $testroot/histedit-script
+	echo "pick $old_commit1" >> $testroot/histedit-script
+	echo "pick $old_commit2" >> $testroot/histedit-script
+
+	(cd $testroot/wt && got histedit -F $testroot/histedit-script \
+		> $testroot/stdout 2> $testroot/stderr)
+	ret=$?
+	if [ $ret -eq 0 ]; then
+		echo "histedit succeeded unexpectedly" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	echo "got: bad histedit command" > $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
+
+	# try again with mesg -> mesg
+
+	echo "pick $old_commit1" > $testroot/histedit-script
+	echo "mesg something something" >> $testroot/histedit-script
+	echo "mesg something something else" >> $testroot/histedit-script
+	echo "pick $old_commit2" >> $testroot/histedit-script
+
+	(cd $testroot/wt && got histedit -F $testroot/histedit-script \
+		> $testroot/stdout 2> $testroot/stderr)
+	ret=$?
+	if [ $ret -eq 0 ]; then
+		echo "histedit succeeded unexpectedly" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	echo "got: bad histedit command" > $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
+
+	# try again with drop -> mesg
+
+	echo "drop $old_commit1" > $testroot/histedit-script
+	echo "mesg something something" >> $testroot/histedit-script
+	echo "pick $old_commit2" >> $testroot/histedit-script
+
+	(cd $testroot/wt && got histedit -F $testroot/histedit-script \
+		> $testroot/stdout 2> $testroot/stderr)
+	ret=$?
+	if [ $ret -eq 0 ]; then
+		echo "histedit succeeded unexpectedly" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	echo "got: bad histedit command" > $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
+
+	# try again with fold -> mesg
+
+	echo "fold $old_commit1" > $testroot/histedit-script
+	echo "mesg something something" >> $testroot/histedit-script
+	echo "pick $old_commit2" >> $testroot/histedit-script
+
+	(cd $testroot/wt && got histedit -F $testroot/histedit-script \
+		> $testroot/stdout 2> $testroot/stderr)
+	ret=$?
+	if [ $ret -eq 0 ]; then
+		echo "histedit succeeded unexpectedly" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	echo "got: bad histedit command" > $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+	fi
+	test_done "$testroot" $ret
+}
+
 test_parseargs "$@"
 run_test test_histedit_no_op
 run_test test_histedit_swap
@@ -1993,3 +2111,4 @@ run_test test_histedit_fold_only
 run_test test_histedit_fold_only_empty_logmsg
 run_test test_histedit_edit_only
 run_test test_histedit_prepend_line
+run_test test_histedit_mesg_invalid