From: "Omar Polo" Subject: Re: fix duplicated tree item with prefix checkout To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Sat, 18 Jul 2026 18:38:39 +0200 Stefan Sperling 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