From: Stefan Sperling <stsp@stsp.name>
Subject: Re: histedit folding "delete, re-add" is broken
To: Christian Weisgerber <naddy@mips.inka.de>
Cc: gameoftrees@openbsd.org
Date: Tue, 7 Mar 2023 09:37:06 +0100

On Tue, Mar 07, 2023 at 12:24:00AM +0100, Christian Weisgerber wrote:
> Histedit folding of a file deletion and subsequent addition of the
> same file is broken.
> 
> You have a file.
> In commit #1 you delete that file.
> In commit #2 you put a modified version of that file back into place.
> Then you histedit and fold commits #1 and #2.
> 
> I would expect that this operation coalesces the commits into one
> that changes the file.  In reality, the file is simply deleted.
> I guess commit #1 preempts further changes.
> 
> By contrast, Git behaves as I would expect.
> 
> (I wasn't idly looking for a corner case.  One lazy way to, say,
> switch the FreeBSD got port to got-portable would be to simply
> delete the old port, put a newly created one into the same place,
> fold the commits, and let God^Ht sort them out...)
> 
> xfail regression test below.  OK?

Yes, thanks!

This is a delete (local change) vs. add (incoming change) on a file, and
hence a tree conflict (per classification the SVN project has been using).

This appears as a tree conflict during histedit fold because fold is
implemented by merging (cherrypicking) multiple commits into the work 
tree in sequence, with the changes from commit N appearing as local
modifications when commit N+1 gets merged. In 'got status' terms we see
local state 'D alpha', and the change being merged is 'A alpha'.

I agree we should tweak our merge logic to "restore" the added incoming
file even if that file has been locally deleted. There are two similar
cases to consider.

One case is where alpha was renamed, rather than deleted:

  git mv alpha alpha2
  git commit
  echo "unrelated content for unrelated file alpha" > alpha
  git add alpha

And this case which is covered by your test:

  git rm alpha
  git commit
  echo "restore alpha, perhaps with tweaks" > alpha
  git add alpha

In either case a deletion event for file alpha will appear while merging
the changes in sequence, followed by an addition event. In both cases the
desired merge result would likely be to re-add file alpha with its new
content. I don't see an obvious alternative way of resolving these cases.
So your proposal makes sense to me.

> diff /home/naddy/got
> commit - 885e96dfba200f362ddd1d9795740251bcb6e39b
> path + /home/naddy/got
> blob - 7f41dbf2a6228ff4edc4bb8fd6d844625b1b6dc5
> file + regress/cmdline/histedit.sh
> --- regress/cmdline/histedit.sh
> +++ regress/cmdline/histedit.sh
> @@ -1547,6 +1547,63 @@ test_histedit_fold_only() {
>  	test_done "$testroot" "$ret"
>  }
>  
> +test_histedit_fold_delete_add() {
> +	local testroot=`test_init histedit_fold_delete_add`
> +
> +	local orig_commit=`git_show_head $testroot/repo`
> +
> +	(cd $testroot/repo && git rm -q alpha)
> +	git_commit $testroot/repo -m "removing alpha"
> +	local old_commit1=`git_show_head $testroot/repo`
> +
> +	echo "modified alpha" >$testroot/repo/alpha
> +	(cd $testroot/repo && git add alpha)
> +	git_commit $testroot/repo -m "add back modified alpha"
> +	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"
> +		return 1
> +	fi
> +
> +	echo "fold $old_commit1" > $testroot/histedit-script
> +	echo "pick $old_commit2" >> $testroot/histedit-script
> +	echo "mesg folded changes" >> $testroot/histedit-script
> +
> +	(cd $testroot/wt && got histedit -F $testroot/histedit-script \
> +		> $testroot/stdout)
> +
> +	local new_commit1=`git_show_head $testroot/repo`
> +	
> +	local short_old_commit1=`trim_obj_id 28 $old_commit1`
> +	local short_old_commit2=`trim_obj_id 28 $old_commit2`
> +	local short_new_commit1=`trim_obj_id 28 $new_commit1`
> +
> +	echo "D  alpha" > $testroot/stdout.expected
> +	echo "$short_old_commit1 ->  fold commit: removing alpha" \
> +		>> $testroot/stdout.expected
> +	echo "A  alpha" >> $testroot/stdout.expected
> +	echo "$short_old_commit2 -> $short_new_commit1: folded 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
> +
> +	if [ ! -e $testroot/wt/alpha ]; then
> +		ret="xfail fold deleted and added file"
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
>  test_histedit_fold_only() {
>  	local testroot=`test_init histedit_fold_only`
>  
> @@ -2467,6 +2524,7 @@ run_test test_histedit_fold_only
>  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_fold_delete_add
>  run_test test_histedit_fold_only
>  run_test test_histedit_fold_only_empty_logmsg
>  run_test test_histedit_edit_only
> -- 
> Christian "naddy" Weisgerber                          naddy@mips.inka.de
> 
>