On Tue, Dec 7, 2021 at 4:27 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Tue, Dec 07 2021, Neeraj Singh via GitGitGadget wrote:

quoted

From: Neeraj Singh <redacted>

This commit introduces the `core.fsyncmethod` configuration

Just a commit msg nit: core.fsyncMethod (I see the docs etc. are using
it camelCased, good..

Will fix.

quoted

diff --git a/compat/win32/flush.c b/compat/win32/flush.c
new file mode 100644
index 00000000000..75324c24ee7
--- /dev/null
+++ b/compat/win32/flush.c

@@ -0,0 +1,28 @@
+#include "../../git-compat-util.h"

nit: Just FWIW I think the better thing is '#include
"git-compat-util.h"', i.e. we're compiling at the top-level and have
added it to -I.

(I know a lot of compat/ and contrib/ and even main-tree stuff does
that, but just FWIW it's not needed).

Will fix.

quoted

+     if (!strcmp(var, "core.fsyncmethod")) {
+             if (!value)
+                     return config_error_nonbool(var);
+             if (!strcmp(value, "fsync"))
+                     fsync_method = FSYNC_METHOD_FSYNC;
+             else if (!strcmp(value, "writeout-only"))
+                     fsync_method = FSYNC_METHOD_WRITEOUT_ONLY;
+             else

As a non-nit comment I think this config schema looks great so far.

quoted

+                     warning(_("unknown %s value '%s'"), var, value);

Just a suggestion maybe something slightly scarier like:

    "unknown core.fsyncMethod value '%s'; config from future git version? ignoring requested fsync strategy"

Also using the nicer camelCased version instead of "var" (also helps
translators with context...)

Will fix.  The motivation for this scheme was to 'factor' the messages
so there would be less to translate. But I see now that the message
doesn't have enough context to translate reasonably.

quoted

+int git_fsync(int fd, enum fsync_action action)
+{
+     switch (action) {
+     case FSYNC_WRITEOUT_ONLY:
+
+#ifdef __APPLE__
+             /*
+              * on macOS, fsync just causes filesystem cache writeback but does not
+              * flush hardware caches.
+              */
+             return fsync(fd);
+#endif
+
+#ifdef HAVE_SYNC_FILE_RANGE
+             /*
+              * On linux 2.6.17 and above, sync_file_range is the way to issue
+              * a writeback without a hardware flush. An offset of 0 and size of 0
+              * indicates writeout of the entire file and the wait flags ensure that all
+              * dirty data is written to the disk (potentially in a disk-side cache)
+              * before we continue.
+              */
+
+             return sync_file_range(fd, 0, 0, SYNC_FILE_RANGE_WAIT_BEFORE |
+                                              SYNC_FILE_RANGE_WRITE |
+                                              SYNC_FILE_RANGE_WAIT_AFTER);
+#endif
+
+#ifdef fsync_no_flush
+             return fsync_no_flush(fd);
+#endif
+
+             errno = ENOSYS;
+             return -1;
+
+     case FSYNC_HARDWARE_FLUSH:
+             /*
+              * On some platforms fsync may return EINTR. Try again in this
+              * case, since callers asking for a hardware flush may die if
+              * this function returns an error.
+              */
+             for (;;) {
+                     int err;
+#ifdef __APPLE__
+                     err = fcntl(fd, F_FULLFSYNC);
+#else
+                     err = fsync(fd);
+#endif
+                     if (err >= 0 || errno != EINTR)
+                             return err;
+             }
+
+     default:
+             BUG("unexpected git_fsync(%d) call", action);

Don't include such "default" cases, you have an exhaustive "enum", if
you skip it the compiler will check this for you.

Junio gave the feedback to include this "default:" case in the switch
[1].  Removing the default leads to the "error: control reaches end of
non-void function" on gcc. Fixing that error and adding a trial option
does give the exhaustiveness error that you're talking about.  I'd
rather just leave this as-is since the BUG() obviates the need for an
in-practice-unreachable return statement.

[1] https://lore.kernel.org/git/xmqqfsu70x58.fsf@gitster.g/ (local)

quoted

+     }
+}
+
 static int warn_if_unremovable(const char *op, const char *file, int rc)

Just a code nit: I think it's very much preferred if possible to have as
much of code like this compile on all platforms. See the series at
4002e87cb25 (grep: remove #ifdef NO_PTHREADS, 2018-11-03) is part of for
a good example.

Maybe not worth it in this case since they're not nested ifdef's.

I'm basically thinking of something (also re Patrick's comment on the
2nd patch) where we have a platform_fsync() whose return
value/arguments/whatever capture this "I want to return now" or "you
should be looping" and takes the enum_fsync_action" strategy.

Then the git_fsync() would be the platform-independent looping etc., and
another funciton would do the "one fsync at a time, maybe call me
again".

Maybe it would suck more, just food for thought... :)

I'm going to introduce a new static function called fsync_loop which
does the looping and I'll call it from git_fsync.  That appears to be
the cleanest to me to address your and Patrick's feedback.

Thanks,
Neeraj