A design for future-proofing fsync() configuration

From: Neeraj Singh <[email protected]>

compat/mingw.h +3 -0

@@ -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
+
 struct rlimit {
 	unsigned int rlim_cur;
 };

compat/win32/flush.c new file +8 -0

new file mode 100644
index 00000000000..75324c24ee7
@@ -0,0 +1,28 @@
+#include "../../git-compat-util.h"
+#include <winternl.h>
+#include "lazyload.h"
+
+int win32_fsync_no_flush(int fd)
+{
+       IO_STATUS_BLOCK io_status;
+

compat/win32/flush.c new file +9 -0

+       memset(&io_status, 0, sizeof(io_status));
+       if (NtFlushBuffersFileEx((HANDLE)_get_osfhandle(fd), FLUSH_FLAGS_FILE_DATA_ONLY,
+				NULL, 0, &io_status)) {
+		errno = EINVAL;
+		return -1;
+       }
+
+       return 0;
+}

wrapper.c +1 -0

index 36e12119d76..1c5f2c87791 100644
@@ -546,6 +546,62 @@ int xmkstemp_mode(char *filename_template, int mode)
 	return fd;
 }
 
+int git_fsync(int fd, enum fsync_action action)

wrapper.c +45 -0

+#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);
+	}
+}
+
 static int warn_if_unremovable(const char *op, const char *file, int rc)
 {
 	int err;

write-or-die.c +6 -4

index 0b1ec8190b6..0702acdd5e8 100644
@@ -57,10 +57,12 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)
 
 void fsync_or_die(int fd, const char *msg)
 {
-	while (fsync(fd) < 0) {
-		if (errno != EINTR)
-			die_errno("fsync error on '%s'", msg);
-	}
+	if (fsync_method == FSYNC_METHOD_WRITEOUT_ONLY &&
+	    git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0)
+		return;
+
+	if (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0)
+		die_errno("fsync error on '%s'", msg);
 }
 
 void write_or_die(int fd, const void *buf, size_t count)

Signature · 1 line

gitgitgadget

[[PGP Signed Part:Undecided]]

Quoted 1 line · Neeraj Singh via GitGitGadget

From: Neeraj Singh <[email protected]>

[...]
[snip]

Quoted 8 lines

wrapper.c +0 -0

index 36e12119d76..1c5f2c87791 100644
@@ -546,6 +546,62 @@ int xmkstemp_mode(char *filename_template, int mode)
 	return fd;
 }
 

wrapper.c int xmkstemp_mode(char *filename_template, int mode) · line 559

Quoted 11 lines

wrapper.c +11 -0

+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);

Below we're looping around EINTR -- are Apple systems never returning it?

Quoted 1 line · Neeraj Singh via GitGitGadget

From: Neeraj Singh <[email protected]>

[snip]

Quoted 33 lines

compat/mingw.h +3 -0

@@ -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
+
 struct rlimit {
      unsigned int rlim_cur;
 };

compat/win32/flush.c new file +15 -0

new file mode 100644
index 00000000000..75324c24ee7
@@ -0,0 +1,28 @@
+#include "../../git-compat-util.h"
+#include <winternl.h>
+#include "lazyload.h"
+
+int win32_fsync_no_flush(int fd)
+{
+       IO_STATUS_BLOCK io_status;
+
+#define FLUSH_FLAGS_FILE_DATA_ONLY 1
+
+       DECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NtFlushBuffersFileEx,
+                      HANDLE FileHandle, ULONG Flags, PVOID Parameters, ULONG ParameterSize,
+                      PIO_STATUS_BLOCK IoStatusBlock);
+
+       if (!INIT_PROC_ADDR(NtFlushBuffersFileEx)) {

compat/win32/flush.c line 18

Quoted 3 lines

compat/win32/flush.c new file +3 -0

+             errno = ENOSYS;
+             return -1;
+       }

I'm wondering whether it would make sense to fall back to fsync(3P) in case we cannot use writeout-only, but I see that were doing essentially that in fsync_or_die(). There is no indicator to the user though that writeout-only doesn't work -- do we want to print a one-time warning?

Quoted 9 lines

compat/win32/flush.c new file +9 -0

+       memset(&io_status, 0, sizeof(io_status));
+       if (NtFlushBuffersFileEx((HANDLE)_get_osfhandle(fd), FLUSH_FLAGS_FILE_DATA_ONLY,
+                             NULL, 0, &io_status)) {
+             errno = EINVAL;
+             return -1;
+       }
+
+       return 0;
+}

[snip]

Quoted 8 lines

wrapper.c +0 -0

index 36e12119d76..1c5f2c87791 100644
@@ -546,6 +546,62 @@ int xmkstemp_mode(char *filename_template, int mode)
      return fd;
 }
 

wrapper.c int xmkstemp_mode(char *filename_template, int mode) · line 559

Quoted 11 lines

wrapper.c +11 -0

+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);

Below we're looping around EINTR -- are Apple systems never returning it?

From: Neeraj Singh <[email protected]>

This commit introduces the core.fsyncmethod configuration

compat/win32/flush.c new file +43 -0

+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__

Quoted 3 lines · Neeraj Singh via GitGitGadget

From: Neeraj Singh <[email protected]>

This commit introduces the core.fsyncmethod configuration

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

compat/win32/flush.c line 1

Quoted 7 lines

compat/win32/flush.c new file +1 -0

new file mode 100644
index 00000000000..75324c24ee7
@@ -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).

compat/win32/flush.c line 9

Quoted 8 lines

compat/win32/flush.c new file +8 -0

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

compat/win32/flush.c line 10

Quoted 1 line

compat/win32/flush.c new file +1 -0

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

Quoted 48 lines

compat/win32/flush.c new file +48 -0

+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)

compat/win32/flush.c

Quoted 5 lines

compat/win32/flush.c new file +5 -0

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

compat/win32/flush.c

Quoted 4 lines

compat/win32/flush.c new file +3 -0

+     }
+}
+
 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... :)

From: Neeraj Singh <[email protected]>

builtin/index-pack.c +2 -2

index c23d01de7dc..c32534c13b4 100644
@@ -1286,7 +1286,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
 			    nr_objects - nr_objects_initial);
 		stop_progress_msg(&progress, msg.buf);
 		strbuf_release(&msg);
