On Sat, Apr 01, 2023 at 04:13:54PM +0200, Christian Weisgerber wrote:
> Christian Weisgerber:
> 
> > git clone --bare https://github.com/openbsd/src.git
> > cd src.git
> > git config remote.origin.fetch '+refs/heads/*:refs/remotes/origin/*'
> > git remote add jca https://github.com/jcourreges/openbsd-src.git
> > git fetch --all
> > cd ..
> > got co src.git
> > cd src
> > got br tmp
> > got br -c jca/llvm15-12 llvm15
> > got up -b tmp
> > got rb llvm15
> > got rb -a
> 
> The problem is here, after the aborted rebase.  Despite "got status"
> and "got diff" showing nothing, the worktree is in reality full of
> changes.
 
Yes, this is where the bug is. Essentially this is a bug in the
'got update' machinery. Let me try to explain:

The work tree does indeed have no local changes. However, the
base commit ID of some files is left at a wrong value.

What happens is that the first commit is rebased ok, i.e. changes
are merged into the work tree and then a new commit is created on
a temporary branch which accumulates rebased commits.
When this commit is created the files which are changed by this
commit get a new base commit ID assigned; the ID of the new tip
commit of the temporary branch.

Then another commit is rebased, and this time there are merge
conflicts, and the rebase operation stops. At this point the work
tree's base commit ID is still pointing at the temporary branch tip
and all state is consistent.

When we abort the rebase operation the work tree's global base commit
ID gets reset to that of the pre-rebase branch (but keep in mind that
each file has a separate base commit ID in the file index).
Any local changes are reverted, which doesn't affect the files which
received a new base commit ID because these files appear unmodified
in the work tree (still relative to the first rebased commit).

Then all files are checked out to ensure that files in the work tree
return to the state from before the rebase operation.
The bug is that base commit IDs of affeted files are left as-is.
This base commit ID is of course the wrong one because the files are
now based on the pre-rebase branch again.

The fix is trivial and I've added test coverage for this issue.

When the rebase_abort test is run without the fix in place then
it now fails like this:

$ ./rebase.sh test_rebase_abort
test_rebase_abort --- /tmp/got-test-rebase_abort-p8vJzncg/stdout.expected       Wed Apr 12 13:25:15 2023
+++ /tmp/got-test-rebase_abort-p8vJzncg/stdout  Wed Apr 12 13:25:15 2023
@@ -1 +1,2 @@
 based on commit: 10162cdd439d90b5c3bcd725cae498d10dfd3dd3
+based on commit: ac3716263a661f70593e8e6ddd192e231612961e
test failed; leaving test data in /tmp/got-test-rebase_abort-p8vJzncg
$

And if we make the test skip this base ID commit check by commenting
the relevant lines then the update which the test now runs at the end
is not a no-op. Instead, it updates the file beta. Which matches what
you saw on your llvm branch.

ok?

-----------------------------------------------
 fix rebase -a not resetting base commit IDs of files changed during rebase
 
 Update base commit IDs of unmodified files while checking out files.
 This avoids spurious merge conflicts when the work tree is later used
 for another rebase operation. It also makes 'got update' right after
 'rebase -a' a no-op, as it should be.
 
 Problem found by naddy@ while rebasing jca's llvm15 branch
 
