On Tue, Sep 14, 2021 at 12:34 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Neeraj Singh via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
