Re: [PATCH v6 4/8] core.fsyncobjectfiles: add windows support for batch mode

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

...

This almost makes me wonder if we want to have a fallback
implementation of fsync_no_flush() that does

   int fsync_no_flush(int unused)
   {
	errno = ENOSYS;
	return -1;
   }

when nobody (like Windows) define their own fsync_no_flush().  That
way, this codepath does not have to have #ifdef/#endif here.

This function is already #ifdef ridden anyway, so reducing just one
instance may not make much difference, but since I noticed it ...

Thanks.

On Mon, Sep 27, 2021 at 1:07 PM Junio C Hamano [off-list ref] wrote:

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

quoted

diff --git a/compat/mingw.h b/compat/mingw.h
index c9a52ad64a6..6074a3d3ced 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h

@@ -329,6 +329,9 @@ int mingw_getpagesize(void);
 #define getpagesize mingw_getpagesize
 #endif

+int win32_fsync_no_flush(int fd);
+#define fsync_no_flush win32_fsync_no_flush

...

quoted

diff --git a/wrapper.c b/wrapper.c
index bb4f9f043ce..1a1e2fba9c9 100644
--- a/wrapper.c
+++ b/wrapper.c

@@ -567,6 +567,10 @@ int git_fsync(int fd, enum fsync_action action)
                                               SYNC_FILE_RANGE_WAIT_AFTER);
 #endif

+#ifdef fsync_no_flush
+             return fsync_no_flush(fd);
+#endif
+
              errno = ENOSYS;
              return -1;

This almost makes me wonder if we want to have a fallback
implementation of fsync_no_flush() that does

   int fsync_no_flush(int unused)
   {
        errno = ENOSYS;
        return -1;
   }

when nobody (like Windows) define their own fsync_no_flush().  That
way, this codepath does not have to have #ifdef/#endif here.

This function is already #ifdef ridden anyway, so reducing just one
instance may not make much difference, but since I noticed it ...

Thanks.

I'll make your suggested change on Github so that it will be available if
we do another re-roll.

Thanks,
Nereaj

On Mon, Sep 27, 2021 at 1:55 PM Neeraj Singh [off-list ref] wrote:

On Mon, Sep 27, 2021 at 1:07 PM Junio C Hamano [off-list ref] wrote:

quoted

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

quoted

diff --git a/compat/mingw.h b/compat/mingw.h
index c9a52ad64a6..6074a3d3ced 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h

@@ -329,6 +329,9 @@ int mingw_getpagesize(void);
 #define getpagesize mingw_getpagesize
 #endif

+int win32_fsync_no_flush(int fd);
+#define fsync_no_flush win32_fsync_no_flush

...

quoted

diff --git a/wrapper.c b/wrapper.c
index bb4f9f043ce..1a1e2fba9c9 100644
--- a/wrapper.c
+++ b/wrapper.c

@@ -567,6 +567,10 @@ int git_fsync(int fd, enum fsync_action action)
                                               SYNC_FILE_RANGE_WAIT_AFTER);
 #endif

+#ifdef fsync_no_flush
+             return fsync_no_flush(fd);
+#endif
+
              errno = ENOSYS;
              return -1;

This almost makes me wonder if we want to have a fallback
implementation of fsync_no_flush() that does

   int fsync_no_flush(int unused)
   {
        errno = ENOSYS;
        return -1;
   }

when nobody (like Windows) define their own fsync_no_flush().  That
way, this codepath does not have to have #ifdef/#endif here.

This function is already #ifdef ridden anyway, so reducing just one
instance may not make much difference, but since I noticed it ...

Thanks.

I'll make your suggested change on Github so that it will be available if
we do another re-roll.

Thanks,
Nereaj

Actually, while trying your suggestion, my conclusion is that we'd
either have the
inverse ifdef around the fsync_no_flush fallback or an #undef, or some
other confusing
state.  The current ifdeffery is unpleasant to read but not too long
and also pretty direct.
Win32 has an extra level of indirection, but the unix platforms
syscalls are directly written
in one place.

Thanks,
Neeraj