From: Omar Polo Subject: Re: forbid overlapping repo + work trees To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Mon, 28 Aug 2023 18:40:35 +0200 On 2023/08/28 16:40:22 +0200, Stefan Sperling wrote: > Some people seem eager to want to use Got like they use Git and check out > a work tree into a repository or vice versa, and then report errors to me > in private then something doesn't work. > > The initial checkout currently works, but subsequent commands can fail in > various ways due to surprising side effects of unveil configurations. > We have always assumed that these directories will be kept separate while > writing our unveil rules. > > I see little point in supporting such use cases and adding test coverage > for them to ensure that they keep working. It's just extra busy work for us, > for no good reason. Instead, 'got checkout' should reject such attempts very > early on, to prevent people from getting the idea that Got could be used in > this way. > > Tests are still passing with this diff. > > ok? OK for me. The opposite would require much more work and I'm not seeing a reason to do so. few nits below. > ----------------------------------------------- > prevent overlapping repo and work tree in 'got checkout' > > Some people are eager to try to make Got work just like Git by overlaying > the repository and work tree. This causes problems with unveil conflicts > at run-time. > Fail as early as possible during 'got checkout' when users attempt this. > > diff 9c6f408e227e19f58acfc34e96b12d9de29581fd ff0a4bbe58b81c63940a51dfccc919139be64600 > commit - 9c6f408e227e19f58acfc34e96b12d9de29581fd > commit + ff0a4bbe58b81c63940a51dfccc919139be64600 > blob - 29feb8935ab716a9191034d2ebede3da7cea1f73 > blob + 6b0ac0a793a06fb02e013282d02b3e9541e02971 > --- got/got.c > +++ got/got.c > @@ -3077,6 +3077,16 @@ cmd_checkout(int argc, char *argv[]) > got_path_strip_trailing_slashes(repo_path); > got_path_strip_trailing_slashes(worktree_path); > > + if (got_path_is_child(worktree_path, repo_path, strlen(repo_path)) || > + got_path_is_child(repo_path, worktree_path, > + strlen(worktree_path))) { I'd still use 4 spaces to indent this line so it's visually obvious that's part of the if while skimming through the code. > + error = got_error_fmt(GOT_ERR_BAD_PATH, > + "work tree and repository paths may not overlap: %s", > + worktree_path); > + goto done; > + } > + > + > error = got_repo_pack_fds_open(&pack_fds); > if (error != NULL) > goto done; > blob - 993c64afcbd5f4598770774738e513c4afb9387c > blob + 54fdac57ec03cd5202c851099383637b93d6d7df > --- regress/cmdline/checkout.sh > +++ regress/cmdline/checkout.sh > @@ -146,6 +146,90 @@ test_checkout_sets_xbit() { > > } > > +test_checkout_into_repo() { > + local testroot=`test_init checkout_into_repo` > + local commit_id=`git_show_head $testroot/repo` > + > + echo "A $testroot/wt/alpha" > $testroot/stdout.expected > + echo "A $testroot/wt/beta" >> $testroot/stdout.expected > + echo "A $testroot/wt/epsilon/zeta" >> $testroot/stdout.expected > + echo "A $testroot/wt/gamma/delta" >> $testroot/stdout.expected > + echo "Checked out refs/heads/master: $commit_id" \ > + >> $testroot/stdout.expected > + echo "Now shut up and hack" >> $testroot/stdout.expected All this chunk to populate stdout.expected is redundant since... > + got checkout $testroot/repo $testroot/repo/wt \ > + > $testroot/stdout 2> $testroot/stderr > + ret=$? > + if [ $ret -eq 0 ]; then > + echo "checkout succeeded unexpectedly" >&2 > + test_done "$testroot" "1" > + return 1 > + fi > + > + echo -n > $testroot/stdout.expected here we clobber it anyway. > + 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 > + > + echo -n "got: work tree and repository paths may not overlap: " \ > + > $testroot/stderr.expected > + echo "$testroot/repo/wt: bad path" >> $testroot/stderr.expected > + cmp -s $testroot/stderr.expected $testroot/stderr > + ret=$? > + if [ $ret -ne 0 ]; then > + diff -u $testroot/stderr.expected $testroot/stderr > + fi > + test_done "$testroot" "$ret" > +} > + > +test_checkout_overlap_repo() { > + local testroot=`test_init checkout_into_repo` > + local commit_id=`git_show_head $testroot/repo` > + > + echo "A $testroot/wt/alpha" > $testroot/stdout.expected > + echo "A $testroot/wt/beta" >> $testroot/stdout.expected > + echo "A $testroot/wt/epsilon/zeta" >> $testroot/stdout.expected > + echo "A $testroot/wt/gamma/delta" >> $testroot/stdout.expected > + echo "Checked out refs/heads/master: $commit_id" \ > + >> $testroot/stdout.expected > + echo "Now shut up and hack" >> $testroot/stdout.expected similarly here. > + got checkout $testroot/repo $testroot \ > + > $testroot/stdout 2> $testroot/stderr > + ret=$? > + if [ $ret -eq 0 ]; then > + echo "checkout succeeded unexpectedly" >&2 > + test_done "$testroot" "1" > + return 1 > + fi > + > + echo -n > $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 > + > + echo -n "got: work tree and repository paths may not overlap: " \ > + > $testroot/stderr.expected > + echo "$testroot: bad path" >> $testroot/stderr.expected > + cmp -s $testroot/stderr.expected $testroot/stderr > + ret=$? > + if [ $ret -ne 0 ]; then > + diff -u $testroot/stderr.expected $testroot/stderr > + fi > + test_done "$testroot" "$ret" > +} > + > test_checkout_sets_xbit() { > local testroot=`test_init checkout_sets_xbit 1` > > @@ -1010,6 +1094,8 @@ run_test test_checkout_sets_xbit > run_test test_checkout_basic > run_test test_checkout_dir_exists > run_test test_checkout_dir_not_empty > +run_test test_checkout_into_repo > +run_test test_checkout_overlap_repo > run_test test_checkout_sets_xbit > run_test test_checkout_commit_from_wrong_branch > run_test test_checkout_tag