diff cc88020e952af813c1e01b91ab6516969562e972 6a06538c34c30a7ad5174e4f2bf1d669224391ef
commit - cc88020e952af813c1e01b91ab6516969562e972
commit + 6a06538c34c30a7ad5174e4f2bf1d669224391ef
blob - 9e87414b038cb6aa0ef965f162999f1108c5d82a
blob + 9ac685c7ccc7e64878f575b0e825c1fce2c67f19
--- lib/worktree.c
+++ lib/worktree.c
@@ -1996,7 +1996,13 @@ update_blob(struct got_worktree *worktree,
 		if (got_fileindex_entry_has_blob(ie) &&
 		    memcmp(ie->blob_sha1, te->id.sha1,
 		    SHA1_DIGEST_LENGTH) == 0) {
-			/* Different commit but the same blob. */
+			/*
+			 * Different commit but the same blob.
+			 * Update the base commit ID of this file.
+			 */
+			memcpy(ie->commit_sha1,
+			    worktree->base_commit_id->sha1,
+			    sizeof(ie->commit_sha1));
 			err = sync_timestamps(worktree->root_fd,
 			    path, status, ie, &sb);
 			if (err)
blob - 6a4b676aff1b3de6b3384590c038eb5b5045981b
blob + d664d34f3511e6a4a2023e233230f89f2bcec896
--- regress/cmdline/rebase.sh
+++ regress/cmdline/rebase.sh
@@ -413,11 +413,16 @@ test_rebase_abort() {
 	local init_commit=`git_show_head $testroot/repo`
 
 	(cd $testroot/repo && git checkout -q -b newbranch)
-	echo "modified alpha on branch" > $testroot/repo/alpha
-	git_commit $testroot/repo -m "committing to alpha on newbranch"
+	echo "modified beta on branch" > $testroot/repo/beta
+	git_commit $testroot/repo -m "committing to beta on newbranch"
 	local orig_commit1=`git_show_head $testroot/repo`
 	local short_orig_commit1=`trim_obj_id 28 $orig_commit1`
 
+	echo "modified alpha on branch" > $testroot/repo/alpha
+	git_commit $testroot/repo -m "committing to alpha on newbranch"
+	local orig_commit2=`git_show_head $testroot/repo`
+	local short_orig_commit2=`trim_obj_id 28 $orig_commit2`
+
 	(cd $testroot/repo && git checkout -q master)
 	echo "modified alpha on master" > $testroot/repo/alpha
 	git_commit $testroot/repo -m "committing to alpha on master"
@@ -436,9 +441,17 @@ test_rebase_abort() {
 	(cd $testroot/wt && got rebase newbranch > $testroot/stdout \
 		2> $testroot/stderr)
 
-	echo "C  alpha" > $testroot/stdout.expected
+	new_commit1=$(cd $testroot/wt && got info beta | \
+		grep '^based on commit:' | cut -d' ' -f4)
+	local short_new_commit1=`trim_obj_id 28 $new_commit1`
+	
+	echo "G  beta" > $testroot/stdout.expected
+	echo -n "$short_orig_commit1 -> $short_new_commit1" \
+		>> $testroot/stdout.expected
+	echo ": committing to beta on newbranch" >> $testroot/stdout.expected
+	echo "C  alpha" >> $testroot/stdout.expected
 	echo "Files with new merge conflicts: 1" >> $testroot/stdout.expected
-	echo -n "$short_orig_commit1 -> merge conflict" \
+	echo -n "$short_orig_commit2 -> merge conflict" \
 		>> $testroot/stdout.expected
 	echo ": committing to alpha on newbranch" >> $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
@@ -461,12 +474,12 @@ test_rebase_abort() {
 
 	echo '<<<<<<<' > $testroot/content.expected
 	echo "modified alpha on master" >> $testroot/content.expected
-	echo "||||||| 3-way merge base: commit $init_commit" \
+	echo "||||||| 3-way merge base: commit $orig_commit1" \
 		>> $testroot/content.expected
 	echo "alpha" >> $testroot/content.expected
 	echo "=======" >> $testroot/content.expected
 	echo "modified alpha on branch" >> $testroot/content.expected
-	echo ">>>>>>> merged change: commit $orig_commit1" \
+	echo ">>>>>>> merged change: commit $orig_commit2" \
 		>> $testroot/content.expected
 	cat $testroot/wt/alpha > $testroot/content
 	cmp -s $testroot/content.expected $testroot/content
@@ -519,13 +532,36 @@ test_rebase_abort() {
 
 	(cd $testroot/wt && got log -l3 -c newbranch \
 		| grep ^commit > $testroot/stdout)
-	echo "commit $orig_commit1 (newbranch)" > $testroot/stdout.expected
+	echo "commit $orig_commit2 (newbranch)" > $testroot/stdout.expected
+	echo "commit $orig_commit1" >> $testroot/stdout.expected
 	echo "commit $init_commit" >> $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
+
+	(cd $testroot/wt && got info .| \
+		grep '^based on commit:' | sort | uniq > $testroot/stdout)
+	echo "based on commit: $master_commit" > $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
+
+	# A subsequent update should be a no-op.
+	(cd $testroot/wt && got update > $testroot/stdout)
+	echo 'Already up-to-date' > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
 	test_done "$testroot" "$ret"
 }
 
blob - 822844175d6069cb0ad59c5bd870b63d886b9990
blob + c08237c6cd1b14453b8e4f8e39789a3c6c017f1c
--- regress/cmdline/update.sh
+++ regress/cmdline/update.sh
@@ -1899,12 +1899,7 @@ test_update_modified_submodules() {
 	(cd $testroot/repo && git add repo2)
 	git_commit $testroot/repo -m "modified submodule link"
 
-	# This update only records the new base commit. Otherwise it is a
-	# no-op change because Got's file index does not track submodules.
-	echo -n "Updated to refs/heads/master: " > $testroot/stdout.expected
-	git_show_head $testroot/repo >> $testroot/stdout.expected
-	echo >> $testroot/stdout.expected
-
+	echo "Already up-to-date" > $testroot/stdout.expected
 	(cd $testroot/wt && got update > $testroot/stdout)
 
 	cmp -s $testroot/stdout.expected $testroot/stdout