-		finalize_hashfile(f, tail_hash, 0);
+		finalize_hashfile(f, tail_hash, FSYNC_COMPONENT_PACK, 0);
 		hashcpy(read_hash, pack_hash);
 		fixup_pack_header_footer(output_fd, pack_hash,
 					 curr_pack, nr_objects,

@@ -1508,7 +1508,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 	if (!from_stdin) {
 		close(input_fd);
 	} else {
-		fsync_or_die(output_fd, curr_pack_name);
+		fsync_component_or_die(FSYNC_COMPONENT_PACK, output_fd, curr_pack_name);
 		err = close(output_fd);
 		if (err)
 			die_errno(_("error while closing pack file"));

builtin/pack-objects.c +0 -0

index 857be7826f3..916c55d6ce9 100644

config.c +74 -0

index c3410b8a868..29c867aab03 100644
@@ -1213,6 +1213,73 @@ static int git_parse_maybe_bool_text(const char *value)
 	return -1;
 }
 
+static const struct fsync_component_entry {
+	const char *name;
+	enum fsync_component component_bits;
+} fsync_component_table[] = {
+	{ "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
+	{ "pack", FSYNC_COMPONENT_PACK },
+	{ "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
+	{ "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
+	{ "objects", FSYNC_COMPONENTS_OBJECTS },
+	{ "default", FSYNC_COMPONENTS_DEFAULT },
+	{ "all", FSYNC_COMPONENTS_ALL },
+};
+
+static enum fsync_component parse_fsync_components(const char *var, const char *string)
+{
+	enum fsync_component output = 0;
+
+	if (!strcmp(string, "none"))
+		return output;
+
+	while (string) {
+		int i;
+		size_t len;
+		const char *ep;
+		int negated = 0;
+		int found = 0;
+
+		string = string + strspn(string, ", \t\n\r");
+		ep = strchrnul(string, ',');
+		len = ep - string;
+
+		if (*string == '-') {
+			negated = 1;
+			string++;
+			len--;
+			if (!len)
+				warning(_("invalid value for variable %s"), var);
+		}
+
+		if (!len)
+			break;
+
+		for (i = 0; i < ARRAY_SIZE(fsync_component_table); ++i) {
+			const struct fsync_component_entry *entry = &fsync_component_table[i];
+
+			if (strncmp(entry->name, string, len))
+				continue;
+
+			found = 1;
+			if (negated)
+				output &= ~entry->component_bits;
+			else
+				output |= entry->component_bits;
+		}
+
+		if (!found) {
+			char *component = xstrndup(string, len);
+			warning(_("unknown %s value '%s'"), var, component);
+			free(component);
+		}
+
+		string = ep;
+	}
+
+	return output;
+}
+
 int git_parse_maybe_bool(const char *value)
 {
 	int v = git_parse_maybe_bool_text(value);

@@ -1490,6 +1557,13 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.fsync")) {
+		if (!value)
+			return config_error_nonbool(var);
+		fsync_components = parse_fsync_components(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.fsyncmethod")) {
 		if (!value)
 			return config_error_nonbool(var);

csum-file.c +3 -2

index 26e8a6df44e..59ef3398ca2 100644
@@ -58,7 +58,8 @@ static void free_hashfile(struct hashfile *f)
 	free(f);
 }
 
-int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int flags)
+int finalize_hashfile(struct hashfile *f, unsigned char *result,
+		      enum fsync_component component, unsigned int flags)
 {
 	int fd;
 

@@ -69,7 +70,7 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int fl
 	if (flags & CSUM_HASH_IN_STREAM)
 		flush(f, f->buffer, the_hash_algo->rawsz);
 	if (flags & CSUM_FSYNC)
-		fsync_or_die(f->fd, f->name);
+		fsync_component_or_die(component, f->fd, f->name);
 	if (flags & CSUM_CLOSE) {
 		if (close(f->fd))
 			die_errno("%s: sha1 file error on close", f->name);

csum-file.h +2 -1

index 291215b34eb..0d29f528fbc 100644
@@ -1,6 +1,7 @@
 #ifndef CSUM_FILE_H
 #define CSUM_FILE_H
 
+#include "cache.h"
 #include "hash.h"
 
 struct progress;

@@ -38,7 +39,7 @@ int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint *);
 struct hashfile *hashfd(int fd, const char *name);
 struct hashfile *hashfd_check(const char *name);
 struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp);
-int finalize_hashfile(struct hashfile *, unsigned char *, unsigned int);
+int finalize_hashfile(struct hashfile *, unsigned char *, enum fsync_component, unsigned int);
 void hashwrite(struct hashfile *, const void *, unsigned int);
 void hashflush(struct hashfile *f);
 void crc32_begin(struct hashfile *);

environment.c +1 -0

index f9140e842cf..09905adecf9 100644
@@ -42,6 +42,7 @@ const char *git_hooks_path;
 int zlib_compression_level = Z_BEST_SPEED;
 int pack_compression_level = Z_DEFAULT_COMPRESSION;
 enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
+enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
 size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
 size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
 size_t delta_base_cache_limit = 96 * 1024 * 1024;

midx.c +2 -1

index 837b46b2af5..882f91f7d57 100644
@@ -1406,7 +1406,8 @@ static int write_midx_internal(const char *object_dir,
 	write_midx_header(f, get_num_chunks(cf), ctx.nr - dropped_packs);
 	write_chunkfile(cf, &ctx);
 
-	finalize_hashfile(f, midx_hash, CSUM_FSYNC | CSUM_HASH_IN_STREAM);
+	finalize_hashfile(f, midx_hash, FSYNC_COMPONENT_PACK_METADATA,
+			  CSUM_FSYNC | CSUM_HASH_IN_STREAM);
 	free_chunkfile(cf);
 
 	if (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))

object-file.c +1 -2

index eb972cdccd2..9d9c4a39e85 100644
@@ -1809,8 +1809,7 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,
 /* Finalize a file on disk, and close it. */
 static void close_loose_object(int fd)
 {
-	if (fsync_object_files)
-		fsync_or_die(fd, "loose object file");
+	fsync_component_or_die(FSYNC_COMPONENT_LOOSE_OBJECT, fd, "loose object file");
 	if (close(fd) != 0)
 		die_errno(_("error when closing loose object file"));
 }

pack-bitmap-write.c +2 -1

index 9c55c1531e1..c16e43d1669 100644
@@ -719,7 +719,8 @@ void bitmap_writer_finish(struct pack_idx_entry **index,
 	if (options & BITMAP_OPT_HASH_CACHE)
 		write_hash_cache(f, index, index_nr);
 
-	finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
+	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
+			  CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
 
 	if (adjust_shared_perm(tmp_file.buf))
 		die_errno("unable to make temporary bitmap file readable");

pack-write.c +7 -6

index a5846f3a346..51812cb1299 100644
@@ -159,9 +159,9 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
 	}
 
 	hashwrite(f, sha1, the_hash_algo->rawsz);
-	finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_CLOSE |
-				    ((opts->flags & WRITE_IDX_VERIFY)
-				    ? 0 : CSUM_FSYNC));
+	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
+			  CSUM_HASH_IN_STREAM | CSUM_CLOSE |
+			  ((opts->flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
 	return index_name;
 }
 

@@ -281,8 +281,9 @@ const char *write_rev_file_order(const char *rev_name,
 	if (rev_name && adjust_shared_perm(rev_name) < 0)
 		die(_("failed to make %s readable"), rev_name);
 
-	finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_CLOSE |
-				    ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
+	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
+			  CSUM_HASH_IN_STREAM | CSUM_CLOSE |
+			  ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
 
 	return rev_name;
 }

@@ -390,7 +391,7 @@ void fixup_pack_header_footer(int pack_fd,
 		the_hash_algo->final_fn(partial_pack_hash, &old_hash_ctx);
 	the_hash_algo->final_fn(new_pack_hash, &new_hash_ctx);
 	write_or_die(pack_fd, new_pack_hash, the_hash_algo->rawsz);
-	fsync_or_die(pack_fd, pack_name);
+	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
 }
 
 char *index_pack_lockfile(int ip_out, int *is_well_formed)

read-cache.c +1 -1

index f3986596623..f3539681f49 100644
@@ -3060,7 +3060,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 			return -1;
 	}
 
-	finalize_hashfile(f, istate->oid.hash, CSUM_HASH_IN_STREAM);
+	finalize_hashfile(f, istate->oid.hash, FSYNC_COMPONENT_NONE, CSUM_HASH_IN_STREAM);
 	if (close_tempfile_gently(tempfile)) {
 		error(_("could not close '%s'"), get_tempfile_path(tempfile));
 		return -1;

Signature · 1 line

gitgitgadget

Quoted 1 line · Neeraj Singh via GitGitGadget

From: Neeraj Singh <[email protected]>

[snip]

Quoted 30 lines

builtin/index-pack.c +2 -2

index c23d01de7dc..c32534c13b4 100644
@@ -1286,7 +1286,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
                          nr_objects - nr_objects_initial);
              stop_progress_msg(&progress, msg.buf);
              strbuf_release(&msg);
-             finalize_hashfile(f, tail_hash, 0);
+             finalize_hashfile(f, tail_hash, FSYNC_COMPONENT_PACK, 0);
              hashcpy(read_hash, pack_hash);
              fixup_pack_header_footer(output_fd, pack_hash,
                                       curr_pack, nr_objects,

@@ -1508,7 +1508,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
      if (!from_stdin) {
              close(input_fd);
      } else {
-             fsync_or_die(output_fd, curr_pack_name);
+             fsync_component_or_die(FSYNC_COMPONENT_PACK, output_fd, curr_pack_name);
              err = close(output_fd);
              if (err)
                      die_errno(_("error while closing pack file"));

builtin/pack-objects.c +0 -0

index 857be7826f3..916c55d6ce9 100644
@@ -1204,11 +1204,13 @@ static void write_pack_file(void)
               * If so, rewrite it like in fast-import
               */
              if (pack_to_stdout) {

builtin/pack-objects.c static void write_pack_file(void) · line 1208

Quoted 3 lines

builtin/pack-objects.c +2 -1

-                     finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE);
+                     finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE,
+                                       CSUM_HASH_IN_STREAM | CSUM_CLOSE);

It doesn't have any effect here given that we don't sync at all when writing to stdout, but I wonder whether we should set up the component correctly regardless of that such that it makes for a less confusing read.

[snip]

Quoted 93 lines

config.c +74 -0

index c3410b8a868..29c867aab03 100644
@@ -1213,6 +1213,73 @@ static int git_parse_maybe_bool_text(const char *value)
      return -1;
 }
 
+static const struct fsync_component_entry {
+     const char *name;
+     enum fsync_component component_bits;
+} fsync_component_table[] = {
+     { "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
+     { "pack", FSYNC_COMPONENT_PACK },
+     { "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
+     { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
+     { "objects", FSYNC_COMPONENTS_OBJECTS },
+     { "default", FSYNC_COMPONENTS_DEFAULT },
+     { "all", FSYNC_COMPONENTS_ALL },
+};
+
+static enum fsync_component parse_fsync_components(const char *var, const char *string)
+{
+     enum fsync_component output = 0;
+
+     if (!strcmp(string, "none"))
+             return output;
+
+     while (string) {
+             int i;
+             size_t len;
+             const char *ep;
+             int negated = 0;
+             int found = 0;
+
+             string = string + strspn(string, ", \t\n\r");
+             ep = strchrnul(string, ',');
+             len = ep - string;
+
+             if (*string == '-') {
+                     negated = 1;
+                     string++;
+                     len--;
+                     if (!len)
+                             warning(_("invalid value for variable %s"), var);
+             }
+
+             if (!len)
+                     break;
+
+             for (i = 0; i < ARRAY_SIZE(fsync_component_table); ++i) {
+                     const struct fsync_component_entry *entry = &fsync_component_table[i];
+
+                     if (strncmp(entry->name, string, len))
+                             continue;
+
+                     found = 1;
+                     if (negated)
+                             output &= ~entry->component_bits;
+                     else
+                             output |= entry->component_bits;
+             }
+
+             if (!found) {
+                     char *component = xstrndup(string, len);
+                     warning(_("unknown %s value '%s'"), var, component);
+                     free(component);
+             }
+
+             string = ep;
+     }
+
+     return output;
+}
+
 int git_parse_maybe_bool(const char *value)
 {
      int v = git_parse_maybe_bool_text(value);

@@ -1490,6 +1557,13 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
              return 0;
      }
 
+     if (!strcmp(var, "core.fsync")) {
+             if (!value)
+                     return config_error_nonbool(var);
+             fsync_components = parse_fsync_components(var, value);
+             return 0;
+     }
+
      if (!strcmp(var, "core.fsyncmethod")) {
              if (!value)
                      return config_error_nonbool(var);

config.c static int git_default_core_config(const char *var, const char *value, void *cb) · line 1582

Quoted 7 lines

config.c +1 -1

@@ -1503,7 +1577,7 @@ 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);
+             warning(_("core.fsyncobjectfiles is deprecated; use core.fsync instead"));
              return 0;
      }

Shouldn't we continue to support this for now such that users can migrate from the old, deprecated value first before we start to ignore it?

Patrick

From: Neeraj Singh <[email protected]>

This commit introduces the core.fsync configuration knob which can be used to control how components of the repository are made durable on disk.

This setting allows future extensibility of the list of syncable components:
* We issue a warning rather than an error for unrecognized components, so new configs can be used with old Git versions.

* We support negation, so users can choose one of the default aggregate options and then remove components that they don't want. The user would then harden any new components added in a Git version update.

This also supports the common request of doing absolutely no fysncing with the core.fsync=none value, which is expected to make the test suite faster.

Signed-off-by Neeraj Singh <[email protected]>

---
 Documentation/config/core.txt | 27 +++++++++----
 builtin/fast-import.c         |  2 +-
 builtin/index-pack.c          |  4 +-
 builtin/pack-objects.c        |  8 ++--
 bulk-checkin.c                |  5 ++-
 cache.h                       | 39 +++++++++++++++++-
 commit-graph.c                |  3 +-
 config.c                      | 76 ++++++++++++++++++++++++++++++++++-
 csum-file.c                   |  5 ++-
 csum-file.h                   |  3 +-
 environment.c                 |  1 +
 midx.c                        |  3 +-
 object-file.c                 |  3 +-
 pack-bitmap-write.c           |  3 +-
 pack-write.c                  | 13 +++---
 read-cache.c                  |  2 +-
 16 files changed, 164 insertions(+), 33 deletions(-)

Documentation/config/core.txt +9 -0

index dbb134f7136..4f1747ec871 100644
@@ -547,6 +547,25 @@ core.whitespace::
   is relevant for `indent-with-non-tab` and when Git fixes `tab-in-indent`
   errors. The default tab width is 8. Allowed values are 1 to 63.
 
+core.fsync::
+	A comma-separated list of parts of the repository which should be
+	hardened via the core.fsyncMethod when created or modified. You can
+	disable hardening of any component by prefixing it with a '-'. Later
+	items take precedence over earlier ones in the list. For example,
+	`core.fsync=all,-pack-metadata` means "harden everything except pack
+	metadata." Items that are not hardened may be lost in the event of an
+	unclean system shutdown.
++

builtin/pack-objects.c +18 -0

+static const struct fsync_component_entry {
+	const char *name;
+	enum fsync_component component_bits;
+} fsync_component_table[] = {
+	{ "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
+	{ "pack", FSYNC_COMPONENT_PACK },
+	{ "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
+	{ "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
+	{ "objects", FSYNC_COMPONENTS_OBJECTS },
+	{ "default", FSYNC_COMPONENTS_DEFAULT },
+	{ "all", FSYNC_COMPONENTS_ALL },
+};
+
+static enum fsync_component parse_fsync_components(const char *var, const char *string)
+{
+	enum fsync_component output = 0;
+
+	if (!strcmp(string, "none"))

Quoted 10 lines · Neeraj Singh via GitGitGadget

From: Neeraj Singh <[email protected]>

This commit introduces the core.fsync configuration knob which can be used to control how components of the repository are made durable on disk.

This setting allows future extensibility of the list of syncable components:
* We issue a warning rather than an error for unrecognized components, so new configs can be used with old Git versions.

Looks good!

Quoted 4 lines

* We support negation, so users can choose one of the default aggregate options and then remove components that they don't want. The user would then harden any new components added in a Git version update.

I think this config schema makes sense, but just a (I think important) comment on the "how" not "what" of it. It's really much better to define config as:

[some]
key = value
key = value2

Than:

[some]
key = value,value2

The reason is that "git config" has good support for working with multi-valued stuff, so you can do e.g.:

    git config --get-all -z some.key

And you can easily (re)set such config e.g. with --replace-all etc., but for comma-delimited you (and users) need to do all that work themselves.

Similarly instead of:

some.key = want-this
some.key = -not-this
some.key = but-want-this

I think it's better to just have two lists, one inclusive another exclusive. E.g. see "log.decorate" and "log.excludeDecoration", "transfer.hideRefs"

Which would mean:

core.fsync = want-this
core.fsyncExcludes = -not-this

For some value of "fsyncExcludes", maybe "noFsync"? Anyway, just a suggestion on making this easier for users & the implementation.

Quoted 3 lines

This also supports the common request of doing absolutely no fysncing with the core.fsync=none value, which is expected to make the test suite faster.

Let's just use the git_parse_maybe_bool() or git_parse_maybe_bool_text() so we'll accept "false", "off", "no" like most other such config?

Quoted 38 lines

Signed-off-by Neeraj Singh <[email protected]>

---
 Documentation/config/core.txt | 27 +++++++++----
 builtin/fast-import.c         |  2 +-
 builtin/index-pack.c          |  4 +-
 builtin/pack-objects.c        |  8 ++--
 bulk-checkin.c                |  5 ++-
 cache.h                       | 39 +++++++++++++++++-
 commit-graph.c                |  3 +-
 config.c                      | 76 ++++++++++++++++++++++++++++++++++-
 csum-file.c                   |  5 ++-
 csum-file.h                   |  3 +-
 environment.c                 |  1 +
 midx.c                        |  3 +-
 object-file.c                 |  3 +-
 pack-bitmap-write.c           |  3 +-
 pack-write.c                  | 13 +++---
 read-cache.c                  |  2 +-
 16 files changed, 164 insertions(+), 33 deletions(-)

Documentation/config/core.txt +9 -0

index dbb134f7136..4f1747ec871 100644
@@ -547,6 +547,25 @@ core.whitespace::
   is relevant for `indent-with-non-tab` and when Git fixes `tab-in-indent`
   errors. The default tab width is 8. Allowed values are 1 to 63.
 
+core.fsync::
+     A comma-separated list of parts of the repository which should be
+     hardened via the core.fsyncMethod when created or modified. You can
+     disable hardening of any component by prefixing it with a '-'. Later
+     items take precedence over earlier ones in the list. For example,
+     `core.fsync=all,-pack-metadata` means "harden everything except pack
+     metadata." Items that are not hardened may be lost in the event of an
+     unclean system shutdown.
++

Documentation/config/core.txt core.whitespace:: · line 568

Quoted 10 lines

Documentation/config/core.txt +10 -0

+* `none` disables fsync completely. This must be specified alone.
+* `loose-object` hardens objects added to the repo in loose-object form.
+* `pack` hardens objects added to the repo in packfile form.
+* `pack-metadata` hardens packfile bitmaps and indexes.
+* `commit-graph` hardens the commit graph file.
+* `objects` is an aggregate option that includes `loose-objects`, `pack`,
+  `pack-metadata`, and `commit-graph`.
+* `default` is an aggregate option that is equivalent to `objects,-loose-object`
+* `all` is an aggregate option that syncs all individual components above.
+

It's probably a *bit* more work to set up, but I wonder if this wouldn't be simpler if we just said (and this is partially going against what I noted above):

== BEGIN DOC

core.fsync is a multi-value config variable where each item is a pathspec that'll get matched the same way as 'git-ls-files' et al.

When we sync pretend that a path like .git/objects/de/adbeef... is relative to the top-level of the git
directory. E.g. "objects/de/adbeaf.." or "objects/pack/...".

You can then supply a list of wildcards and exclusions to configure syncing. or "false", "off" etc. to turn it off. These are synonymous with:

; same as "false"
core.fsync = ":!*"

Or:

; same as "true"
core.fsync = "*"

Or, to selectively sync some things and not others:

    ;; Sync objects, but not "info"
    core.fsync = ":!objects/info/**"
    core.fsync = "objects/**"

See gitrepository-layout(5) for details about what sort of paths you might be expected to match. Not all paths listed there will go through this mechanism (e.g. currently objects do, but nothing to do with config does).

We can and will match this against "fake paths", e.g. when writing out packs we may match against just the string "objects/pack", we're not going to re-check if every packfile we're writing matches your globs, ditto for loose objects. Be reasonable!

This metharism is intended as a shorthand that provides some flexibility when fsyncing, while not forcing git to come up with labels for all paths the git dir, or to support crazyness like "objects/de/adbeef*"

More paths may be added or removed in the future, and we make no promises that we won't move things around, so if in doubt use e.g. "true" or a wide pattern match like "objects/**". When in doubt stick to the golden path of examples provided in this documentation.

== END DOC

It's a tad more complex to set up, but I wonder if that isn't worth it. It nicely gets around any current and future issues of deciding what labels such as "loose-object" etc. to pick, as well as slotting into an existing method of doing exclude/include lists.

builtin/pack-objects.c static void write_pack_file(void) · line 1208

Quoted 11 lines

builtin/pack-objects.c +2 -1

index 857be7826f3..916c55d6ce9 100644
@@ -1204,11 +1204,13 @@ static void write_pack_file(void)
               * If so, rewrite it like in fast-import
               */
              if (pack_to_stdout) {
-                     finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE);
+                     finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE,
+                                       CSUM_HASH_IN_STREAM | CSUM_CLOSE);

Not really related to this per-se, but since you're touching the API everything goes through I wonder if callers should just always try to fsync, and we can just catch EROFS and EINVAL in the wrapper if someone tries to flush stdout, or catch the fd at that lower level.

Or maybe there's a good reason for this...

builtin/pack-objects.c

Quoted 8 lines

[...]

builtin/pack-objects.c +7 -0

+/*
+ * These values are used to help identify parts of a repository to fsync.
+ * FSYNC_COMPONENT_NONE identifies data that will not be a persistent part of the
+ * repository and so shouldn't be fsynced.
+ */
+enum fsync_component {
+     FSYNC_COMPONENT_NONE                    = 0,

I haven't read ahead much but in most other such cases we don't define the "= 0", just start at 1<<0, then check the flags elsewhere...

Quoted 23 lines

builtin/pack-objects.c +23 -0

+static const struct fsync_component_entry {
+     const char *name;
+     enum fsync_component component_bits;
+} fsync_component_table[] = {
+     { "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
+     { "pack", FSYNC_COMPONENT_PACK },
+     { "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
+     { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
+     { "objects", FSYNC_COMPONENTS_OBJECTS },
+     { "default", FSYNC_COMPONENTS_DEFAULT },
+     { "all", FSYNC_COMPONENTS_ALL },
+};
+
+static enum fsync_component parse_fsync_components(const char *var, const char *string)
+{
+     enum fsync_component output = 0;
+
+     if (!strcmp(string, "none"))
+             return output;
+
+     while (string) {
+             int i;
+             size_t len;

builtin/pack-objects.c

Quoted 5 lines

builtin/pack-objects.c +5 -0

+             const char *ep;
+             int negated = 0;
+             int found = 0;
+
+             string = string + strspn(string, ", \t\n\r");

Aside from the "use a list" isn't this hardcoding some windows-specific assumptions with \n\r? Maybe not...

Quoted 53 lines · Ævar Arnfjörð Bjarmason

Quoted 10 lines · Neeraj Singh via GitGitGadget

From: Neeraj Singh <[email protected]>

This commit introduces the core.fsync configuration knob which can be used to control how components of the repository are made durable on disk.

This setting allows future extensibility of the list of syncable components:
* We issue a warning rather than an error for unrecognized components, so new configs can be used with old Git versions.

Looks good!

Quoted 4 lines

* We support negation, so users can choose one of the default aggregate options and then remove components that they don't want. The user would then harden any new components added in a Git version update.

I think this config schema makes sense, but just a (I think important) comment on the "how" not "what" of it. It's really much better to define config as:

[some]
key = value
key = value2

Than:

[some]
key = value,value2

The reason is that "git config" has good support for working with multi-valued stuff, so you can do e.g.:

    git config --get-all -z some.key

And you can easily (re)set such config e.g. with --replace-all etc., but for comma-delimited you (and users) need to do all that work themselves.

Similarly instead of:

some.key = want-this
some.key = -not-this
some.key = but-want-this

I think it's better to just have two lists, one inclusive another exclusive. E.g. see "log.decorate" and "log.excludeDecoration", "transfer.hideRefs"

Quoted 8 lines

Which would mean:

core.fsync = want-this
core.fsyncExcludes = -not-this

For some value of "fsyncExcludes", maybe "noFsync"? Anyway, just a suggestion on making this easier for users & the implementation.

Maybe there's some way to handle this I'm unaware of, but a disadvantage of your multi-valued config proposal is that it's harder, for example, for a per-repo config store to reasonably override a per-user config store. With the configuration scheme as-is, I can have a per-user setting like core.fsync=all which covers my typical repos, but then have a maintainer repo with a private setting of core.fsync=none to speed up cases where I'm mostly working with other people's changes that are backed up in email or server-side repos. The latter setting conveniently overrides the former setting in all aspects.

Also, with the core.fsync and core.fsyncExcludes, how would you spell "don't sync anything"? Would you still have the aggregate options.?

Quoted 6 lines

Quoted 3 lines

This also supports the common request of doing absolutely no fysncing with the core.fsync=none value, which is expected to make the test suite faster.

Let's just use the git_parse_maybe_bool() or git_parse_maybe_bool_text() so we'll accept "false", "off", "no" like most other such config?

Junio's previous feedback when discussing batch mode [1] was to offer less flexibility when parsing new values of these configuration options. I agree with his statement that "making machine-readable tokens be spelled in different ways is a 'disease'." I'd like to leave this as-is so that the documentation can clearly state the exact set of allowable values.

[1] https://lore.kernel.org/git/[email protected]/

Quoted 99 lines

Quoted 38 lines

Signed-off-by Neeraj Singh <[email protected]>

---
 Documentation/config/core.txt | 27 +++++++++----
 builtin/fast-import.c         |  2 +-
 builtin/index-pack.c          |  4 +-
 builtin/pack-objects.c        |  8 ++--
 bulk-checkin.c                |  5 ++-
 cache.h                       | 39 +++++++++++++++++-
 commit-graph.c                |  3 +-
 config.c                      | 76 ++++++++++++++++++++++++++++++++++-
 csum-file.c                   |  5 ++-
 csum-file.h                   |  3 +-
 environment.c                 |  1 +
 midx.c                        |  3 +-
 object-file.c                 |  3 +-
 pack-bitmap-write.c           |  3 +-
 pack-write.c                  | 13 +++---
 read-cache.c                  |  2 +-
 16 files changed, 164 insertions(+), 33 deletions(-)

Documentation/config/core.txt +9 -0

index dbb134f7136..4f1747ec871 100644
@@ -547,6 +547,25 @@ core.whitespace::
   is relevant for `indent-with-non-tab` and when Git fixes `tab-in-indent`
   errors. The default tab width is 8. Allowed values are 1 to 63.
 
+core.fsync::
+     A comma-separated list of parts of the repository which should be
+     hardened via the core.fsyncMethod when created or modified. You can
+     disable hardening of any component by prefixing it with a '-'. Later
+     items take precedence over earlier ones in the list. For example,
+     `core.fsync=all,-pack-metadata` means "harden everything except pack
+     metadata." Items that are not hardened may be lost in the event of an
+     unclean system shutdown.
++

Documentation/config/core.txt core.whitespace:: · line 568

Quoted 10 lines

Documentation/config/core.txt +10 -0

+* `none` disables fsync completely. This must be specified alone.
+* `loose-object` hardens objects added to the repo in loose-object form.
+* `pack` hardens objects added to the repo in packfile form.
+* `pack-metadata` hardens packfile bitmaps and indexes.
+* `commit-graph` hardens the commit graph file.
+* `objects` is an aggregate option that includes `loose-objects`, `pack`,
+  `pack-metadata`, and `commit-graph`.
+* `default` is an aggregate option that is equivalent to `objects,-loose-object`
+* `all` is an aggregate option that syncs all individual components above.
+

It's probably a *bit* more work to set up, but I wonder if this wouldn't be simpler if we just said (and this is partially going against what I noted above):

== BEGIN DOC

core.fsync is a multi-value config variable where each item is a pathspec that'll get matched the same way as 'git-ls-files' et al.

When we sync pretend that a path like .git/objects/de/adbeef... is relative to the top-level of the git
directory. E.g. "objects/de/adbeaf.." or "objects/pack/...".

You can then supply a list of wildcards and exclusions to configure syncing. or "false", "off" etc. to turn it off. These are synonymous with:

; same as "false"
core.fsync = ":!*"

Or:

; same as "true"
core.fsync = "*"

Or, to selectively sync some things and not others:

    ;; Sync objects, but not "info"
    core.fsync = ":!objects/info/**"
    core.fsync = "objects/**"

See gitrepository-layout(5) for details about what sort of paths you might be expected to match. Not all paths listed there will go through this mechanism (e.g. currently objects do, but nothing to do with config does).

We can and will match this against "fake paths", e.g. when writing out packs we may match against just the string "objects/pack", we're not going to re-check if every packfile we're writing matches your globs, ditto for loose objects. Be reasonable!

This metharism is intended as a shorthand that provides some flexibility when fsyncing, while not forcing git to come up with labels for all paths the git dir, or to support crazyness like "objects/de/adbeef*"

More paths may be added or removed in the future, and we make no promises that we won't move things around, so if in doubt use e.g. "true" or a wide pattern match like "objects/**". When in doubt stick to the golden path of examples provided in this documentation.

Quoted 8 lines

== END DOC

It's a tad more complex to set up, but I wonder if that isn't worth it. It nicely gets around any current and future issues of deciding what labels such as "loose-object" etc. to pick, as well as slotting into an existing method of doing exclude/include lists.

I think this proposal is a lot of complexity to avoid coming up with a new name for syncable things as they are added to Git. A path based mechanism makes it hard to document for the (advanced) user what the full set of things is and how it might change from release to release. I think the current core.fsync scheme is a bit easier to understand, query, and extend.

Quoted 17 lines

builtin/pack-objects.c static void write_pack_file(void) · line 1208

Quoted 11 lines

builtin/pack-objects.c +2 -1

index 857be7826f3..916c55d6ce9 100644
@@ -1204,11 +1204,13 @@ static void write_pack_file(void)
               * If so, rewrite it like in fast-import
               */
              if (pack_to_stdout) {
-                     finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE);
+                     finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE,
+                                       CSUM_HASH_IN_STREAM | CSUM_CLOSE);

Not really related to this per-se, but since you're touching the API everything goes through I wonder if callers should just always try to fsync, and we can just catch EROFS and EINVAL in the wrapper if someone tries to flush stdout, or catch the fd at that lower level.

Quoted 1 line

Or maybe there's a good reason for this...

It's platform dependent, but I'd expect fsync would do something for pipes or stdout redirected to a file. In these cases we really don't want to fsync since we have no idea what we're talking to and we're potentially worsening performance for probably no benefit.

Quoted 35 lines

builtin/pack-objects.c

Quoted 8 lines

[...]

builtin/pack-objects.c +7 -0

+/*
+ * These values are used to help identify parts of a repository to fsync.
+ * FSYNC_COMPONENT_NONE identifies data that will not be a persistent part of the
+ * repository and so shouldn't be fsynced.
+ */
+enum fsync_component {
+     FSYNC_COMPONENT_NONE                    = 0,

I haven't read ahead much but in most other such cases we don't define the "= 0", just start at 1<<0, then check the flags elsewhere...

Quoted 23 lines

builtin/pack-objects.c +23 -0

+static const struct fsync_component_entry {
+     const char *name;
+     enum fsync_component component_bits;
+} fsync_component_table[] = {
+     { "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
+     { "pack", FSYNC_COMPONENT_PACK },
+     { "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
+     { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
+     { "objects", FSYNC_COMPONENTS_OBJECTS },
+     { "default", FSYNC_COMPONENTS_DEFAULT },
+     { "all", FSYNC_COMPONENTS_ALL },
+};
+
+static enum fsync_component parse_fsync_components(const char *var, const char *string)
+{
+     enum fsync_component output = 0;
+
+     if (!strcmp(string, "none"))
+             return output;
+
+     while (string) {
+             int i;
+             size_t len;

Quoted 8 lines

builtin/pack-objects.c

Quoted 5 lines

builtin/pack-objects.c +5 -0

+             const char *ep;
+             int negated = 0;
+             int found = 0;
+
+             string = string + strspn(string, ", \t\n\r");

Aside from the "use a list" isn't this hardcoding some windows-specific assumptions with \n\r? Maybe not...

I shamelessly stole this code from parse_whitespace_rule. I thought about making a helper to be called by both functions, but the amount of state going into and out of the wrapper via arguments was substantial and seemed to negate the benefit of deduplication.

Quoted 63 lines · Neeraj Singh

Quoted 53 lines · Ævar Arnfjörð Bjarmason

Quoted 10 lines · Neeraj Singh via GitGitGadget

From: Neeraj Singh <[email protected]>

This commit introduces the core.fsync configuration knob which can be used to control how components of the repository are made durable on disk.

This setting allows future extensibility of the list of syncable components:
* We issue a warning rather than an error for unrecognized components, so new configs can be used with old Git versions.

Looks good!

Quoted 4 lines

* We support negation, so users can choose one of the default aggregate options and then remove components that they don't want. The user would then harden any new components added in a Git version update.

I think this config schema makes sense, but just a (I think important) comment on the "how" not "what" of it. It's really much better to define config as:

[some]
key = value
key = value2

Than:

[some]
key = value,value2

The reason is that "git config" has good support for working with multi-valued stuff, so you can do e.g.:

    git config --get-all -z some.key

And you can easily (re)set such config e.g. with --replace-all etc., but for comma-delimited you (and users) need to do all that work themselves.

Similarly instead of:

some.key = want-this
some.key = -not-this
some.key = but-want-this

I think it's better to just have two lists, one inclusive another exclusive. E.g. see "log.decorate" and "log.excludeDecoration", "transfer.hideRefs"

Quoted 8 lines

Which would mean:

core.fsync = want-this
core.fsyncExcludes = -not-this

For some value of "fsyncExcludes", maybe "noFsync"? Anyway, just a suggestion on making this easier for users & the implementation.

Quoted 10 lines

Maybe there's some way to handle this I'm unaware of, but a disadvantage of your multi-valued config proposal is that it's harder, for example, for a per-repo config store to reasonably override a per-user config store. With the configuration scheme as-is, I can have a per-user setting like core.fsync=all which covers my typical repos, but then have a maintainer repo with a private setting of core.fsync=none to speed up cases where I'm mostly working with other people's changes that are backed up in email or server-side repos. The latter setting conveniently overrides the former setting in all aspects.

Even if you turn just your comma-delimited proposal into a list proposal can't we just say that the last one wins? Then it won't matter what cmae before, you'd specify "core.fsync=none" in your local .git/config.

But this is also a general issue with a bunch of things in git's config space. I'd rather see us use the list-based values and just come up with some general way to reset them that works for all keys, rather than regretting having comma-delimited values that'll be harder to work with & parse, which will be a legacy wart if/when we come up with a way to say "reset all previous settings".

Quoted 2 lines

Also, with the core.fsync and core.fsyncExcludes, how would you spell "don't sync anything"? Would you still have the aggregate options.?

core.fsyncExcludes = *

I.e. the same as the core.fsync=none above, anyway I still like the wildcard thing below a bit more...

Quoted 14 lines

Quoted 6 lines

Quoted 3 lines

This also supports the common request of doing absolutely no fysncing with the core.fsync=none value, which is expected to make the test suite faster.

Let's just use the git_parse_maybe_bool() or git_parse_maybe_bool_text() so we'll accept "false", "off", "no" like most other such config?

Junio's previous feedback when discussing batch mode [1] was to offer less flexibility when parsing new values of these configuration options. I agree with his statement that "making machine-readable tokens be spelled in different ways is a 'disease'." I'd like to leave this as-is so that the documentation can clearly state the exact set of allowable values.

Quoted 1 line

[1] https://lore.kernel.org/git/[email protected]/

I think he's talking about batch, Batch, BATCH, bAtCh etc. there. But the "maybe bool" is a stanard pattern we use.

I don't think we'd call one of these 0, off, no or false etc. to avoid confusion, so then you can use git_parse_maybe_...()

Quoted 108 lines

Quoted 99 lines

Quoted 38 lines

Signed-off-by Neeraj Singh <[email protected]>

---
 Documentation/config/core.txt | 27 +++++++++----
 builtin/fast-import.c         |  2 +-
 builtin/index-pack.c          |  4 +-
 builtin/pack-objects.c        |  8 ++--
 bulk-checkin.c                |  5 ++-
 cache.h                       | 39 +++++++++++++++++-
 commit-graph.c                |  3 +-
 config.c                      | 76 ++++++++++++++++++++++++++++++++++-
 csum-file.c                   |  5 ++-
 csum-file.h                   |  3 +-
 environment.c                 |  1 +
 midx.c                        |  3 +-
 object-file.c                 |  3 +-
 pack-bitmap-write.c           |  3 +-
 pack-write.c                  | 13 +++---
 read-cache.c                  |  2 +-
 16 files changed, 164 insertions(+), 33 deletions(-)

Documentation/config/core.txt +9 -0

index dbb134f7136..4f1747ec871 100644
@@ -547,6 +547,25 @@ core.whitespace::
   is relevant for `indent-with-non-tab` and when Git fixes `tab-in-indent`
   errors. The default tab width is 8. Allowed values are 1 to 63.
 
+core.fsync::
+     A comma-separated list of parts of the repository which should be
+     hardened via the core.fsyncMethod when created or modified. You can
+     disable hardening of any component by prefixing it with a '-'. Later
+     items take precedence over earlier ones in the list. For example,
+     `core.fsync=all,-pack-metadata` means "harden everything except pack
+     metadata." Items that are not hardened may be lost in the event of an
+     unclean system shutdown.
++

Documentation/config/core.txt core.whitespace:: · line 568

Quoted 10 lines

Documentation/config/core.txt +10 -0

+* `none` disables fsync completely. This must be specified alone.
+* `loose-object` hardens objects added to the repo in loose-object form.
+* `pack` hardens objects added to the repo in packfile form.
+* `pack-metadata` hardens packfile bitmaps and indexes.
+* `commit-graph` hardens the commit graph file.
+* `objects` is an aggregate option that includes `loose-objects`, `pack`,
+  `pack-metadata`, and `commit-graph`.
+* `default` is an aggregate option that is equivalent to `objects,-loose-object`
+* `all` is an aggregate option that syncs all individual components above.
+

It's probably a *bit* more work to set up, but I wonder if this wouldn't be simpler if we just said (and this is partially going against what I noted above):

== BEGIN DOC

core.fsync is a multi-value config variable where each item is a pathspec that'll get matched the same way as 'git-ls-files' et al.

When we sync pretend that a path like .git/objects/de/adbeef... is relative to the top-level of the git
directory. E.g. "objects/de/adbeaf.." or "objects/pack/...".

You can then supply a list of wildcards and exclusions to configure syncing. or "false", "off" etc. to turn it off. These are synonymous with:

; same as "false"
core.fsync = ":!*"

Or:

; same as "true"
core.fsync = "*"

Or, to selectively sync some things and not others:

    ;; Sync objects, but not "info"
    core.fsync = ":!objects/info/**"
    core.fsync = "objects/**"

See gitrepository-layout(5) for details about what sort of paths you might be expected to match. Not all paths listed there will go through this mechanism (e.g. currently objects do, but nothing to do with config does).

We can and will match this against "fake paths", e.g. when writing out packs we may match against just the string "objects/pack", we're not going to re-check if every packfile we're writing matches your globs, ditto for loose objects. Be reasonable!

This metharism is intended as a shorthand that provides some flexibility when fsyncing, while not forcing git to come up with labels for all paths the git dir, or to support crazyness like "objects/de/adbeef*"

More paths may be added or removed in the future, and we make no promises that we won't move things around, so if in doubt use e.g. "true" or a wide pattern match like "objects/**". When in doubt stick to the golden path of examples provided in this documentation.

Quoted 8 lines

== END DOC

It's a tad more complex to set up, but I wonder if that isn't worth it. It nicely gets around any current and future issues of deciding what labels such as "loose-object" etc. to pick, as well as slotting into an existing method of doing exclude/include lists.

Quoted 6 lines

I think this proposal is a lot of complexity to avoid coming up with a new name for syncable things as they are added to Git. A path based mechanism makes it hard to document for the (advanced) user what the full set of things is and how it might change from release to release. I think the current core.fsync scheme is a bit easier to understand, query, and extend.

We document it in gitrepository-layout(5). Yeah it has some disadvantages, but one advantage is that you could make the composability easy. I.e. if last exclude wins then a setting of:

    core.fsync = ":!*"
    core.fsync = "objects/**"

Would reset all previous matches & only match objects/**.

Quoted 17 lines

Quoted 17 lines

builtin/pack-objects.c static void write_pack_file(void) · line 1208

Quoted 11 lines

builtin/pack-objects.c +2 -1

index 857be7826f3..916c55d6ce9 100644
@@ -1204,11 +1204,13 @@ static void write_pack_file(void)
               * If so, rewrite it like in fast-import
               */
              if (pack_to_stdout) {
-                     finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE);
+                     finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE,
+                                       CSUM_HASH_IN_STREAM | CSUM_CLOSE);

Not really related to this per-se, but since you're touching the API everything goes through I wonder if callers should just always try to fsync, and we can just catch EROFS and EINVAL in the wrapper if someone tries to flush stdout, or catch the fd at that lower level.

Quoted 6 lines

Quoted 1 line

Or maybe there's a good reason for this...

It's platform dependent, but I'd expect fsync would do something for pipes or stdout redirected to a file. In these cases we really don't want to fsync since we have no idea what we're talking to and we're potentially worsening performance for probably no benefit.

Yeah maybe we should just leave it be.

I'd think the C library returning EINVAL would be a trivial performance cost though.

It just seemed odd to hardcode assumptions about what can and can't be synced when the POSIX defined function will also tell us that.

Quoted 45 lines

Quoted 44 lines

builtin/pack-objects.c

Quoted 9 lines

[...]

builtin/pack-objects.c +7 -0

+/*
+ * These values are used to help identify parts of a repository to fsync.
+ * FSYNC_COMPONENT_NONE identifies data that will not be a persistent part of the
+ * repository and so shouldn't be fsynced.
+ */
+enum fsync_component {
+     FSYNC_COMPONENT_NONE                    = 0,

I haven't read ahead much but in most other such cases we don't define the "= 0", just start at 1<<0, then check the flags elsewhere...

Quoted 18 lines

builtin/pack-objects.c +18 -0

+static const struct fsync_component_entry {
+     const char *name;
+     enum fsync_component component_bits;
+} fsync_component_table[] = {
+     { "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
+     { "pack", FSYNC_COMPONENT_PACK },
+     { "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
+     { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
+     { "objects", FSYNC_COMPONENTS_OBJECTS },
+     { "default", FSYNC_COMPONENTS_DEFAULT },
+     { "all", FSYNC_COMPONENTS_ALL },
+};
+
+static enum fsync_component parse_fsync_components(const char *var, const char *string)
+{
+     enum fsync_component output = 0;
+
+     if (!strcmp(string, "none"))

builtin/pack-objects.c

Quoted 10 lines

builtin/pack-objects.c +10 -0

+             return output;
+
+     while (string) {
+             int i;
+             size_t len;
+             const char *ep;
+             int negated = 0;
+             int found = 0;
+
+             string = string + strspn(string, ", \t\n\r");

Aside from the "use a list" isn't this hardcoding some windows-specific assumptions with \n\r? Maybe not...

Quoted 4 lines

I shamelessly stole this code from parse_whitespace_rule. I thought about making a helper to be called by both functions, but the amount of state going into and out of the wrapper via arguments was substantial and seemed to negate the benefit of deduplication.

FWIW string_list_split() is easier to work with in those cases, or at least I think so...

I'm not going to take this feedback unless there are additional votes from the Git community in this direction. I make the claim that single-valued comma-separated config lists are easier to work with in the existing Git infrastructure. We already use essentially the same parsing code for the core.whitespace variable and users are used to this syntax there. There are several other comma-separated lists in the config space, so this construct has precedence and will be with Git for some time. Also, fsync configurations aren't composable like some other configurations may be. It makes sense to have a holistic singular fsync configuration, which is best represented by a single variable.

Quoted 94 lines · Ævar Arnfjörð Bjarmason

Quoted 63 lines · Neeraj Singh

Quoted 53 lines · Ævar Arnfjörð Bjarmason

Quoted 10 lines · Neeraj Singh via GitGitGadget

From: Neeraj Singh <[email protected]>

This commit introduces the core.fsync configuration knob which can be used to control how components of the repository are made durable on disk.

This setting allows future extensibility of the list of syncable components:
* We issue a warning rather than an error for unrecognized components, so new configs can be used with old Git versions.

Looks good!

Quoted 4 lines

* We support negation, so users can choose one of the default aggregate options and then remove components that they don't want. The user would then harden any new components added in a Git version update.

I think this config schema makes sense, but just a (I think important) comment on the "how" not "what" of it. It's really much better to define config as:

[some]
key = value
key = value2

Than:

[some]
key = value,value2

The reason is that "git config" has good support for working with multi-valued stuff, so you can do e.g.:

    git config --get-all -z some.key

And you can easily (re)set such config e.g. with --replace-all etc., but for comma-delimited you (and users) need to do all that work themselves.

Similarly instead of:

some.key = want-this
some.key = -not-this
some.key = but-want-this

I think it's better to just have two lists, one inclusive another exclusive. E.g. see "log.decorate" and "log.excludeDecoration", "transfer.hideRefs"

Quoted 8 lines

Which would mean:

core.fsync = want-this
core.fsyncExcludes = -not-this

For some value of "fsyncExcludes", maybe "noFsync"? Anyway, just a suggestion on making this easier for users & the implementation.

Quoted 10 lines

Maybe there's some way to handle this I'm unaware of, but a disadvantage of your multi-valued config proposal is that it's harder, for example, for a per-repo config store to reasonably override a per-user config store. With the configuration scheme as-is, I can have a per-user setting like core.fsync=all which covers my typical repos, but then have a maintainer repo with a private setting of core.fsync=none to speed up cases where I'm mostly working with other people's changes that are backed up in email or server-side repos. The latter setting conveniently overrides the former setting in all aspects.

Even if you turn just your comma-delimited proposal into a list proposal can't we just say that the last one wins? Then it won't matter what cmae before, you'd specify "core.fsync=none" in your local .git/config.

But this is also a general issue with a bunch of things in git's config space. I'd rather see us use the list-based values and just come up with some general way to reset them that works for all keys, rather than regretting having comma-delimited values that'll be harder to work with & parse, which will be a legacy wart if/when we come up with a way to say "reset all previous settings".

Quoted 2 lines

Also, with the core.fsync and core.fsyncExcludes, how would you spell "don't sync anything"? Would you still have the aggregate options.?

core.fsyncExcludes = *

Quoted 2 lines

I.e. the same as the core.fsync=none above, anyway I still like the wildcard thing below a bit more...

I'm not going to take this feedback unless there are additional votes from the Git community in this direction. I make the claim that single-valued comma-separated config lists are easier to work with in the existing Git infrastructure.

parsing code for the core.whitespace variable and users are used to this syntax there. There are several other comma-separated lists in the config space, so this construct has precedence and will be with Git for some time.

Also, fsync configurations aren't composable like
some other configurations may be. It makes sense to have a holistic singular fsync configuration, which is best represented by a single variable.

Quoted 14 lines

Quoted 14 lines

Quoted 6 lines

Quoted 3 lines

This also supports the common request of doing absolutely no fysncing with the core.fsync=none value, which is expected to make the test suite faster.

Let's just use the git_parse_maybe_bool() or git_parse_maybe_bool_text() so we'll accept "false", "off", "no" like most other such config?

Junio's previous feedback when discussing batch mode [1] was to offer less flexibility when parsing new values of these configuration options. I agree with his statement that "making machine-readable tokens be spelled in different ways is a 'disease'." I'd like to leave this as-is so that the documentation can clearly state the exact set of allowable values.

Quoted 7 lines

Quoted 1 line

[1] https://lore.kernel.org/git/[email protected]/

I think he's talking about batch, Batch, BATCH, bAtCh etc. there. But the "maybe bool" is a stanard pattern we use.

I don't think we'd call one of these 0, off, no or false etc. to avoid confusion, so then you can use git_parse_maybe_...()

I don't see the advantage of having multiple ways of specifying "none". The user can read the doc and know exactly what to write. If they write something unallowable, they get a clear warning and they can read the doc again to figure out what to write. This isn't a boolean options at all, so why should we entertain bool-like ways of spelling it?

Quoted 121 lines

Quoted 107 lines

Quoted 98 lines

Quoted 37 lines

Signed-off-by Neeraj Singh <[email protected]>

---
 Documentation/config/core.txt | 27 +++++++++----
 builtin/fast-import.c         |  2 +-
 builtin/index-pack.c          |  4 +-
 builtin/pack-objects.c        |  8 ++--
 bulk-checkin.c                |  5 ++-
 cache.h                       | 39 +++++++++++++++++-
 commit-graph.c                |  3 +-
 config.c                      | 76 ++++++++++++++++++++++++++++++++++-
 csum-file.c                   |  5 ++-
 csum-file.h                   |  3 +-
 environment.c                 |  1 +
 midx.c                        |  3 +-
 object-file.c                 |  3 +-
 pack-bitmap-write.c           |  3 +-
 pack-write.c                  | 13 +++---
 read-cache.c                  |  2 +-
 16 files changed, 164 insertions(+), 33 deletions(-)

Documentation/config/core.txt +9 -0

index dbb134f7136..4f1747ec871 100644
@@ -547,6 +547,25 @@ core.whitespace::
   is relevant for `indent-with-non-tab` and when Git fixes `tab-in-indent`
   errors. The default tab width is 8. Allowed values are 1 to 63.
 
+core.fsync::
+     A comma-separated list of parts of the repository which should be
+     hardened via the core.fsyncMethod when created or modified. You can
+     disable hardening of any component by prefixing it with a '-'. Later
+     items take precedence over earlier ones in the list. For example,
+     `core.fsync=all,-pack-metadata` means "harden everything except pack
+     metadata." Items that are not hardened may be lost in the event of an
+     unclean system shutdown.
++

Documentation/config/core.txt core.whitespace:: · line 568

Quoted 10 lines

Documentation/config/core.txt +10 -0

+* `none` disables fsync completely. This must be specified alone.
+* `loose-object` hardens objects added to the repo in loose-object form.
+* `pack` hardens objects added to the repo in packfile form.
+* `pack-metadata` hardens packfile bitmaps and indexes.
+* `commit-graph` hardens the commit graph file.
+* `objects` is an aggregate option that includes `loose-objects`, `pack`,
+  `pack-metadata`, and `commit-graph`.
+* `default` is an aggregate option that is equivalent to `objects,-loose-object`
+* `all` is an aggregate option that syncs all individual components above.
+

It's probably a *bit* more work to set up, but I wonder if this wouldn't be simpler if we just said (and this is partially going against what I noted above):

== BEGIN DOC

core.fsync is a multi-value config variable where each item is a pathspec that'll get matched the same way as 'git-ls-files' et al.

When we sync pretend that a path like .git/objects/de/adbeef... is relative to the top-level of the git
directory. E.g. "objects/de/adbeaf.." or "objects/pack/...".

You can then supply a list of wildcards and exclusions to configure syncing. or "false", "off" etc. to turn it off. These are synonymous with:

; same as "false"
core.fsync = ":!*"

Or:

; same as "true"
core.fsync = "*"

Or, to selectively sync some things and not others:

    ;; Sync objects, but not "info"
    core.fsync = ":!objects/info/**"
    core.fsync = "objects/**"

See gitrepository-layout(5) for details about what sort of paths you might be expected to match. Not all paths listed there will go through this mechanism (e.g. currently objects do, but nothing to do with config does).

We can and will match this against "fake paths", e.g. when writing out packs we may match against just the string "objects/pack", we're not going to re-check if every packfile we're writing matches your globs, ditto for loose objects. Be reasonable!

This metharism is intended as a shorthand that provides some flexibility when fsyncing, while not forcing git to come up with labels for all paths the git dir, or to support crazyness like "objects/de/adbeef*"

More paths may be added or removed in the future, and we make no promises that we won't move things around, so if in doubt use e.g. "true" or a wide pattern match like "objects/**". When in doubt stick to the golden path of examples provided in this documentation.

Quoted 8 lines

== END DOC

It's a tad more complex to set up, but I wonder if that isn't worth it. It nicely gets around any current and future issues of deciding what labels such as "loose-object" etc. to pick, as well as slotting into an existing method of doing exclude/include lists.

Quoted 6 lines

I think this proposal is a lot of complexity to avoid coming up with a new name for syncable things as they are added to Git. A path based mechanism makes it hard to document for the (advanced) user what the full set of things is and how it might change from release to release. I think the current core.fsync scheme is a bit easier to understand, query, and extend.

We document it in gitrepository-layout(5). Yeah it has some disadvantages, but one advantage is that you could make the composability easy. I.e. if last exclude wins then a setting of:

    core.fsync = ":!*"
    core.fsync = "objects/**"

Quoted 2 lines

Would reset all previous matches & only match objects/**.

The value of changing this is predicated on taking your previous multi-valued config proposal, which I'm still not at all convinced about.

The schema in the current (v1-v2) version of the patch already includes an example of extending the list of syncable things, and Patrick Steinhardt made it clear that he feels comfortable adding 'refs' to the same schema in a future change.

I'll also emphasize that we're talking about a non-functional, relatively corner-case behavioral configuration. These values don't change how git's interface behaves except when the system crashes during a git command or shortly after one completes.

While you may not personally love the proposed configuration interface, I'd want your view on some questions:
1. Is it easy for the (advanced) user to set a configuration?
2. Is it easy for the (advanced) user to see what was configured?
3. Is it easy for the Git community to build on this as we want to add things to the list of things to sync?
a) Is there a good best practice configuration so that people can
avoid losing integrity for new stuff that they are intending to sync.
b) If someone has a custom configuration, can that custom
configuration do something reasonable as they upgrade versions of Git?
** In response to this question, I might see some value

in adding a 'derived-metadata' aggregate that can be disabled so that a custom configuration can exclude those as they change version to version.
c) Is it too much maintenance overhead to consider how to present
this configuration knob for any new hashfile or other datafile in the git repo?
4. Is there a good path forward to change the default syncable set, both in git-for-windows and in Git for other platforms?

Quoted 24 lines

Quoted 17 lines

Quoted 17 lines

builtin/pack-objects.c static void write_pack_file(void) · line 1208

Quoted 11 lines

builtin/pack-objects.c +2 -1

index 857be7826f3..916c55d6ce9 100644
@@ -1204,11 +1204,13 @@ static void write_pack_file(void)
               * If so, rewrite it like in fast-import
               */
              if (pack_to_stdout) {
-                     finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE);
+                     finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE,
+                                       CSUM_HASH_IN_STREAM | CSUM_CLOSE);

Not really related to this per-se, but since you're touching the API everything goes through I wonder if callers should just always try to fsync, and we can just catch EROFS and EINVAL in the wrapper if someone tries to flush stdout, or catch the fd at that lower level.

Quoted 6 lines

Quoted 1 line

Or maybe there's a good reason for this...

It's platform dependent, but I'd expect fsync would do something for pipes or stdout redirected to a file. In these cases we really don't want to fsync since we have no idea what we're talking to and we're potentially worsening performance for probably no benefit.

Quoted 8 lines

Yeah maybe we should just leave it be.

I'd think the C library returning EINVAL would be a trivial performance cost though.

It just seemed odd to hardcode assumptions about what can and can't be synced when the POSIX defined function will also tell us that.

Redirecting stdout to a file seems like a common usage for this command. That would definitely be fsyncable, but Git has no idea what its proper category is since there's no way to know the purpose or lifetime of the packfile. I'm going to leave this be, because I'd posit that "can it be fsynced?" is not the same as "should it be fsynced?". The latter question can't be answered for stdout.

Quoted 50 lines

Quoted 49 lines

Quoted 36 lines

builtin/pack-objects.c

Quoted 9 lines

[...]

builtin/pack-objects.c +7 -0

+/*
+ * These values are used to help identify parts of a repository to fsync.
+ * FSYNC_COMPONENT_NONE identifies data that will not be a persistent part of the
+ * repository and so shouldn't be fsynced.
+ */
+enum fsync_component {
+     FSYNC_COMPONENT_NONE                    = 0,

I haven't read ahead much but in most other such cases we don't define the "= 0", just start at 1<<0, then check the flags elsewhere...

Quoted 23 lines

builtin/pack-objects.c +23 -0

+static const struct fsync_component_entry {
+     const char *name;
+     enum fsync_component component_bits;
+} fsync_component_table[] = {
+     { "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
+     { "pack", FSYNC_COMPONENT_PACK },
+     { "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
+     { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
+     { "objects", FSYNC_COMPONENTS_OBJECTS },
+     { "default", FSYNC_COMPONENTS_DEFAULT },
+     { "all", FSYNC_COMPONENTS_ALL },
+};
+
+static enum fsync_component parse_fsync_components(const char *var, const char *string)
+{
+     enum fsync_component output = 0;
+
+     if (!strcmp(string, "none"))
+             return output;
+
+     while (string) {
+             int i;
+             size_t len;

Quoted 8 lines

builtin/pack-objects.c

Quoted 5 lines

builtin/pack-objects.c +5 -0

+             const char *ep;
+             int negated = 0;
+             int found = 0;
+
+             string = string + strspn(string, ", \t\n\r");

Aside from the "use a list" isn't this hardcoding some windows-specific assumptions with \n\r? Maybe not...

I shamelessly stole this code from parse_whitespace_rule. I thought about making a helper to be called by both functions, but the amount of state going into and out of the wrapper via arguments was substantial and seemed to negate the benefit of deduplication.

Quoted 2 lines

FWIW string_list_split() is easier to work with in those cases, or at least I think so...

This code runs at startup for a variable that may be present on some installations. The nice property of the current patch's code is that it's already a well-tested pattern that doesn't do any allocations as it's working, unlike string_list_split().

I hope you know that I appreciate your review feedback, even though I'm pushing back on most of it so far this round. I'll be sending v3 to the list soon after giving it another look over.

Quoted 95 lines · Neeraj Singh

Quoted 94 lines · Ævar Arnfjörð Bjarmason

Quoted 63 lines · Neeraj Singh

Quoted 53 lines · Ævar Arnfjörð Bjarmason

Quoted 10 lines · Neeraj Singh via GitGitGadget

From: Neeraj Singh <[email protected]>

This commit introduces the core.fsync configuration knob which can be used to control how components of the repository are made durable on disk.

This setting allows future extensibility of the list of syncable components:
* We issue a warning rather than an error for unrecognized components, so new configs can be used with old Git versions.

Looks good!

Quoted 4 lines

* We support negation, so users can choose one of the default aggregate options and then remove components that they don't want. The user would then harden any new components added in a Git version update.

I think this config schema makes sense, but just a (I think important) comment on the "how" not "what" of it. It's really much better to define config as:

[some]
key = value
key = value2

Than:

[some]
key = value,value2

The reason is that "git config" has good support for working with multi-valued stuff, so you can do e.g.:

    git config --get-all -z some.key

And you can easily (re)set such config e.g. with --replace-all etc., but for comma-delimited you (and users) need to do all that work themselves.

Similarly instead of:

some.key = want-this
some.key = -not-this
some.key = but-want-this

I think it's better to just have two lists, one inclusive another exclusive. E.g. see "log.decorate" and "log.excludeDecoration", "transfer.hideRefs"

Quoted 8 lines

Which would mean:

core.fsync = want-this
core.fsyncExcludes = -not-this

For some value of "fsyncExcludes", maybe "noFsync"? Anyway, just a suggestion on making this easier for users & the implementation.

Quoted 10 lines

Maybe there's some way to handle this I'm unaware of, but a disadvantage of your multi-valued config proposal is that it's harder, for example, for a per-repo config store to reasonably override a per-user config store. With the configuration scheme as-is, I can have a per-user setting like core.fsync=all which covers my typical repos, but then have a maintainer repo with a private setting of core.fsync=none to speed up cases where I'm mostly working with other people's changes that are backed up in email or server-side repos. The latter setting conveniently overrides the former setting in all aspects.

Even if you turn just your comma-delimited proposal into a list proposal can't we just say that the last one wins? Then it won't matter what cmae before, you'd specify "core.fsync=none" in your local .git/config.

But this is also a general issue with a bunch of things in git's config space. I'd rather see us use the list-based values and just come up with some general way to reset them that works for all keys, rather than regretting having comma-delimited values that'll be harder to work with & parse, which will be a legacy wart if/when we come up with a way to say "reset all previous settings".

Quoted 2 lines

Also, with the core.fsync and core.fsyncExcludes, how would you spell "don't sync anything"? Would you still have the aggregate options.?

core.fsyncExcludes = *

Quoted 7 lines

Quoted 2 lines

I.e. the same as the core.fsync=none above, anyway I still like the wildcard thing below a bit more...

I'm not going to take this feedback unless there are additional votes from the Git community in this direction. I make the claim that single-valued comma-separated config lists are easier to work with in the existing Git infrastructure.

Easier in what sense? I showed examples of how "git-config" trivially works with multi-valued config, but for comma-delimited you'll need to write your own shellscript boilerplate around simple things like adding values, removing existing ones etc.

I.e. you can use --add, --unset, --unset-all, --get, --get-all etc.

Quoted 4 lines

parsing code for the core.whitespace variable and users are used to this syntax there. There are several other comma-separated lists in the config space, so this construct has precedence and will be with Git for some time.

That's not really an argument either way for why we'd pick X over Y for something new. We've got some comma-delimited, some multi-value (I'm fairly sure we have more multi-value).

Quoted 4 lines

Also, fsync configurations aren't composable like
some other configurations may be. It makes sense to have a holistic singular fsync configuration, which is best represented by a single variable.

What's a "variable" here? We call these "keys", you can have a single-value key like user.name that you get with --get, or a multi-value key like say branch.<name>.merge or push.pushOption that you'd get with --get-all.

I think you may be referring to either not wanting these to be "inherited" (which is not really a think we do for anything else in config), or lacking the ability to "reset".

For the latter if that's absolutely needed we could just use the same trick as "diff.wsErrorHighlight" uses of making an empty value "reset" the list, and you'd get better "git config" support for editing it.

Quoted 22 lines

Quoted 14 lines

Quoted 14 lines

Quoted 6 lines

Quoted 3 lines

This also supports the common request of doing absolutely no fysncing with the core.fsync=none value, which is expected to make the test suite faster.

Let's just use the git_parse_maybe_bool() or git_parse_maybe_bool_text() so we'll accept "false", "off", "no" like most other such config?

Junio's previous feedback when discussing batch mode [1] was to offer less flexibility when parsing new values of these configuration options. I agree with his statement that "making machine-readable tokens be spelled in different ways is a 'disease'." I'd like to leave this as-is so that the documentation can clearly state the exact set of allowable values.

Quoted 7 lines

Quoted 1 line

[1] https://lore.kernel.org/git/[email protected]/

I think he's talking about batch, Batch, BATCH, bAtCh etc. there. But the "maybe bool" is a stanard pattern we use.

I don't think we'd call one of these 0, off, no or false etc. to avoid confusion, so then you can use git_parse_maybe_...()

Quoted 6 lines

I don't see the advantage of having multiple ways of specifying "none". The user can read the doc and know exactly what to write. If they write something unallowable, they get a clear warning and they can read the doc again to figure out what to write. This isn't a boolean options at all, so why should we entertain bool-like ways of spelling it?

It's not boolean, it's multi-value and one of the values includes a true or false boolean value. You just spell it "none".

I think both this and your comment above suggest that you think there's no point in this because you haven't interacted with/used "git config" as a command line or API mechanism, but have just hand-crafted config files.

That's fair enough, but there's a lot of tooling that benefits from the latter.

E.g.:

    $ git -c core.fsync=off config --type=bool core.fsync
    false
    $ git -c core.fsync=blah config --type=bool core.fsync
    fatal: bad boolean config value 'blah' for 'core.fsync'

Here we can get 'git config' to normalize what you call 'none', and you can tell via exit codes/normalization if it's "false". But if you invent a new term for "false" you can't do that as easily.

We have various historical keys that take odd exceptions to that, e.g. "never", but unless we have a good reason to let's not invent more exceptions.

Quoted 121 lines

Quoted 121 lines

Quoted 107 lines

Quoted 98 lines

Quoted 37 lines

Signed-off-by Neeraj Singh <[email protected]>

---
 Documentation/config/core.txt | 27 +++++++++----
 builtin/fast-import.c         |  2 +-
 builtin/index-pack.c          |  4 +-
 builtin/pack-objects.c        |  8 ++--
 bulk-checkin.c                |  5 ++-
 cache.h                       | 39 +++++++++++++++++-
 commit-graph.c                |  3 +-
 config.c                      | 76 ++++++++++++++++++++++++++++++++++-
 csum-file.c                   |  5 ++-
 csum-file.h                   |  3 +-
 environment.c                 |  1 +
 midx.c                        |  3 +-
 object-file.c                 |  3 +-
 pack-bitmap-write.c           |  3 +-
 pack-write.c                  | 13 +++---
 read-cache.c                  |  2 +-
 16 files changed, 164 insertions(+), 33 deletions(-)

Documentation/config/core.txt +9 -0

index dbb134f7136..4f1747ec871 100644
@@ -547,6 +547,25 @@ core.whitespace::
   is relevant for `indent-with-non-tab` and when Git fixes `tab-in-indent`
   errors. The default tab width is 8. Allowed values are 1 to 63.
 
+core.fsync::
+     A comma-separated list of parts of the repository which should be
+     hardened via the core.fsyncMethod when created or modified. You can
+     disable hardening of any component by prefixing it with a '-'. Later
+     items take precedence over earlier ones in the list. For example,
+     `core.fsync=all,-pack-metadata` means "harden everything except pack
+     metadata." Items that are not hardened may be lost in the event of an
+     unclean system shutdown.
++

Documentation/config/core.txt core.whitespace:: · line 568

Quoted 10 lines

Documentation/config/core.txt +10 -0

+* `none` disables fsync completely. This must be specified alone.
+* `loose-object` hardens objects added to the repo in loose-object form.
+* `pack` hardens objects added to the repo in packfile form.
+* `pack-metadata` hardens packfile bitmaps and indexes.
+* `commit-graph` hardens the commit graph file.
+* `objects` is an aggregate option that includes `loose-objects`, `pack`,
+  `pack-metadata`, and `commit-graph`.
+* `default` is an aggregate option that is equivalent to `objects,-loose-object`
+* `all` is an aggregate option that syncs all individual components above.
+

It's probably a *bit* more work to set up, but I wonder if this wouldn't be simpler if we just said (and this is partially going against what I noted above):

== BEGIN DOC

core.fsync is a multi-value config variable where each item is a pathspec that'll get matched the same way as 'git-ls-files' et al.

When we sync pretend that a path like .git/objects/de/adbeef... is relative to the top-level of the git
directory. E.g. "objects/de/adbeaf.." or "objects/pack/...".

You can then supply a list of wildcards and exclusions to configure syncing. or "false", "off" etc. to turn it off. These are synonymous with:

; same as "false"
core.fsync = ":!*"

Or:

; same as "true"
core.fsync = "*"

Or, to selectively sync some things and not others:

    ;; Sync objects, but not "info"
    core.fsync = ":!objects/info/**"
    core.fsync = "objects/**"

See gitrepository-layout(5) for details about what sort of paths you might be expected to match. Not all paths listed there will go through this mechanism (e.g. currently objects do, but nothing to do with config does).

We can and will match this against "fake paths", e.g. when writing out packs we may match against just the string "objects/pack", we're not going to re-check if every packfile we're writing matches your globs, ditto for loose objects. Be reasonable!

This metharism is intended as a shorthand that provides some flexibility when fsyncing, while not forcing git to come up with labels for all paths the git dir, or to support crazyness like "objects/de/adbeef*"

More paths may be added or removed in the future, and we make no promises that we won't move things around, so if in doubt use e.g. "true" or a wide pattern match like "objects/**". When in doubt stick to the golden path of examples provided in this documentation.

Quoted 8 lines

== END DOC

It's a tad more complex to set up, but I wonder if that isn't worth it. It nicely gets around any current and future issues of deciding what labels such as "loose-object" etc. to pick, as well as slotting into an existing method of doing exclude/include lists.

Quoted 6 lines

I think this proposal is a lot of complexity to avoid coming up with a new name for syncable things as they are added to Git. A path based mechanism makes it hard to document for the (advanced) user what the full set of things is and how it might change from release to release. I think the current core.fsync scheme is a bit easier to understand, query, and extend.

We document it in gitrepository-layout(5). Yeah it has some disadvantages, but one advantage is that you could make the composability easy. I.e. if last exclude wins then a setting of:

    core.fsync = ":!*"
    core.fsync = "objects/**"

Quoted 6 lines

Quoted 2 lines

Would reset all previous matches & only match objects/**.

The value of changing this is predicated on taking your previous multi-valued config proposal, which I'm still not at all convinced about.

They're orthagonal. I.e. you get benefits from multi-value with or without this globbing mechanism.

In any case, I don't feel strongly about/am really advocating this globbing mechanism. I just wondered if it wouldn't make things simpler since it would sidestep the need to create any sort of categories for subsets of gitrepository-layout(5), but maybe not...

Quoted 9 lines

The schema in the current (v1-v2) version of the patch already includes an example of extending the list of syncable things, and Patrick Steinhardt made it clear that he feels comfortable adding 'refs' to the same schema in a future change.

I'll also emphasize that we're talking about a non-functional, relatively corner-case behavioral configuration. These values don't change how git's interface behaves except when the system crashes during a git command or shortly after one completes.

That may be something some OS's promise, but it's not something fsync() or POSIX promises. I.e. you might write a ref, but unless you fsync and the relevant dir entries another process might not see the update, crash or not.

That's an aside from these config design questions, and I think most/(all?) OS's anyone cares about these days tend to make that implicit promise as part of their VFS behavior, but we should probably design such an interface to fsync() with such pedantic portability in mind.

Quoted 11 lines

While you may not personally love the proposed configuration interface, I'd want your view on some questions:
1. Is it easy for the (advanced) user to set a configuration?
2. Is it easy for the (advanced) user to see what was configured?
3. Is it easy for the Git community to build on this as we want to add things to the list of things to sync?
a) Is there a good best practice configuration so that people can
avoid losing integrity for new stuff that they are intending to sync.
b) If someone has a custom configuration, can that custom
configuration do something reasonable as they upgrade versions of Git?
** In response to this question, I might see some value

Quoted 8 lines

in adding a 'derived-metadata' aggregate that can be disabled so that a custom configuration can exclude those as they change version to version.
c) Is it too much maintenance overhead to consider how to present
this configuration knob for any new hashfile or other datafile in the git repo?
4. Is there a good path forward to change the default syncable set, both in git-for-windows and in Git for other platforms?

I'm not really sure this globbing this is a good idea, as noted above just a suggestion etc.

As noted there it just gets you out of the business of re-defining gitrepository-layout(5), and assuming too much in advance about certain use-cases.

E.g. even "refs" might be too broad for some. I don't tend to be I/O limited, but I could see how someone who would be would care about refs/heads but not refs/remotes, or want to exclude logs/* but not the refs updates themselves etc.

Quoted 33 lines

Quoted 24 lines

Quoted 17 lines

Quoted 17 lines

builtin/pack-objects.c static void write_pack_file(void) · line 1208

Quoted 11 lines

builtin/pack-objects.c +2 -1

index 857be7826f3..916c55d6ce9 100644
@@ -1204,11 +1204,13 @@ static void write_pack_file(void)
               * If so, rewrite it like in fast-import
               */
              if (pack_to_stdout) {
-                     finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE);
+                     finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE,
+                                       CSUM_HASH_IN_STREAM | CSUM_CLOSE);

Not really related to this per-se, but since you're touching the API everything goes through I wonder if callers should just always try to fsync, and we can just catch EROFS and EINVAL in the wrapper if someone tries to flush stdout, or catch the fd at that lower level.

Quoted 6 lines

Quoted 1 line

Or maybe there's a good reason for this...

It's platform dependent, but I'd expect fsync would do something for pipes or stdout redirected to a file. In these cases we really don't want to fsync since we have no idea what we're talking to and we're potentially worsening performance for probably no benefit.

Quoted 8 lines

Yeah maybe we should just leave it be.

I'd think the C library returning EINVAL would be a trivial performance cost though.

It just seemed odd to hardcode assumptions about what can and can't be synced when the POSIX defined function will also tell us that.

Quoted 6 lines

Redirecting stdout to a file seems like a common usage for this command. That would definitely be fsyncable, but Git has no idea what its proper category is since there's no way to know the purpose or lifetime of the packfile. I'm going to leave this be, because I'd posit that "can it be fsynced?" is not the same as "should it be fsynced?". The latter question can't be answered for stdout.

As noted this was just an aside, and I don't even know if any OS would do anything meaningful with an fsync() of such a FD anyway.

I just don't see why we wouldn't say:

1. We're syncing this category of thing
2. Try it
3. If fsync returns "can't fsync that sort of thing" move on

As opposed to trying to shortcut #3 by doing the detection ourselves.

I.e. maybe there was a good reason, but it seemed to be some easy potential win for more simplification, since you were re-doing and simplifying some of the interface anyway...

Quoted 50 lines

Quoted 50 lines

Quoted 49 lines

Quoted 36 lines

builtin/pack-objects.c

Quoted 9 lines

[...]

builtin/pack-objects.c +7 -0

+/*
+ * These values are used to help identify parts of a repository to fsync.
+ * FSYNC_COMPONENT_NONE identifies data that will not be a persistent part of the
+ * repository and so shouldn't be fsynced.
+ */
+enum fsync_component {
+     FSYNC_COMPONENT_NONE                    = 0,

I haven't read ahead much but in most other such cases we don't define the "= 0", just start at 1<<0, then check the flags elsewhere...

Quoted 23 lines

builtin/pack-objects.c +23 -0

+static const struct fsync_component_entry {
+     const char *name;
+     enum fsync_component component_bits;
+} fsync_component_table[] = {
+     { "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
+     { "pack", FSYNC_COMPONENT_PACK },
+     { "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
+     { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
+     { "objects", FSYNC_COMPONENTS_OBJECTS },
+     { "default", FSYNC_COMPONENTS_DEFAULT },
+     { "all", FSYNC_COMPONENTS_ALL },
+};
+
+static enum fsync_component parse_fsync_components(const char *var, const char *string)
+{
+     enum fsync_component output = 0;
+
+     if (!strcmp(string, "none"))
+             return output;
+
+     while (string) {
+             int i;
+             size_t len;

Quoted 8 lines

builtin/pack-objects.c

Quoted 5 lines

builtin/pack-objects.c +5 -0

+             const char *ep;
+             int negated = 0;
+             int found = 0;
+
+             string = string + strspn(string, ", \t\n\r");

Aside from the "use a list" isn't this hardcoding some windows-specific assumptions with \n\r? Maybe not...

I shamelessly stole this code from parse_whitespace_rule. I thought about making a helper to be called by both functions, but the amount of state going into and out of the wrapper via arguments was substantial and seemed to negate the benefit of deduplication.

Quoted 7 lines

Quoted 2 lines

FWIW string_list_split() is easier to work with in those cases, or at least I think so...

This code runs at startup for a variable that may be present on some installations. The nice property of the current patch's code is that it's already a well-tested pattern that doesn't do any allocations as it's working, unlike string_list_split().

Multi-value config would also get you fewer allocations :)

Anyway, I mainly meant to point out that for stuff like this it's fine to optimize it for ease rather than micro-optimize allocations. Those really aren't a bottleneck on this scale.

Even in that case there's string_list_split_in_place(), which can be a bit nicer than manual C-string fiddling.

Quoted 3 lines

I hope you know that I appreciate your review feedback, even though I'm pushing back on most of it so far this round. I'll be sending v3 to the list soon after giving it another look over.

Sure, no worries. Just hoping to help. If you go for something different etc. that's fine. Just hoping to bridge the gap in some knowledge / offer potentially interesting suggestions (some of which may be dead ends, like the config glob thing...).