git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Goss Geppert <gg.oss.dev@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	christian w <usebees@gmail.com>,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [RFC PATCH 1/1] dir: consider worktree config in path recursion
Date: Fri, 6 May 2022 20:26:15 -0700	[thread overview]
Message-ID: <CABPp-BFwiLXMVaFs-Lc-zU6X=pBiF8+o9rOuHEAArD2zMQ1NNQ@mail.gmail.com> (raw)
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:
>
> 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?

  reply	other threads:[~2022-05-07  3:27 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-05 20:32 [RFC PATCH 0/1] " Goss Geppert
2022-05-05 20:32 ` [RFC PATCH 1/1] " Goss Geppert
2022-05-07  3:26   ` Elijah Newren [this message]
2022-05-07 17:59     ` oss dev
2022-05-06 17:02 ` [RFC PATCH 0/1] " Junio C Hamano
2022-05-06 20:00   ` oss dev
2022-05-10 17:15 ` [PATCH v2 0/2] " Goss Geppert
2022-05-10 17:15   ` [PATCH v2 1/2] " Goss Geppert
2022-05-11 16:37     ` Junio C Hamano
2022-05-20 19:45       ` oss dev
2022-05-24 14:29       ` Elijah Newren
2022-05-24 19:45         ` Junio C Hamano
2022-05-25  3:46           ` Elijah Newren
2022-05-11 23:07     ` Junio C Hamano
2022-05-20 20:01       ` oss dev
2022-05-23 19:23     ` Derrick Stolee
2022-05-30 18:48       ` oss dev
2022-05-10 17:15   ` [PATCH v2 2/2] dir: minor refactoring / clean-up Goss Geppert
2022-05-11 16:51     ` Junio C Hamano
2022-05-20 20:03       ` oss dev
2022-05-20 19:28 ` [PATCH v3 0/3] dir: traverse into repository Goss Geppert
2022-05-20 19:28   ` [PATCH v3 1/3] " Goss Geppert
2022-05-20 19:28   ` [PATCH v3 2/3] dir: cache git_dir's realpath Goss Geppert
2022-05-24 14:32     ` Elijah Newren
2022-05-20 19:28   ` [PATCH v3 3/3] dir: minor refactoring / clean-up Goss Geppert
2022-06-16 23:19 ` [PATCH v4 0/2] dir: traverse into repository Goss Geppert
2022-06-22  4:57   ` Elijah Newren
     [not found] ` <20220616231956.154-1-gg.oss@outlook.com>
2022-06-16 23:19   ` [PATCH v4 1/2] " Goss Geppert
2022-06-16 23:44 ` [PATCH v4 0/2] dir: traverse into repository (resending) Goss Geppert
     [not found] ` <20220616234433.225-1-gg.oss@outlook.com>
2022-06-16 23:44   ` [PATCH v4 1/2] dir: traverse into repository Goss Geppert
2022-06-16 23:44   ` [PATCH v4 2/2] dir: minor refactoring / clean-up Goss Geppert

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABPp-BFwiLXMVaFs-Lc-zU6X=pBiF8+o9rOuHEAArD2zMQ1NNQ@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=gg.oss.dev@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=usebees@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).