Re: [PATCH v3 2/6] core.fsyncobjectfiles: batched disk flushes

[Neeraj Singh <nksingh85@gmail.com>]

On Tue, Sep 14, 2021 at 12:34 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > diff --git a/config.c b/config.c
> > index cb4a8058bff..9fe3602e1c4 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -1509,7 +1509,13 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
> >       }
> >
> >       if (!strcmp(var, "core.fsyncobjectfiles")) {
> > -             fsync_object_files = git_config_bool(var, value);
> > +             if (!value)
> > +                     return config_error_nonbool(var);
> > +             if (!strcasecmp(value, "batch"))
> > +                     fsync_object_files = FSYNC_OBJECT_FILES_BATCH;
> > +             else
> > +                     fsync_object_files = git_config_bool(var, value)
> > +                             ? FSYNC_OBJECT_FILES_ON : FSYNC_OBJECT_FILES_OFF;
> >               return 0;
>
> The original code used to allow the short-and-sweet valueless true
>
>         [core]
>                 fsyncobjectfiles
>
> but it no longer does by calling it a nonbool error.  This breaks
> existing users' repositories that have been happily working, doesn't
> it?
>
> Perhaps
>
>         if (value && !strcmp(value, "batch"))
>                 fsync_object_files = FSYNC_OBJECT_FILES_BATCH;
>         else if (git_config_bool(var, value))
>                 fsync_object_files = FSYNC_OBJECT_FILES_ON;
>         else
>                 fsync_object_files = FSYNC_OBJECT_FILES_OFF;

I'll take your suggestion, including the change to case-sensitive.

> > +#ifdef __APPLE__
> > +     return fcntl(fd, F_FULLFSYNC);
> > +#else
> > +     return fsync(fd);
> > +#endif
> > +}
>
> If we are introducing "enum fsync_action", we should have some way
> to make it clear that we are covering all the possible values of
> "action".
>
> Switching on action, i.e.
>
>         switch (action) {
>         case FSYNC_WRITEOUT_ONLY:
>                 ...
>                 break;
>         case FSYNC_HARDWARE_FLUSH:
>                 ...
>                 break;
>         default:
>                 BUG("unexpected git_fsync(%d) call", action);
>         }
>
> would be one way to do so.
>

Will do.

Thanks for reviewing my changes. I've updated the github PR.
I'll wait for a few more days to see if anyone has more feedback
before sending out another round of patches.