"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
Re: Interactions between worktrees
To:
Christian Weisgerber <naddy@mips.inka.de>
Cc:
gameoftrees@openbsd.org
Date:
Thu, 7 Apr 2022 23:53:59 +0200

Download raw body.

Thread
On Thu, Apr 07, 2022 at 09:57:29PM +0200, Christian Weisgerber wrote:
> Using different work trees that are checked out from the same
> repository can lead to various interactions, because many operations
> affect the repository state and thus indirectly the other worktree.
> 
> This one is interesting:
> 
> $ got up -b const
> got: a rebase operation is in progress in this work tree and must be continued or aborted first
> $ got rb -a       
> got: reference refs/got/worktree/rebase/commit-e001ae79-bf70-4253-bac2-bdced68c8189 not found
> 
> How did I get there?
> 
> $ got rb const                                                
> got: cannot rebase branch which contains changes outside of this work tree's path prefix                            
> 
> So I ran the rebase operation in a second work tree that contained
> the path in question.  But now my first work tree is stuck in limbo,
> see above.  I can't abort or continue the rebase, and I can't switch
> to a different branch.
> 
> I guess that's a case of "don't do that then", but, hmm, the result
> is a bit unfortunate.  Is there a way to recover?

Deleting all other rebase-related references should recover your work tree:

got ref -d refs/got/worktree/rebase/branch-e001ae79-bf70-4253-bac2-bdced68c8189
got ref -d refs/got/worktree/rebase/newbase-e001ae79-bf70-4253-bac2-bdced68c8189
got ref -d refs/got/worktree/rebase/tmp-e001ae79-bf70-4253-bac2-bdced68c8189

The ref which is missing is only created if we manage to attempt a file
merge during the rebase operation. However, the path-prefix check currently
runs between the rebase_prepare() step and the merge_files() step.
So if the path-prefix check errors out we end up not creating a 'commit-'
reference, which is a problem I overlooked.

'got rebase -a' is now failing because it first attempts to run the
rebase_continue() function in order to read in prior rebase state,
which is stored in these 4 references, one of which is now missing.
This is why you now run into "got: reference ... not found"

It is impossible to make this reliable in all cases. Our implementation
needs to create 4 files on disk in order to store rebase state. If we fail
to create all of them for any reason the state will be incomplete. The next
time we run we cannot tell why files are missing and we cannot try to
recover automatically. For all we know, someone deleted a ref by accident
with 'got ref -d' or rm(1) and caused trouble.

However, the path-prefix check is an expected failure case so we should
handle it better. My proposed patch below moves the rebase_prepare()
step further down such that we verify the path prefix before creating
any references. With this change your work tree remains usable.
The patch also extends a test case to reproduce the problem you have
found and to validate the suggested fix.

This is the best I can do, unless someone has a better idea.

It is possible that histedit and merge have the same problem.
I haven't looked yet.

diff 9d34261edbfb3574a41b29233fdc722b526ea226 /home/stsp/src/got
blob - d75debc645426b7976fa18ab9a53d9d7e3391ba8
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -9454,10 +9454,6 @@ cmd_rebase(int argc, char *argv[])
 			print_update_progress_stats(&upa);
 			goto done;
 		}
-		error = got_worktree_rebase_prepare(&new_base_branch,
-		    &tmp_branch, &fileindex, worktree, branch, repo);
-		if (error)
-			goto done;
 	}
 
 	commit_id = branch_head_commit_id;
@@ -9468,16 +9464,6 @@ cmd_rebase(int argc, char *argv[])
 	parent_ids = got_object_commit_get_parent_ids(commit);
 	pid = STAILQ_FIRST(parent_ids);
 	if (pid == NULL) {
-		if (!continue_rebase) {
-			error = got_worktree_rebase_abort(worktree, fileindex,
-			    repo, new_base_branch, abort_progress, &upa);
-			if (error)
-				goto done;
-			printf("Rebase of %s aborted\n",
-			    got_ref_get_name(branch));
-			print_merge_progress_stats(&upa);
-
-		}
 		error = got_error(GOT_ERR_EMPTY_REBASE);
 		goto done;
 	}
@@ -9489,6 +9475,13 @@ cmd_rebase(int argc, char *argv[])
 	if (error)
 		goto done;
 
+	if (!continue_rebase) {
+		error = got_worktree_rebase_prepare(&new_base_branch,
+		    &tmp_branch, &fileindex, worktree, branch, repo);
+		if (error)
+			goto done;
+	}
+
 	if (STAILQ_EMPTY(&commits)) {
 		if (continue_rebase) {
 			error = rebase_complete(worktree, fileindex,
blob - 5e74b959165a7294b2db81a0c1423f2573dd875f
file + regress/cmdline/rebase.sh
--- regress/cmdline/rebase.sh
+++ regress/cmdline/rebase.sh
@@ -788,7 +788,60 @@ test_rebase_path_prefix() {
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
 	fi
+
+	# rebase should succeed when using a complete work tree
+	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 rebase newbranch \
+		> $testroot/stdout 2> $testroot/stderr)
+
+	(cd $testroot/repo && git checkout -q newbranch)
+	local new_commit1=`git_show_parent_commit $testroot/repo`
+	local new_commit2=`git_show_head $testroot/repo`
+
+	local short_orig_commit2=`trim_obj_id 28 $orig_commit2`
+	local short_new_commit2=`trim_obj_id 28 $new_commit2`
+
+	echo "G  gamma/delta" > $testroot/stdout.expected
+	echo -n "$short_orig_commit2 -> $short_new_commit2" \
+		>> $testroot/stdout.expected
+	echo ": committing to delta on newbranch" \
+		>> $testroot/stdout.expected
+	echo "Switching work tree to refs/heads/newbranch" \
+		>> $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
+
+	# the first work tree should remain usable
+	(cd $testroot/wt && got update -b master \
+		> $testroot/stdout 2> $testroot/stderr)
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "update failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	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"
 }
 
@@ -878,8 +931,7 @@ test_rebase_no_commits_to_rebase() {
 		return 1
 	fi
 
-	echo "Rebase of refs/heads/newbranch aborted" \
-		> $testroot/stdout.expected
+	echo -n > $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret=$?
 	if [ $ret -ne 0 ]; then