On 2023/08/28 16:40:22 +0200, Stefan Sperling <stsp@stsp.name> 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