Stefan Sperling <stsp@stsp.name> wrote:
> 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?

I'm fixing this as well, it helps =)

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

agreed, mine was really a quick'n'dirty fix.  in the updated diff below
I'm fixing the paths in the library code, instead that in got.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()?

done

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

yep!

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

done as well

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

sure

> > +
> > +	(cd $testroot/wt && \
> > +		got commit -m 'test commit_subdirs' > $testroot/stdout)
> > +
> > [...]


diff /home/op/w/got
path + /home/op/w/got
commit - f90dbd6e6d9fa970e460f89a909e1b48f79c5692
blob - 83b04b102ee7c7794010dc1e7a5e1aee931f63e9
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -180,7 +180,12 @@ got_worktree_init(const char *path, struct got_referen
 	if (!got_path_is_absolute(prefix)) {
 		if (asprintf(&absprefix, "/%s", prefix) == -1)
 			return got_error_from_errno("asprintf");
+	} else {
+		absprefix = strdup(prefix);
+		if (absprefix == NULL)
+			return got_error_from_errno("strdup");
 	}
+	got_path_strip_trailing_slashes(absprefix);
 
 	/* Create top-level directory (may already exist). */
 	if (mkdir(path, GOT_DEFAULT_DIR_MODE) == -1 && errno != EEXIST) {
@@ -229,8 +234,7 @@ got_worktree_init(const char *path, struct got_referen
 		goto done;
 
 	/* Store in-repository path prefix. */
-	err = create_meta_file(path_got, GOT_WORKTREE_PATH_PREFIX,
-	    absprefix ? absprefix : prefix);
+	err = create_meta_file(path_got, GOT_WORKTREE_PATH_PREFIX, absprefix);
 	if (err)
 		goto done;
 
@@ -273,13 +277,21 @@ got_worktree_match_path_prefix(int *match, struct got_
     const char *path_prefix)
 {
 	char *absprefix = NULL;
+	const char *a, *b;
+	size_t al, bl;
 
 	if (!got_path_is_absolute(path_prefix)) {
 		if (asprintf(&absprefix, "/%s", path_prefix) == -1)
 			return got_error_from_errno("asprintf");
 	}
-	*match = (strcmp(absprefix ? absprefix : path_prefix,
-	    worktree->path_prefix) == 0);
+
+	a = absprefix ? absprefix : path_prefix;
+	al = strlen(a);
+
+	b = worktree->path_prefix;
+	bl = strlen(b);
+
+	*match = (got_path_cmp(a, b, al, bl) == 0);
 	free(absprefix);
 	return NULL;
 }
@@ -6111,7 +6123,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;
 }
 
@@ -6868,7 +6880,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 - f90dbd6e6d9fa970e460f89a909e1b48f79c5692
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 (got_path_is_root_dir((*worktree)->path_prefix))
+		got_path_strip_trailing_slashes((*worktree)->path_prefix);
 
 	err = read_meta_file(&base_commit_id_str, path_meta,
 	    GOT_WORKTREE_BASE_COMMIT);
commit - f90dbd6e6d9fa970e460f89a909e1b48f79c5692
blob - c8651ab26909a5ce8ff104fcc69b1029cc18605e
file + regress/cmdline/commit.sh
--- regress/cmdline/commit.sh
+++ regress/cmdline/commit.sh
@@ -107,6 +107,59 @@ test_commit_subdir() {
 	test_done "$testroot" "$ret"
 }
 
+test_commit_subdirs() {
+	local testroot=`test_init commit_subdir`
+
+	# trailing slash for -p on purpose, we used to have a bug
+	# that was triggered by the presence of trailing slashes.
+	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 >$testroot/stdout)
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got add failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "A  foo/bar/kappa" > $testroot/stdout.expected
+	echo "A  foo/bar/omega" >>$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
+
+	(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 +2170,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