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

On Tue, Sep 14, 2021 at 12:34 PM Junio C Hamano [off-list ref] wrote:

"Neeraj Singh via GitGitGadget" [off-list ref] writes:

quoted

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.

quoted

+#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.