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

From:
Stefan Sperling <stsp@stsp.name>
Subject:
prevent 'got merge' from modifying arbitrary references
To:
gameoftrees@openbsd.org
Date:
Wed, 21 Jun 2023 11:53:56 +0200

Download raw body.

Thread
It is possible to switch the work tree to an arbitrary reference
and then run:

	got merge some/other/reference

This will cause a merge commit to be created, and the work tree's
current reference will be changed to point at this merge commit.

There is an awful trap. Imagine someone did this:

  got update -b origin/main
  got merge main

This sequence matches the 'got rebase' workflow so it may seem like
a reasonable thing to do. But the outcome of this sequence is that
origin/main no longer matches the state last fetched by 'got fetch',
which is very bad. I am adding regression tests below which cover
this situation, and also test the more desirable sequence:

  got update -b main
  got merge origin/main

Limiting 'got merge' to first parents in the "refs/heads/" namespace is
a solution that aligns with the restrictions imposed by 'got commit'.
I don't believe this change negatively impacts any workflows that we
should be supporting -- is this correct?

ok?

-----------------------------------------------
 prevent 'got merge' from creating commits on branches outside of "refs/heads/"
 
diff b88936d3f94e26ab32d9ef5d893b39fe633c6485 73097a92238a552f665580cf41942cda976a0a81
commit - b88936d3f94e26ab32d9ef5d893b39fe633c6485
commit + 73097a92238a552f665580cf41942cda976a0a81
blob - 8122fe9273cc37091dc13c83016ef03a2a436be5
blob + b6da5695eaab874ad02b4ba5ad0801b9748b60fb
--- got/got.1
+++ got/got.1
@@ -2837,7 +2837,10 @@ The tip commit of the work tree's current branch, whic
 .Cm got import .
 .Pp
 Merge commits are commits based on multiple parent commits.
-The tip commit of the work tree's current branch, which must be set with
+The tip commit of the work tree's current branch, which must be
+must be in the
+.Dq refs/heads/
+reference namespace and must be set with
 .Cm got update -b
 before starting the
 .Cm merge
@@ -2882,6 +2885,13 @@ If the work tree is not yet fully updated to the tip c
 .Pp
 .Cm got merge
 will refuse to run if certain preconditions are not met.
+If the work tree's current branch is not in the
+.Dq refs/heads/
+reference namespace then the work tree must first be switched to a
+branch in the
+.Dq refs/heads/
+namespace with
+.Cm got update -b .
 If the work tree is not yet fully updated to the tip commit of its
 branch, then the work tree must first be updated with
 .Cm got update .
blob - ea8a08baed68affba73cafa577c3125e9756466a
blob + 4c6ca7ff2f76eaa431b055ab0a6912772b492b38
--- got/got.c
+++ got/got.c
@@ -13264,6 +13264,16 @@ cmd_merge(int argc, char *argv[])
 		goto done; /* nothing else to do */
 	}
 
+	if (strncmp(got_worktree_get_head_ref_name(worktree),
+	    "refs/heads/", 11) != 0) {
+		error = got_error_fmt(GOT_ERR_COMMIT_BRANCH,
+		    "work tree's current branch %s is outside the "
+		    "\"refs/heads/\" reference namespace; "
+		    "update -b required",
+		    got_worktree_get_head_ref_name(worktree));
+		goto done;
+	}
+
 	error = get_author(&author, repo, worktree);
 	if (error)
 		goto done;
blob - bdd7571df06f3c7d45127433eabfcdc76e0e94ed
blob + 55823c98f07c65faeafacd31ad7723d7cbae0af1
--- regress/cmdline/fetch.sh
+++ regress/cmdline/fetch.sh
@@ -1935,6 +1935,171 @@ test_parseargs "$@"
 
 }
 
