Re: [RFC PATCH 1/1] dir: consider worktree config in path recursion
- From
Elijah Newren <newren@gmail.com>
- Date
- May 7, 2022, 03:26 UTC
- Message-ID
- <CABPp-BFwiLXMVaFs-Lc-zU6X=pBiF8+o9rOuHEAArD2zMQ1NNQ@mail.gmail.com>
- In-Reply-To
- <20220505203234.21586-2-ggossdev@gmail.com>
On Thu, May 5, 2022 at 1:32 PM Goss Geppert <gg.oss.dev@gmail.com> wrote:
Show 29 quoted lines
> > Since 8d92fb2927 (dir: replace exponential algorithm with a linear one, > 2020-04-01) the following no longer works: > > 1) Initialize a repository. > 2) Set the `core.worktree` location to a parent directory of the > default worktree. > 3) Add a file located in the default worktree location to the index > (i.e. anywhere in the immediate parent directory of the gitdir). > > This commit adds a check to determine whether a nested repository that > is encountered in recursing a path is actually `the_repository`. If so, > simply treat the directory as if it doesn't contain a nested repository. > > Prior to this commit, the `add` operation was silently ignored. > --- > dir.c | 37 ++++++++++++++++++++++++++++++------- > 1 file changed, 30 insertions(+), 7 deletions(-) > > diff --git a/dir.c b/dir.c > index f2b0f24210..cef39f43d8 100644 > --- a/dir.c > +++ b/dir.c > @@ -1861,7 +1861,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir, > */ > enum path_treatment state; > int matches_how = 0; > - int nested_repo = 0, check_only, stop_early; > + int check_only, stop_early;
This part of the patch, along with two other parts below, is orthogonal to the actual fix being made. It probably makes the code clearer, but should be done in an independent cleanup commit.
Show 14 quoted lines
> int old_ignored_nr, old_untracked_nr;
> /* The "len-1" is to strip the final '/' */
> enum exist_status status = directory_exists_in_index(istate, dirname, len-1);
> @@ -1893,16 +1893,39 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
>
> if ((dir->flags & DIR_SKIP_NESTED_GIT) ||
> !(dir->flags & DIR_NO_GITLINKS)) {
> + /*
> + * Determine if `dirname` is a nested repo by confirming that:
> + * 1) we are in a nonbare repository, and
> + * 2) `dirname` is not an immediate parent of `the_repository->gitdir`,
> + * which could occur if the `worktree` location was manually
> + * configured by the user
> + */This is a good comment, but it'd be really nice to be able to point a user at a testcase in the testsuite for them to inspect further (e.g. "see t####, 'TESTCASE DESCRIPTION' for an case where this matters").
> + int nested_repo;
Just this line immediately above this comment is part of the orthogonal cleanup.
Show 10 quoted lines
> struct strbuf sb = STRBUF_INIT;
> strbuf_addstr(&sb, dirname);
> nested_repo = is_nonbare_repository_dir(&sb);
> +
> + if (nested_repo) {
> + char *real_dirname, *real_gitdir;
> + strbuf_reset(&sb);
> + strbuf_addstr(&sb, dirname);
> + strbuf_complete(&sb, '/');
> + strbuf_addstr(&sb, ".git");> + real_dirname = real_pathdup(sb.buf, 0); > + real_gitdir = real_pathdup(the_repository->gitdir, 0);
I haven't thought it through, but is there a reason you can't just compare the_repository->gitdir to sb.buf at this point? Why is real_pathdup needed?
Show 5 quoted lines
> + > + nested_repo = !!strcmp(real_dirname, real_gitdir); > + free(real_gitdir); > + free(real_dirname); > + }
Everything up to here other than the two parts I mentioned as being orthogonal (both relating to where "nested_repo" is defined), makes up the actual fix.
Show 14 quoted lines
> strbuf_release(&sb);
> - }
> - if (nested_repo) {
> - if ((dir->flags & DIR_SKIP_NESTED_GIT) ||
> - (matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))
> - return path_none;
> - return excluded ? path_excluded : path_untracked;
> +
> + if (nested_repo) {
> + if ((dir->flags & DIR_SKIP_NESTED_GIT) ||
> + (matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))
> + return path_none;
> + return excluded ? path_excluded : path_untracked;
> + }This final block is part of the orthogonal code cleanup that belongs in a separate commit.
Show 5 quoted lines
> }
>
> if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES)) {
> --
> 2.36.0Thanks for finding, reporting, and sending in a patch. Could you split up the patch as indicated above, and add a testcase to the patch with the fix, and include your Signed-off-by trailer on the commits?