From: Omar Polo Subject: Re: prevent 'got merge' from modifying arbitrary references To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Wed, 21 Jun 2023 13:54:02 +0200 On 2023/06/21 11:53:56 +0200, Stefan Sperling 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 < +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