Re: [PATCH 6/8] core.fsync: add `fsync_component()` wrapper which doesn't die

[Neeraj Singh <nksingh85@gmail.com>]

On Thu, Mar 10, 2022 at 9:34 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > We have a `fsync_component_or_die()` helper function which only syncs
> > changes to disk in case the corresponding config is enabled by the user.
> > This wrapper will always die on an error though, which makes it
> > insufficient for new callsites we are about to add.
>
> You can replace "which makes it ..." part with a bit more concrete
> description to save suspense from the readers.
>
>     fsync_component_or_die() that dies upon an error is not useful
>     for callers with their own error handling or recovery logic,
>     like ref transaction API.
>
>     Split fsync_component() out that returns an error to help them.
>
> > -void fsync_or_die(int fd, const char *);
> > +int maybe_fsync(int fd);
> > ...
> > +static inline int fsync_component(enum fsync_component component, int fd)
> > +{
> > +     if (fsync_components & component)
> > +             return maybe_fsync(fd);
> > +     return 0;
> > +}
> >
> >  static inline void fsync_component_or_die(enum fsync_component component, int fd, const char *msg)
> >  {
> > -     if (fsync_components & component)
> > -             fsync_or_die(fd, msg);
> > +     if (fsync_component(component, fd) < 0)
> > +             die_errno("fsync error on '%s'", msg);
> >  }
>
> I think in the eventuall reroll, these "static inline" functions on
> the I/O code path will become real functions in write-or-die.c but
> other than that this reorganization looks sensible.
>
> Thanks.
>

Yes, that will be part of v6 of the base changeset.

> > diff --git a/write-or-die.c b/write-or-die.c
> > index 9faa5f9f56..4a5455ce46 100644
> > --- a/write-or-die.c
> > +++ b/write-or-die.c
> > @@ -56,19 +56,21 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)
> >       }
> >  }
> >
> > -void fsync_or_die(int fd, const char *msg)
> > +int maybe_fsync(int fd)
> >  {
> >       if (use_fsync < 0)
> >               use_fsync = git_env_bool("GIT_TEST_FSYNC", 1);
> >       if (!use_fsync)
> > -             return;
> > +             return 0;
> >
> >       if (fsync_method == FSYNC_METHOD_WRITEOUT_ONLY &&
> >           git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0)
> > -             return;
> > +             return 0;
> >
> >       if (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0)
> > -             die_errno("fsync error on '%s'", msg);
> > +             return -1;
> > +
> > +     return 0;
> >  }
> >
> >  void write_or_die(int fd, const void *buf, size_t count)