Re: [PATCH v2 6/6] odb/source-files: only reload alternates when the file has changed

[Patrick Steinhardt <ps@pks.im>]

On Tue, Oct 27, 2026 at 12:22:50PM +0100, Christian Couder wrote:
> On Fri, Oct 23, 2026 at 2:48 PM Patrick Steinhardt <ps@pks.im> wrote:
> 
> > -static int read_alternates(const char *object_dir, struct strvec *out)
> > +static void read_alternates(const char *object_dir, struct strvec *out,
> > +                           struct stat_validity *validity)
> >  {
> >         struct strbuf buf = STRBUF_INIT;
> >         char *path;
> >
> >         path = xstrfmt("%s/info/alternates", object_dir);
> > -       if (strbuf_read_file(&buf, path, 1024) < 0) {
> > -               warn_on_fopen_errors(path);
> > -               free(path);
> > -               return 0;
> > +       if (!validity || !stat_validity_check(validity, path)) {
> > +               bool successful = false;
> > +               int fd;
> > +
> > +               fd = open(path, O_RDONLY);
> > +               if (fd < 0) {
> > +                       warn_on_fopen_errors(path);
> > +               } else if (strbuf_read(&buf, fd, 1024) < 0) {
> 
> Here we start reading from the alternates file, but that file could
> start to be written to...
> 
> > +                       warning_errno(_("unable to read alternates file '%s'"),
> > +                                     path);
> > +               } else {
> > +                       parse_alternates(buf.buf, '\n', object_dir, out);
> 
> ...suppose we parse the old content of the file (which calls
> strbuf_realpath() for each entry and so does a few syscalls) while an
> in-place rewrite of the file, like the:
> 
>   `echo "$PWD/alternate/.git/objects" >repo/.git/objects/info/alternates`
> 
> in the new t1006 test added by patch 3/6, happens...
> 
> > +                       successful = true;
> > +               }
> > +
> > +               if (validity) {
> > +                       if (successful)
> > +                               stat_validity_update(validity, fd);
> 
> ...then we would update the validity while the alternates file's
> content has changed (and we only parsed its old content).
> 
> So, if the file content is not changed after that, we never parse the
> content of the file from the in-place rewrite.
> 
> (If the rewrite is not an in-place one, but the file is for example
> replaced through a lock file and rename, the race doesn't happen.)
> 
> It seems to me that if we want to avoid a race here, we should have
> the `stat_validity_update(..., fd);` call before reading from fd like
> in "refs/packed-backend.c" and "shallow.c".
> 
> > +                       else
> > +                               stat_validity_clear(validity);
> > +               }
> > +
> > +               if (fd >= 0)
> > +                       close(fd);
> 
> It looks like v1 didn't have this issue, as it called
> stat_validity_update() before reading. A concurrent rewrite then only
> caused a spurious re-read on the next check. So I wonder if we could
> keep that order, and just clear the validity when reading fails, which
> would also address Junio's point:
> 
>                fd = open(path, O_RDONLY);
>                if (fd < 0) {
>                        warn_on_fopen_errors(path);
>                } else {
>                        if (validity)
>                                stat_validity_update(validity, fd);
>                        if (strbuf_read(&buf, fd, 1024) < 0) {
>                                warning_errno(_("unable to read
> alternates file '%s'"),
>                                                        path);
>                                if (validity)
>                                        stat_validity_clear(validity);
>                        } else {
>                                parse_alternates(buf.buf, '\n', object_dir, out);
>                        }
>                        close(fd);
>                }

Hmm, that's fair. I don't think that we should worry too much about
in-place updates when concurrent processes are active as this is
inherently broken already. That process may see any in-between state,
and that may or may not result in something sensible. But fixing this is
easy enough.

Your proposed variant is subtly broken because you don't clear the stat
cache in the case where open(3p) itself fails. But I'll adapt that a bit
and then include this in the next version.

Thanks!

Patrick