On 23-04-12 02:26PM, Stefan Sperling wrote:
> 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?

Nice! Thanks for the explanation, it all makes sense and is good to
know. I thought this was going to be a lot more work to fix.

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
> 

-- 
Mark Jamsek <fnc.bsdbox.org|got.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68