On Thu, May 5, 2022 at 1:32 PM Goss Geppert <gg.oss.dev@xxxxxxxxx> wrote:
>
> 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.
> 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.
> 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?
> +
> + 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.
> 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.
> }
>
> if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES)) {
> --
> 2.36.0
Thanks 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?
