On 2023/06/21 11:53:56 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> 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?

looks fine to me.

I'm in doubt however for the regress, shouldn't merge.sh be more
accurate than fetch.sh?  Ideally one should run all the regresses, but
I can see myself limiting to only the subset of the ones directly
impacted when hacking on something, and only later running the full
blown regress suite.

> -----------------------------------------------
>  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