From: Elijah Newren <newren@gmail.com>
Date: Thu, 17 Feb 2022 04:40:48 GMT
Subject: Re: [PATCH v2 5/5] Accelerate clear_skip_worktree_from_present_files() by caching
Message-ID: <CABPp-BHbpO7CUXApVcEuv-Q-+_+oCTh_gmRO9UOwQPYPsbAH+A@mail.gmail.com>
In-Reply-To: <CABPp-BEog_CBEjx3FBGdUAhjwrPPDuP54HWQssAWnGeUnr0cBg@mail.gmail.com>

On Wed, Feb 16, 2022 at 8:30 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Wed, Feb 16, 2022 at 1:37 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> >
> > On Fri, Jan 14 2022, Elijah Newren via GitGitGadget wrote:
> >
...
> > > +
> > > +     /* Free previous dirname, and cache path's dirname */
> > > +     *dirname = path;
> > > +     *dir_len = newdir - path + 1;
> > > +
> > > +     tmp = xstrndup(path, *dir_len);
> > > +     *dir_found = !lstat(tmp, &st);
> >
> > In most other places we're a bit more careful about lstat() error handling, e.g.:
> >
> >     builtin/init-db.c:              if (lstat(path->buf, &st_git)) {
> >     builtin/init-db.c-                      if (errno != ENOENT)
> >     builtin/init-db.c-                              die_errno(_("cannot stat '%s'"), path->buf);
> >     builtin/init-db.c-              }
> >
> > Shouldn't we do the same here and at least error() on return values of
> > -1 with an accompanying errno that isn't ENOENT?
>
> If we should do that everywhere, should we have an xlstat in wrapper.[ch]?

Turns out we already DO have a file_exists() wrapper function, but
it's found in dir.c rather than wrapper.[ch].  However, it doesn't
bother to check errno either...