From: Stefan Sperling <stsp@stsp.name>
Subject: Re: fix duplicated tree item with prefix checkout
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Fri, 19 Jun 2026 10:54:41 +0200

On Fri, Jun 19, 2026 at 01:08:47AM +0200, Omar Polo wrote:
> Hello =)
> 
> I've noticed that if I include a trailing '/' when checking out a
> portion of the tree, then `got add' might fail on subdirectories.
> 
> The underlying issue is that we end up with two consecutive '//' which
> messes up the logic for commit, and we end up internally trying to add
> twice (or more) the subdirectories, which fails with a cryptic
> 'duplicated tree item'.
> 
> Diff below improves the error message, add a regression test for this
> case, and hopefully fixes it in a decent way ;)
> 
> (if okay, i'll commit the got_error_path part in a separate commit)

A related problem might be that got_worktree_match_path_prefix() is
using strcmp() rather than got_path_cmp(). Should we also fix that?

> diff /home/op/w/got
> path + /home/op/w/got
> commit - e3df3fa2b325547b4c69fcb62527422eb7d5f108
> blob - 7b2602c1b70e154f1f39d3222ca03af18120b5ff
> file + got/got.c
> --- got/got.c
> +++ got/got.c
> @@ -3189,6 +3189,7 @@ cmd_checkout(int argc, char *argv[])
>  			allow_nonempty = 1;
>  			break;
>  		case 'p':
> +			got_path_strip_trailing_slashes(optarg);

I would avoid modifying argv in-place. It will usually work but if I
recall correctly there are C runtimes which don't like this, so it
could create problems for -portable.

Easy fix is to strdup(optarg) here.

Or we could strip the slash at a lower layer, in got_worktree_init().
This might be better anyway, just in case we start calling got_worktree_init()
from tools other than got checkout. Replace absprefix with a normalized-path
version of the prefix argument and avoid using the prefix argument as-is
in all cases.

>  			path_prefix = optarg;
>  			break;
>  		case 'q':
> commit - e3df3fa2b325547b4c69fcb62527422eb7d5f108
> blob - 2f2919d37662afe63851b5b51815d5022a69051e
> file + lib/worktree.c
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -6088,7 +6088,7 @@ insert_tree_entry(struct got_tree_entry *new_te,
>  	if (err)
>  		return err;
>  	if (new_pe == NULL)
> -		return got_error(GOT_ERR_TREE_DUP_ENTRY);
> +		return got_error_path(new_te->name, GOT_ERR_TREE_DUP_ENTRY);
>  	return NULL;
>  }
>  
> @@ -6845,7 +6845,6 @@ got_worktree_commit(struct got_object_id **new_commit_
>  		    head_commit_id, repo, GOT_ERR_COMMIT_OUT_OF_DATE);
>  		if (err)
>  			goto done;
> -
>  	}
>  
>  	err = commit_worktree(new_commit_id, &commitable_paths,
> commit - e3df3fa2b325547b4c69fcb62527422eb7d5f108
> blob - 997d20f28754d7e0afc5af0f4375e2f0d19ed2d2
> file + lib/worktree_open.c
> --- lib/worktree_open.c
> +++ lib/worktree_open.c
> @@ -195,6 +195,8 @@ open_worktree(struct got_worktree **worktree, const ch
>  	    GOT_WORKTREE_PATH_PREFIX);
>  	if (err)
>  		goto done;
> +	if (strcmp((*worktree)->path_prefix, "/") != 0)

Use got_path_is_root_dir() instead of manual strcmp()?

> +		got_path_strip_trailing_slashes((*worktree)->path_prefix);

Good. We will need this tweak in any case, to avoid the problem in
existing work trees which carry a trailing slash.

>  	err = read_meta_file(&base_commit_id_str, path_meta,
>  	    GOT_WORKTREE_BASE_COMMIT);
> commit - e3df3fa2b325547b4c69fcb62527422eb7d5f108
> blob - c8651ab26909a5ce8ff104fcc69b1029cc18605e
> file + regress/cmdline/commit.sh
> --- regress/cmdline/commit.sh
> +++ regress/cmdline/commit.sh
> @@ -107,6 +107,46 @@ test_commit_subdir() {
>  	test_done "$testroot" "$ret"
>  }
>  
> +test_commit_subdirs() {
> +	local testroot=`test_init commit_subdir`
> +

Maybe add a comment here which points out that the trailing slash on
epsilon was added on purpose to trigger a bug which used to exist in
got add (or is it in got commit)?

> +	got checkout -p epsilon/ $testroot/repo $testroot/wt > /dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	mkdir -p $testroot/wt/foo/bar
> +	mkdir -p $testroot/wt/foo/baz
> +
> +	echo omega > $testroot/wt/foo/bar/omega
> +	echo kappa > $testroot/wt/foo/bar/kappa
> +
> +	(cd $testroot/wt && got add -R foo >/dev/null)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got add failed unexpectedly" >&2
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi

Could verify the expected output of 'got add' here for completeness.

> +
> +	(cd $testroot/wt && \
> +		got commit -m 'test commit_subdirs' > $testroot/stdout)
> +
> +	local head_rev=`git_show_head $testroot/repo`
> +	echo "A  foo/bar/kappa" >> $testroot/stdout.expected
> +	echo "A  foo/bar/omega" >> $testroot/stdout.expected
> +	echo "Created commit $head_rev" >> $testroot/stdout.expected
> +
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
>  test_commit_single_file() {
>  	local testroot=`test_init commit_single_file`
>  
> @@ -2117,6 +2157,7 @@ test_parseargs "$@"
>  run_test test_commit_basic
>  run_test test_commit_new_subdir
>  run_test test_commit_subdir
> +run_test test_commit_subdirs
>  run_test test_commit_single_file
>  run_test test_commit_out_of_date
>  run_test test_commit_added_subdirs
>