Re: [PATCH v3 4/8] t7300: add testcase showing unnecessary traversal into ignored directory

From: Elijah Newren <newren@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Derrick Stolee <stolee@gmail.com>, Jeff King <peff@peff.net>,
	Philip Oakley <philipoakley@iee.email>,
	Jeff Hostetler <jeffhost@microsoft.com>,
	Josh Steadmon <steadmon@google.com>
Subject: Re: [PATCH v3 4/8] t7300: add testcase showing unnecessary traversal into ignored directory
Date: Tue, 11 May 2021 19:07:40 -0700	[thread overview]
Message-ID: <CABPp-BEJQ5RQdvm58xrNcWm9VdPrUF8_-bym-M424bs7xR=8Gg@mail.gmail.com> (raw)
In-Reply-To: <xmqqsg2sj3id.fsf@gitster.g>

On Tue, May 11, 2021 at 3:43 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > But to answer your question, the paths we visit are '.', '..', '.git',
> > and 'untracked', the first three of which we mark as path_none and
> > don't recurse into because of special rules for those paths, and the
> > last of which we shouldn't recurse into since it is ignored.
>
> Not a hard requirement, but I wish if we entirely ignored "." and
> ".." in our code (not just not counting, but making whoever calls
> readdir() skip and call it again when it gets "." or "..").
>
>   https://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html
>
> seems to imply that readdir() may not give "." or ".." (if dot or
> dot-dot exists, you are to return them only once, which implies that
> it is perfectly OK for dot or dot-dot to be missing).


Something like this?

diff --git a/dir.c b/dir.c
index 993a12145f..7f470bc701 100644
--- a/dir.c
+++ b/dir.c
@@ -2341,7 +2341,11 @@ static int read_cached_dir(struct cached_dir *cdir)
        struct dirent *de;

        if (cdir->fdir) {
-               de = readdir(cdir->fdir);
+               while ((de = readdir(cdir->fdir))) {
+                       /* Ignore '.' and '..' by re-looping; handle the rest */
+                       if (!de || !is_dot_or_dotdot(de->d_name))
+                               break;
+               }
                if (!de) {
                        cdir->d_name = NULL;
                        cdir->d_type = DT_UNKNOWN;

It appears that the other two callers of readdir() in dir.c, namely in
is_empty_dir() and remove_dir_recurse() already have such special
repeat-if-is_dot_or_dotdot() logic built into them, so this was
partially lifted from those.

If you'd like, I can add another patch in the series with this change
so that all readdir() calls in dir.c have such ignore '.' and '..'
logic.  Or, we could perhaps introduce a new readdir() wrapper that
does nothing other than ignore '.' and '..' and have all three of
these callsites use that new wrapper.

> So dropping the test for number of visited paths would be nicer from
> portability's point of view ;-)

Yep, makes sense.  I already did that in v4, which means it'll
continue to pass with or without the above proposed change to
read_cached_dir().
