On Mon, Apr 17, 2023 at 12:38:41AM +1000, Mark Jamsek wrote:
> On 23-04-17 12:02AM, Mark Jamsek wrote:
> > On 23-04-16 10:43PM, Mark Jamsek wrote:
> > > On 23-04-16 10:40PM, Mark Jamsek wrote:
> > > > On 23-04-14 03:52PM, Stefan Sperling wrote:
> > > > > On Mon, Mar 13, 2023 at 12:01:42AM +0000, James Cook wrote:
> > > > > > I recently noticed the following bugs. I just verified that they still
> > > > > > happen with the lastest got, commit 264c43dd.
> > > > > > 
> > > > > > Two of them have scripts to reproduce; the other's pretty simple.
> > > > >  
> > > > > I would like to fix these issues but haven't found time to take
> > > > > a closer look yet, sorry.
> > > > 
> > 
> > In regards to the first two bugs, the second branch and merge commit
> > appear to be a red herring: simply committing to main in another work
> > tree has the same effect:
> 
> No luck debugging this yet, but here's a quick regress test:

Thanks!
Below is a fix with your test adjusted to expect correct behaviour.

The bug is that the first parent's object ID was derived from the work
tree's base commit. It should be derived from the branch tip commit.
The comitted tree is already based on the branch tip's tree which is
why the added file did not disappear.

It is unfortunate that the test suite didn't already catch such a basic
problem, but here we are. All other tests are still passing for me.

diff /home/stsp/src/got
commit - c736b84ab8efb53399d58afe57a2e40c4c7dd1b5
path + /home/stsp/src/got
blob - df29e65f90431310315474962e5c738ec544f38d
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -6252,7 +6252,7 @@ commit_worktree(struct got_object_id **new_commit_id,
 	if (err)
 		goto done;
 
-	err = got_object_qid_alloc(&pid, worktree->base_commit_id);
+	err = got_object_qid_alloc(&pid, head_commit_id);
 	if (err)
 		goto done;
 	STAILQ_INSERT_TAIL(&parent_ids, pid, entry);
blob - 26a31c050dfc034e8c024f4e945dafbc2d72cf28
file + regress/cmdline/commit.sh
--- regress/cmdline/commit.sh
+++ regress/cmdline/commit.sh
@@ -2077,6 +2077,41 @@ test_parseargs "$@"
 	test_done "$testroot" "$ret"
 }
 
+test_commit_from_different_worktrees() {
+	local testroot=$(test_init commit_from_different_worktrees)
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	got checkout $testroot/repo $testroot/wt2 > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "new file" > $testroot/wt2/new
+	(cd $testroot/wt2 && got add new >/dev/null)
+	(cd $testroot/wt2 && got commit -m 'add new file from wt2' > \
+	    $testroot/stdout)
+	local wt2_head_id=$(git_show_head $testroot/repo)
+
+	echo "modified alpha" > $testroot/wt/alpha
+	(cd $testroot/wt && got commit -m 'mod alpha in wt' > $testroot/stdout)
+	local wt1_parent_id=$(git_show_parent_commit $testroot/repo)
+
+	if [ $wt2_head_id != $wt1_parent_id ]; then
+		echo "commit lost from different work tree" >&2
+		test_done "$testroot" "1"
+	fi
+
+	test_done "$testroot" "0"
+}
+
 test_parseargs "$@"
 run_test test_commit_basic
 run_test test_commit_new_subdir
@@ -2108,3 +2143,4 @@ run_test test_commit_logmsg_ref
 run_test test_commit_gitignore
 run_test test_commit_bad_author
 run_test test_commit_logmsg_ref
+run_test test_commit_from_different_worktrees