+test_fetch_branch_and_merge() {
+	local testroot=`test_init fetch_branch_and_merge`
+	local testurl=ssh://127.0.0.1/$testroot
+	local commit_id=`git_show_head $testroot/repo`
+
+	got clone -q $testurl/repo $testroot/repo-clone
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got clone command failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "modified alpha" > $testroot/repo/alpha
+	git_commit $testroot/repo -m "modified alpha"
+	local commit_id2=`git_show_head $testroot/repo`
+
+	got fetch -q -r $testroot/repo-clone > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got fetch command failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo -n > $testroot/stdout.expected
+
+	cmp -s $testroot/stdout $testroot/stdout.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	got ref -l -r $testroot/repo-clone > $testroot/stdout
+
+	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
+	echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected
+	echo "refs/remotes/origin/HEAD: refs/remotes/origin/master" \
+		>> $testroot/stdout.expected
+	echo "refs/remotes/origin/master: $commit_id2" \
+		>> $testroot/stdout.expected
+
+	cmp -s $testroot/stdout $testroot/stdout.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	got checkout $testroot/repo-clone $testroot/wt > /dev/null
+
+	echo "modified beta" > $testroot/wt/beta
+	(cd $testroot/wt && got commit -m "modified beta" > /dev/null)
+	local commit_id3=`git_show_head $testroot/repo-clone`
+
+	(cd $testroot/wt && got update > /dev/null)
+	(cd $testroot/wt && got merge origin/master > $testroot/stdout)
+	local merge_commit_id=`git_show_head $testroot/repo-clone`
+
+	cat > $testroot/stdout.expected <<EOF
+G  alpha
+Merged refs/remotes/origin/master into refs/heads/master: $merge_commit_id
+EOF
+
+	cmp -s $testroot/stdout $testroot/stdout.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+	test_done "$testroot" "$ret"
+}
+
+test_fetch_branch_and_merge_remote() {
+	local testroot=`test_init fetch_branch_and_merge`
+	local testurl=ssh://127.0.0.1/$testroot
+	local commit_id=`git_show_head $testroot/repo`
+
+	got clone -q $testurl/repo $testroot/repo-clone
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got clone command failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "modified alpha" > $testroot/repo/alpha
+	git_commit $testroot/repo -m "modified alpha"
+	local commit_id2=`git_show_head $testroot/repo`
+
+	got fetch -q -r $testroot/repo-clone > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got fetch command failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo -n > $testroot/stdout.expected
+
+	cmp -s $testroot/stdout $testroot/stdout.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	got ref -l -r $testroot/repo-clone > $testroot/stdout
+
+	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
+	echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected
+	echo "refs/remotes/origin/HEAD: refs/remotes/origin/master" \
+		>> $testroot/stdout.expected
+	echo "refs/remotes/origin/master: $commit_id2" \
+		>> $testroot/stdout.expected
+
+	cmp -s $testroot/stdout $testroot/stdout.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	got checkout $testroot/repo-clone $testroot/wt > /dev/null
+
+	echo "modified beta" > $testroot/wt/beta
+	(cd $testroot/wt && got commit -m "modified beta" > /dev/null)
+	local commit_id3=`git_show_head $testroot/repo-clone`
+
+	(cd $testroot/wt && got update -b origin/master > /dev/null)
+	(cd $testroot/wt && got merge master > \
+		$testroot/stdout 2> $testroot/stderr)
+	local merge_commit_id=`git_show_head $testroot/repo-clone`
+
+	echo -n > $testroot/stdout.expected
+
+	cmp -s $testroot/stdout $testroot/stdout.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo -n "got: work tree's current branch refs/remotes/origin/master " \
+		> $testroot/stderr.expected
+	echo -n 'is outside the "refs/heads/" reference namespace; ' \
+		>> $testroot/stderr.expected
+	echo -n "update -b required: will not commit to a branch " \
+		>> $testroot/stderr.expected
+	echo 'outside the "refs/heads/" reference namespace' \
+		>> $testroot/stderr.expected
+
+	cmp -s $testroot/stderr $testroot/stderr.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+	fi
+	test_done "$testroot" "$ret"
+}
+
 test_parseargs "$@"
 run_test test_fetch_basic
 run_test test_fetch_list
@@ -1952,3 +2117,5 @@ run_test test_fetch_from_out_of_date_remote
 run_test test_fetch_delete_remote_refs
 run_test test_fetch_honor_wt_conf_bflag
 run_test test_fetch_from_out_of_date_remote
+run_test test_fetch_branch_and_merge
+run_test test_fetch_branch_and_merge_remote