| Message ID | 20220505203234.21586-2-ggossdev@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dir: consider worktree config in path recursion | expand |
On Thu, May 5, 2022 at 1:32 PM Goss Geppert <gg.oss.dev@gmail.com> 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?
On Fri, May 6, 2022 at 11:26 PM Elijah Newren <newren@gmail.com> wrote: > 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? I used `real_pathdup` here because `the_repository->gitdir` is not necessarily normalized as required for the `strcmp`. For example, when using option `--git-dir=./././xyz/.git` while outside the worktree, the path will remain as is (see [1] and [2]). [1]: https://github.com/git/git/blob/e8005e4871f130c4e402ddca2032c111252f070a/setup.c#L962 [2]: https://github.com/git/git/blob/e8005e4871f130c4e402ddca2032c111252f070a/environment.c#L353 > 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? WIll do, thx.
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; 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 + */ + int nested_repo; 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); + + nested_repo = !!strcmp(real_dirname, real_gitdir); + free(real_gitdir); + free(real_dirname); + } 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; + } } if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES)) {
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(-)