From: Stefan Sperling <stsp@stsp.name>
Subject: Re: commit coverage of new backout/cherrypick logmsg refs
To: Mark Jamsek <mark@jamsek.com>
Cc: Game of Trees <gameoftrees@openbsd.org>
Date: Sun, 29 Jan 2023 14:41:43 +0100

On Sun, Jan 29, 2023 at 11:31:11PM +1100, Mark Jamsek wrote:
> The below diff adds a test covering the commit side of the new logmsg
> ref feature, and incidentally, albeit not completely, covers the revert
> aspect too.
> 
> We test for:
>   - multiple logmsg refs being used in one commit
>   - only relevant logmsg refs being used in a commit involving multiple
>     bo/cy operations preceding a revert
>   - logmsg ref is used when affected paths are committed following
>     a partial revert
>   - logmsg ref is neither used nor deleted when the affected paths are
>     not included in a commit
>   - previously unused logmsg ref is subsequently used when the affected
>     paths are committed
>   - we don't litter the tree with got-logmsg-* tmp files
> 
> On the last point, the diff also includes a change to cmd_commit() to
> delete the merged_log tmp file.

Very nice, ok.

> 
> diffstat /home/mark/src/got
>  M  got/got.c                  |    3+  0-
>  M  regress/cmdline/commit.sh  |  259+  0-
> 
> 2 files changed, 262 insertions(+), 0 deletions(-)
> 
> diff /home/mark/src/got
> commit - 71a61c8ccc19248c397974fbd63b952d2665771c
> path + /home/mark/src/got
> blob - 872cc74858a6d9a7d5be7fffffe97fc4f28c9927
> file + got/got.c
> --- got/got.c
> +++ got/got.c
> @@ -9203,6 +9203,9 @@ done:
>  	    error == NULL)
>  		error = got_error_from_errno2("unlink", cl_arg.logmsg_path);
>  	free(cl_arg.logmsg_path);
> +	if (merged_logmsg && unlink(merged_logmsg) == -1 && error == NULL)
> +		error = got_error_from_errno2("unlink", merged_logmsg);
> +	free(merged_logmsg);
>  	if (repo) {
>  		const struct got_error *close_err = got_repo_close(repo);
>  		if (error == NULL)
> blob - bf310d5b1b095c5545625bae6cba5acc7abc8a66
> file + regress/cmdline/commit.sh
> --- regress/cmdline/commit.sh
> +++ regress/cmdline/commit.sh
> @@ -1719,6 +1719,264 @@ test_parseargs "$@"
>  	test_done "$testroot" 0
>  }
>  
> +test_commit_logmsg_ref() {
> +	local testroot=`test_init commit_logmsg_ref`
> +
> +	got checkout $testroot/repo $testroot/wt > /dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	(cd $testroot/repo && git checkout -q -b newbranch)
> +
> +	local bo_logmsg_prefix="log message of backed-out commit"
> +	local cy_logmsg_prefix="log message of cherrypicked commit"
> +	local branch_rev_logmsg="changes on newbranch to cherrypick"
> +	local branch_rev2_logmsg="modified zeta on newbranch to cherrypick"
> +
> +	echo "modified delta on branch" > $testroot/repo/gamma/delta
> +	echo "modified alpha on branch" > $testroot/repo/alpha
> +	(cd $testroot/repo && git rm -q beta)
> +	echo "new file on branch" > $testroot/repo/epsilon/new
> +	(cd $testroot/repo && git add epsilon/new)
> +
> +	git_commit $testroot/repo -m "$branch_rev_logmsg"
> +	local branch_rev=`git_show_head $testroot/repo`
> +
> +	echo "modified zeta on branch" > $testroot/repo/epsilon/zeta
> +
> +	git_commit $testroot/repo -m "$branch_rev2_logmsg"
> +	local branch_rev2=`git_show_head $testroot/repo`
> +
> +	(cd $testroot/wt && got cherrypick $branch_rev > /dev/null)
> +	(cd $testroot/wt && got cherrypick $branch_rev2 > /dev/null)
> +
> +	cat > $testroot/editor.sh <<EOF
> +#!/bin/sh
> +sed -i 's/# l/l/' "\$1"
> +EOF
> +	chmod +x $testroot/editor.sh
> +
> +	(cd $testroot/wt && env VISUAL="$testroot/editor.sh" \
> +	    got commit > /dev/null)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "'got commit' failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	# check that multiple cherrypicked log messages populate the editor
> +	local first=`printf '%s\n%s' $branch_rev $branch_rev2 | sort | head -1`
> +	local second=`printf '%s\n%s' $branch_rev $branch_rev2 | sort | tail -1`
> +
> +	if [ $branch_rev == $first ]; then
> +		local first_msg=$branch_rev_logmsg
> +		local second_msg=$branch_rev2_logmsg
> +	else
> +		local first_msg=$branch_rev2_logmsg
> +		local second_msg=$branch_rev_logmsg
> +	fi
> +
> +	echo " $cy_logmsg_prefix $first:" > $testroot/stdout.expected
> +	echo " $first_msg" >> $testroot/stdout.expected
> +	echo " " >> $testroot/stdout.expected
> +	echo " $cy_logmsg_prefix $second:" >> $testroot/stdout.expected
> +	echo " $second_msg" >> $testroot/stdout.expected
> +	echo " " >> $testroot/stdout.expected
> +
> +	(cd $testroot/wt && got log -l2 | \
> +	    grep -A2 'log message' | sed '/^--/d' > $testroot/stdout)
> +
> +	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
> +
> +	# check that only the relevant log message populates the editor
> +	# when the changes from one of two backout commits are reverted
> +	got checkout $testroot/repo $testroot/wt2 > /dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	(cd $testroot/wt2 && got backout $branch_rev > /dev/null)
> +	(cd $testroot/wt2 && got backout $branch_rev2 > /dev/null)
> +	(cd $testroot/wt2 && got revert epsilon/zeta > /dev/null)
> +
> +	(cd $testroot/wt2 && env VISUAL="$testroot/editor.sh" \
> +	    got commit > /dev/null)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "'got commit' failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	echo " $bo_logmsg_prefix $branch_rev:" > $testroot/stdout.expected
> +	echo " $branch_rev_logmsg" >> $testroot/stdout.expected
> +	echo " " >> $testroot/stdout.expected
> +
> +	(cd $testroot/wt2 && got log -l1 | \
> +	    grep -A2 'log message' > $testroot/stdout)
> +
> +	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
> +
> +	# check that a cherrypicked log message is still
> +	# used when its changes are only partially reverted
> +	branch_rev_logmsg="changes to cherrypick and partially revert"
> +
> +	echo "newline in alpha" >> $testroot/repo/alpha
> +	echo "modified epsilon/zeta on branch" > $testroot/repo/epsilon/zeta
> +
> +	git_commit $testroot/repo -m "$branch_rev_logmsg"
> +	branch_rev=`git_show_head $testroot/repo`
> +
> +	(cd $testroot/wt && got cherrypick $branch_rev > /dev/null)
> +	(cd $testroot/wt && got revert alpha > /dev/null)
> +
> +	(cd $testroot/wt && env VISUAL="$testroot/editor.sh" \
> +	    got commit > /dev/null)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "'got commit' failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	echo " $cy_logmsg_prefix $branch_rev:" > $testroot/stdout.expected
> +	echo " $branch_rev_logmsg" >> $testroot/stdout.expected
> +	echo " " >> $testroot/stdout.expected
> +
> +	(cd $testroot/wt && got log -l1 | \
> +	    grep -A2 'log message' > $testroot/stdout)
> +
> +	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
> +
> +	# check we don't use and consequently delete the logmsg ref of a
> +	# cherrypicked commit when omitting its changed path from the commit
> +	branch_rev_logmsg="changes to cherrypick but omit from the commit"
> +
> +	echo "changed delta" >> $testroot/repo/gamma/delta
> +
> +	git_commit $testroot/repo -m "$branch_rev_logmsg"
> +	local author_time=`git_show_author_time $testroot/repo`
> +	local d=`date -u -r $author_time +"%a %b %e %X %Y UTC"`
> +	branch_rev=`git_show_head $testroot/repo`
> +
> +	(cd $testroot/wt && got update > /dev/null)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got update failed unexpectedly" >&2
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	(cd $testroot/wt && got cherrypick $branch_rev > /dev/null)
> +
> +	echo "changed alpha" >> $testroot/wt/alpha
> +
> +	(cd $testroot/wt && got commit -m \
> +	    "don't commit cy change to gamma/delta" alpha > /dev/null)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "'got commit' failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	# confirm logmsg ref was not deleted with got cherrypick -l
> +	echo "-----------------------------------------------" \
> +	    > $testroot/stdout.expected
> +	echo "commit $branch_rev (newbranch)" >> $testroot/stdout.expected
> +	echo "from: $GOT_AUTHOR" >> $testroot/stdout.expected
> +	echo "date: $d" >> $testroot/stdout.expected
> +	echo " " >> $testroot/stdout.expected
> +	echo " $branch_rev_logmsg" >> $testroot/stdout.expected
> +	echo " " >> $testroot/stdout.expected
> +	echo " M  gamma/delta" >> $testroot/stdout.expected
> +	echo >> $testroot/stdout.expected
> +
> +	(cd $testroot/wt && got cherrypick -l > $testroot/stdout)
> +
> +	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
> +
> +	# confirm a previously unused logmsg ref is picked up
> +	# when an affected path is actually committed
> +	(cd $testroot/wt && env VISUAL="$testroot/editor.sh" \
> +	    got commit > /dev/null)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "'got commit' failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	echo " $cy_logmsg_prefix $branch_rev:" > $testroot/stdout.expected
> +	echo " $branch_rev_logmsg" >> $testroot/stdout.expected
> +	echo " " >> $testroot/stdout.expected
> +
> +	(cd $testroot/wt && got log -l1 | \
> +	    grep -A2 'log message' > $testroot/stdout)
> +
> +	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
> +
> +	# make sure we are not littering work trees
> +	# by leaving temp got-logmsg-* files behind
> +	echo -n > $testroot/stdout.expected
> +	(cd $testroot/wt && got status > $testroot/stdout)
> +
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "$testroot/wt is not clean"
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	(cd $testroot/wt2 && got status > $testroot/stdout)
> +
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "$testroot/repo is not clean"
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
>  test_parseargs "$@"
>  run_test test_commit_basic
>  run_test test_commit_new_subdir
> @@ -1749,3 +2007,4 @@ run_test test_commit_bad_author
>  run_test test_commit_large_file
>  run_test test_commit_gitignore
>  run_test test_commit_bad_author
> +run_test test_commit_logmsg_ref
> 
> -- 
> Mark Jamsek <fnc.bsdbox.org>
> GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68