Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

A design for future-proofing fsync() configuration #1093

Closed
wants to merge 6 commits into from
Closed

A design for future-proofing fsync() configuration #1093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
825079b
wrapper: make inclusion of Windows csprng header tightly scoped
neerajsi-msft Mar 9, 2022
a41bd4a
core.fsyncmethod: add writeout-only mode
neerajsi-msft Dec 3, 2021
64e2bdc
core.fsync: introduce granular fsync control infrastructure
neerajsi-msft Dec 4, 2021
6adc8dc
core.fsync: add configuration parsing
neerajsi-msft Mar 10, 2022
757f6d0
core.fsync: new option to harden the index
neerajsi-msft Dec 6, 2021
67d21ed
core.fsync: documentation and user-friendly aggregate options
neerajsi-msft Dec 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 54 additions & 4 deletions Documentation/config/core.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,13 +547,63 @@ core.whitespace::
is relevant for `indent-with-non-tab` and when Git fixes `tab-in-indent`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):


--FRW3ky2b/hqIXfuW
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Dec 07, 2021 at 02:46:49AM +0000, Neeraj Singh via GitGitGadget wro=
te:
> From: Neeraj Singh <neerajsi@microsoft.com>
[snip]
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -329,6 +329,9 @@ int mingw_getpagesize(void);
>  #define getpagesize mingw_getpagesize
>  #endif
> =20
> +int win32_fsync_no_flush(int fd);
> +#define fsync_no_flush win32_fsync_no_flush
> +
>  struct rlimit {
>  	unsigned int rlim_cur;
>  };
> diff --git a/compat/win32/flush.c b/compat/win32/flush.c
> new file mode 100644
> index 00000000000..75324c24ee7
> --- /dev/null
> +++ b/compat/win32/flush.c
> @@ -0,0 +1,28 @@
> +#include "../../git-compat-util.h"
> +#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 ParameterSiz=
e,
> +			 PIO_STATUS_BLOCK IoStatusBlock);
> +
> +       if (!INIT_PROC_ADDR(NtFlushBuffersFileEx)) {
> +		errno =3D 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?

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

[snip]
> diff --git a/wrapper.c b/wrapper.c
> index 36e12119d76..1c5f2c87791 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -546,6 +546,62 @@ int xmkstemp_mode(char *filename_template, int mode)
>  	return fd;
>  }
> =20
> +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?

Patrick

> +#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 tha=
t 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 =3D 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 =3D fcntl(fd, F_FULLFSYNC);
> +#else
> +			err =3D fsync(fd);
> +#endif
> +			if (err >=3D 0 || errno !=3D 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;
> diff --git a/write-or-die.c b/write-or-die.c
> index 0b1ec8190b6..0702acdd5e8 100644
> --- a/write-or-die.c
> +++ b/write-or-die.c
> @@ -57,10 +57,12 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)
> =20
>  void fsync_or_die(int fd, const char *msg)
>  {
> -	while (fsync(fd) < 0) {
> -		if (errno !=3D EINTR)
> -			die_errno("fsync error on '%s'", msg);
> -	}
> +	if (fsync_method =3D=3D FSYNC_METHOD_WRITEOUT_ONLY &&
> +	    git_fsync(fd, FSYNC_WRITEOUT_ONLY) >=3D 0)
> +		return;
> +
> +	if (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0)
> +		die_errno("fsync error on '%s'", msg);
>  }
> =20
>  void write_or_die(int fd, const void *buf, size_t count)
> --=20
> gitgitgadget
>=20

--FRW3ky2b/hqIXfuW
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAmGvSSYACgkQVbJhu7ck
PpSusRAAkSCjo70fipT9enLRntvP2bsxSqddv23zIdvhQMzsThePBBlvA8+6lK6H
VT6LHKUpeSeknLmMNOOF1+JX38aKOqn1OGFTviNchPDrsJYxM6wRUGrMoEpD1yOJ
fol0XQ74XC9v5eW81Fc4XEc6rsXMfByRT5Iyp3e5mp+plekzspUh4+i4v/FdrE4e
eY7dRZOD009h2sxXqlHQVUjPZ1O2eIUGKjdHX1T1T/4/ctuv2zkOR1dHpAChNKRe
KxKjTDWiR0sLZjrvwpuDu7Wyo+z5bDg3t8NFthZKYmmvf+R40GOOlXN8eVDT6kiU
zF6h3L4+sNCJl13KWKIaHLSVech4Xst0c4cEfVyE9Z7d7MDBRuRBBJbEEm1znLJm
54LfkAFIvGArfUFSyecwgL+4k7PAncrn3uIR5Qf0qSgIsGdYI8cbnl8N3w9zOdNM
GMH6VafsDwPQG5+jGt4KpQIOTq6KsoYaTkyntvCTkgRNyg0FneEj/HRFnwuGx8h4
P7q7VMhgD3Q7YFxLWyYNQMxyCgPdMNhhcw83CfTMgubcGCHr3Ubw0xYrFIt7wtfV
RDmrtrCculP94prD4tJwpvkkvbtVXBYIQDu7lDgk50yMvKSYfZu0FqelaiuMwKzX
u6RTZ56FHEYYeuuJoPJk4zulJOpSiu50ETYUsloS15kSrYfxpMQ=
=oHAu
-----END PGP SIGNATURE-----

--FRW3ky2b/hqIXfuW--

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


On Tue, Dec 07 2021, Patrick Steinhardt wrote:

> [[PGP Signed Part:Undecided]]
> On Tue, Dec 07, 2021 at 02:46:49AM +0000, Neeraj Singh via GitGitGadget wrote:
>> From: Neeraj Singh <neerajsi@microsoft.com>
> [...]
> [snip]
>> diff --git a/wrapper.c b/wrapper.c
>> index 36e12119d76..1c5f2c87791 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -546,6 +546,62 @@ int xmkstemp_mode(char *filename_template, int mode)
>>  	return fd;
>>  }
>>  
>> +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?

I think so per cccdfd22436 (fsync(): be prepared to see EINTR,
2021-06-04), but I'm not sure, but in any case it would make sense for
this to just call the same loop we've been calling elsewhere. It doesn't
seem to hurt, so we can just do that part in the portable portion of the
code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Tue, Dec 7, 2021 at 3:45 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Dec 07, 2021 at 02:46:49AM +0000, Neeraj Singh via GitGitGadget wrote:
> > From: Neeraj Singh <neerajsi@microsoft.com>
> [snip]
> > --- a/compat/mingw.h
> > +++ b/compat/mingw.h
> > @@ -329,6 +329,9 @@ int mingw_getpagesize(void);
> >  #define getpagesize mingw_getpagesize
> >  #endif
> >
> > +int win32_fsync_no_flush(int fd);
> > +#define fsync_no_flush win32_fsync_no_flush
> > +
> >  struct rlimit {
> >       unsigned int rlim_cur;
> >  };
> > diff --git a/compat/win32/flush.c b/compat/win32/flush.c
> > new file mode 100644
> > index 00000000000..75324c24ee7
> > --- /dev/null
> > +++ b/compat/win32/flush.c
> > @@ -0,0 +1,28 @@
> > +#include "../../git-compat-util.h"
> > +#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)) {
> > +             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?
>

I wanted to leave the fallback to the caller so that the algorithm can
be adjusted in some way based on whether writeout-only succeeded.  For
batched fsync object files, we refrain from doing the last fsync if we
were doing real fsyncs all along.

I didn't want to issue a warning, since this writeout-only codepath
might be invoked from multiple subprocesses, which would each
potentially issue their one warning.  The consequence of failing
writeout only is worse performance, but should not be compromised
safety, so I'm not sure the user gets enough from the warning to
justify something that's potentially spammy.  In practice, when batch
mode is adopted on Windows (by default), some older pre-Win8 systems
will experience fsyncs and equivalent performance to what they're
seeing today. I don't want these users to have a warning too.

> > +       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]
> > diff --git a/wrapper.c b/wrapper.c
> > index 36e12119d76..1c5f2c87791 100644
> > --- a/wrapper.c
> > +++ b/wrapper.c
> > @@ -546,6 +546,62 @@ int xmkstemp_mode(char *filename_template, int mode)
> >       return fd;
> >  }
> >
> > +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?
>

The EINTR check was added due to a test failure on HP NonStop.  I
don't believe any other platform has actually complained about that.

Thanks again for the code review!
-Neeraj

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):


--rj+/7kVG+u/4iNlT
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Dec 07, 2021 at 02:46:50AM +0000, Neeraj Singh via GitGitGadget wro=
te:
> From: Neeraj Singh <neerajsi@microsoft.com>
[snip]
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index c23d01de7dc..c32534c13b4 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -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, cons=
t 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 =3D close(output_fd);
>  		if (err)
>  			die_errno(_("error while closing pack file"));
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 857be7826f3..916c55d6ce9 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -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);

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]
> diff --git a/config.c b/config.c
> index c3410b8a868..29c867aab03 100644
> --- a/config.c
> +++ b/config.c
> @@ -1213,6 +1213,73 @@ static int git_parse_maybe_bool_text(const char *v=
alue)
>  	return -1;
>  }
> =20
> +static const struct fsync_component_entry {
> +	const char *name;
> +	enum fsync_component component_bits;
> +} fsync_component_table[] =3D {
> +	{ "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, cons=
t char *string)
> +{
> +	enum fsync_component output =3D 0;
> +
> +	if (!strcmp(string, "none"))
> +		return output;
> +
> +	while (string) {
> +		int i;
> +		size_t len;
> +		const char *ep;
> +		int negated =3D 0;
> +		int found =3D 0;
> +
> +		string =3D string + strspn(string, ", \t\n\r");
> +		ep =3D strchrnul(string, ',');
> +		len =3D ep - string;
> +
> +		if (*string =3D=3D '-') {
> +			negated =3D 1;
> +			string++;
> +			len--;
> +			if (!len)
> +				warning(_("invalid value for variable %s"), var);
> +		}
> +
> +		if (!len)
> +			break;
> +
> +		for (i =3D 0; i < ARRAY_SIZE(fsync_component_table); ++i) {
> +			const struct fsync_component_entry *entry =3D &fsync_component_table[=
i];
> +
> +			if (strncmp(entry->name, string, len))
> +				continue;
> +
> +			found =3D 1;
> +			if (negated)
> +				output &=3D ~entry->component_bits;
> +			else
> +				output |=3D entry->component_bits;
> +		}
> +
> +		if (!found) {
> +			char *component =3D xstrndup(string, len);
> +			warning(_("unknown %s value '%s'"), var, component);
> +			free(component);
> +		}
> +
> +		string =3D ep;
> +	}
> +
> +	return output;
> +}
> +
>  int git_parse_maybe_bool(const char *value)
>  {
>  	int v =3D 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;
>  	}
> =20
> +	if (!strcmp(var, "core.fsync")) {
> +		if (!value)
> +			return config_error_nonbool(var);
> +		fsync_components =3D parse_fsync_components(var, value);
> +		return 0;
> +	}
> +
>  	if (!strcmp(var, "core.fsyncmethod")) {
>  		if (!value)
>  			return config_error_nonbool(var);
> @@ -1503,7 +1577,7 @@ static int git_default_core_config(const char *var,=
 const char *value, void *cb)
>  	}
> =20
>  	if (!strcmp(var, "core.fsyncobjectfiles")) {
> -		fsync_object_files =3D 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

> diff --git a/csum-file.c b/csum-file.c
> index 26e8a6df44e..59ef3398ca2 100644
> --- a/csum-file.c
> +++ b/csum-file.c
> @@ -58,7 +58,8 @@ static void free_hashfile(struct hashfile *f)
>  	free(f);
>  }
> =20
> -int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigne=
d int flags)
> +int finalize_hashfile(struct hashfile *f, unsigned char *result,
> +		      enum fsync_component component, unsigned int flags)
>  {
>  	int fd;
> =20
> @@ -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);
> diff --git a/csum-file.h b/csum-file.h
> index 291215b34eb..0d29f528fbc 100644
> --- a/csum-file.h
> +++ b/csum-file.h
> @@ -1,6 +1,7 @@
>  #ifndef CSUM_FILE_H
>  #define CSUM_FILE_H
> =20
> +#include "cache.h"
>  #include "hash.h"
> =20
>  struct progress;
> @@ -38,7 +39,7 @@ int hashfile_truncate(struct hashfile *, struct hashfil=
e_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 prog=
ress *tp);
> -int finalize_hashfile(struct hashfile *, unsigned char *, unsigned int);
> +int finalize_hashfile(struct hashfile *, unsigned char *, enum fsync_com=
ponent, unsigned int);
>  void hashwrite(struct hashfile *, const void *, unsigned int);
>  void hashflush(struct hashfile *f);
>  void crc32_begin(struct hashfile *);
> diff --git a/environment.c b/environment.c
> index f9140e842cf..09905adecf9 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -42,6 +42,7 @@ const char *git_hooks_path;
>  int zlib_compression_level =3D Z_BEST_SPEED;
>  int pack_compression_level =3D Z_DEFAULT_COMPRESSION;
>  enum fsync_method fsync_method =3D FSYNC_METHOD_DEFAULT;
> +enum fsync_component fsync_components =3D FSYNC_COMPONENTS_DEFAULT;
>  size_t packed_git_window_size =3D DEFAULT_PACKED_GIT_WINDOW_SIZE;
>  size_t packed_git_limit =3D DEFAULT_PACKED_GIT_LIMIT;
>  size_t delta_base_cache_limit =3D 96 * 1024 * 1024;
> diff --git a/midx.c b/midx.c
> index 837b46b2af5..882f91f7d57 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -1406,7 +1406,8 @@ static int write_midx_internal(const char *object_d=
ir,
>  	write_midx_header(f, get_num_chunks(cf), ctx.nr - dropped_packs);
>  	write_chunkfile(cf, &ctx);
> =20
> -	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);
> =20
>  	if (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))
> diff --git a/object-file.c b/object-file.c
> index eb972cdccd2..9d9c4a39e85 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1809,8 +1809,7 @@ int hash_object_file(const struct git_hash_algo *al=
go, 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) !=3D 0)
>  		die_errno(_("error when closing loose object file"));
>  }
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index 9c55c1531e1..c16e43d1669 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -719,7 +719,8 @@ void bitmap_writer_finish(struct pack_idx_entry **ind=
ex,
>  	if (options & BITMAP_OPT_HASH_CACHE)
>  		write_hash_cache(f, index, index_nr);
> =20
> -	finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOS=
E);
> +	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
> +			  CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
> =20
>  	if (adjust_shared_perm(tmp_file.buf))
>  		die_errno("unable to make temporary bitmap file readable");
> diff --git a/pack-write.c b/pack-write.c
> index a5846f3a346..51812cb1299 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -159,9 +159,9 @@ const char *write_idx_file(const char *index_name, st=
ruct pack_idx_entry **objec
>  	}
> =20
>  	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;
>  }
> =20
> @@ -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);
> =20
> -	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));
> =20
>  	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);
>  }
> =20
>  char *index_pack_lockfile(int ip_out, int *is_well_formed)
> diff --git a/read-cache.c b/read-cache.c
> index f3986596623..f3539681f49 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -3060,7 +3060,7 @@ static int do_write_index(struct index_state *istat=
e, struct tempfile *tempfile,
>  			return -1;
>  	}
> =20
> -	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;
> --=20
> gitgitgadget
>=20

--rj+/7kVG+u/4iNlT
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAmGvSzsACgkQVbJhu7ck
PpQzlg/9H/anSyKJQnHnBpnzSJ3FvmehwLWUbeM96Sk/zcA2q0A+Xb0nKe9vvBcY
LlGV/rxcAUuwZMlJ9pm88Q92EI3fUNUzqJP5lyaW48hUQsA91vSx9tZE2FiaDryp
V4gMzkc6tOhqu739HjSIBYkJ2XW+iabcpITNLodeKl0nFSKJ8rDTK3m69O0NEB4z
pGZs/agFpkI8FPBS/FKioVpmAOe/eM2mcWLGfL4gy2NjtO4FJsFNuP/sKtIxak6Q
0eaRgFcHDknkrvmgtmSCikZtCcuG58CnqcfVy1t1j2E8VwW3bvyrawBD7vWcFRb/
XALqryMaF3+8Rq/yhp7gCtfPqU18xSo/8/t6rjclBL0ldmYaBqXBLA8Ci9TXdDkh
QKlp28wMs4EnhthqXCjm/iUk+0izrDfPWbHPHnGhwiVqHP83fVF4+cNNbrYk6tM5
XaCrR5USoAjQ5IAlpOeuTr5tT2qVHBf5opiX69Ld+riIczFPrBFavV0DP32HMuPM
VOVBcveLBB+LVCR5a6JKMUxJXvypwNN61YOli7fSLhIzzW+voqIlu9q1VbdFZkkS
kscXsSTnKf+tGTEBZi2q5+SfpQFz4rfhAr710W+Zlbp5KZBgDR9v2qOKdnPfMYi4
qx1EfL4k9OmE9SLuvHMfWBiIiYdzJpUWMQ2oVO7hsfOTMKuYDvo=
=xPhw
-----END PGP SIGNATURE-----

--rj+/7kVG+u/4iNlT--

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Tue, Dec 7, 2021 at 3:54 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Dec 07, 2021 at 02:46:50AM +0000, Neeraj Singh via GitGitGadget wrote:
> > From: Neeraj Singh <neerajsi@microsoft.com>
> [snip]
> > diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> > index c23d01de7dc..c32534c13b4 100644
> > --- a/builtin/index-pack.c
> > +++ b/builtin/index-pack.c
> > @@ -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"));
> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index 857be7826f3..916c55d6ce9 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> > @@ -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);
>
> 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.
>

If it's not actually a file with some name known to git, is it really
a component of the repository? I'd like to leave this one as-is.

> [snip]
> > diff --git a/config.c b/config.c
> > index c3410b8a868..29c867aab03 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -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);
> > @@ -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
>

That's a good question and one I was hoping to answer through this
discussion.  I'm guessing that most users do not have this setting
explicitly set, and it's largely a non-functional change. Git only
behaves differently in the extreme corner case of a system crash after
git exits.  That's why I believe it's okay to deprecate and remove in
one release.

If we choose to keep supporting the setting, it would introduce a
little complexity in the configuration code, but it should be doable.
I think the right semantics would be to ignore core.fsyncobjectfiles
if core.fsync is specified with any value. Otherwise,
core.fsyncobjectfiles would be equivalent to `default,-loose-object`
or `default,loose-object` for `false` and `true` respectively. I'd
prefer not to do this if I can get away with it :).

Thanks,
Neeraj

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


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

> From: Neeraj Singh <neerajsi@microsoft.com>
>
> This commit introduces the `core.fsyncmethod` configuration

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

> diff --git a/compat/win32/flush.c b/compat/win32/flush.c
> new file mode 100644
> index 00000000000..75324c24ee7
> --- /dev/null
> +++ b/compat/win32/flush.c
> @@ -0,0 +1,28 @@
> +#include "../../git-compat-util.h"

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

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

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

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

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

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

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Tue, Dec 7, 2021 at 4:27 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> On Tue, Dec 07 2021, Neeraj Singh via GitGitGadget wrote:
>
> > From: Neeraj Singh <neerajsi@microsoft.com>
> >
> > This commit introduces the `core.fsyncmethod` configuration
>
> Just a commit msg nit: core.fsyncMethod (I see the docs etc. are using
> it camelCased, good..

Will fix.

> > diff --git a/compat/win32/flush.c b/compat/win32/flush.c
> > new file mode 100644
> > index 00000000000..75324c24ee7
> > --- /dev/null
> > +++ b/compat/win32/flush.c
> > @@ -0,0 +1,28 @@
> > +#include "../../git-compat-util.h"
>
> nit: Just FWIW I think the better thing is '#include
> "git-compat-util.h"', i.e. we're compiling at the top-level and have
> added it to -I.
>
> (I know a lot of compat/ and contrib/ and even main-tree stuff does
> that, but just FWIW it's not needed).
>

Will fix.

> > +     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.
>
> > +                     warning(_("unknown %s value '%s'"), var, value);
>
> Just a suggestion maybe something slightly scarier like:
>
>     "unknown core.fsyncMethod value '%s'; config from future git version? ignoring requested fsync strategy"
>
> Also using the nicer camelCased version instead of "var" (also helps
> translators with context...)
>

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

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

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

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

> > +     }
> > +}
> > +
> >  static int warn_if_unremovable(const char *op, const char *file, int rc)
>
> Just a code nit: I think it's very much preferred if possible to have as
> much of code like this compile on all platforms. See the series at
> 4002e87cb25 (grep: remove #ifdef NO_PTHREADS, 2018-11-03) is part of for
> a good example.
>
> Maybe not worth it in this case since they're not nested ifdef's.
>
> I'm basically thinking of something (also re Patrick's comment on the
> 2nd patch) where we have a platform_fsync() whose return
> value/arguments/whatever capture this "I want to return now" or "you
> should be looping" and takes the enum_fsync_action" strategy.
>
> Then the git_fsync() would be the platform-independent looping etc., and
> another funciton would do the "one fsync at a time, maybe call me
> again".
>
> Maybe it would suck more, just food for thought... :)

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

Thanks,
Neeraj

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


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

> From: Neeraj Singh <neerajsi@microsoft.com>
>
> 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!

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

> 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?

> Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
> ---
>  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(-)
>
> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> index dbb134f7136..4f1747ec871 100644
> --- a/Documentation/config/core.txt
> +++ b/Documentation/config/core.txt
> @@ -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.
> ++
> +* `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.

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 857be7826f3..916c55d6ce9 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -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...

> [...]
> +/*
> + * 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...

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

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Tue, Dec 7, 2021 at 5:01 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> On Tue, Dec 07 2021, Neeraj Singh via GitGitGadget wrote:
>
> > From: Neeraj Singh <neerajsi@microsoft.com>
> >
> > 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!
>
> > * 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.
>

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

> > 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/xmqqr1dqzyl7.fsf@gitster.g/

>
> > Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
> > ---
> >  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(-)
> >
> > diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> > index dbb134f7136..4f1747ec871 100644
> > --- a/Documentation/config/core.txt
> > +++ b/Documentation/config/core.txt
> > @@ -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.
> > ++
> > +* `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.
>

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.

> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index 857be7826f3..916c55d6ce9 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> > @@ -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...

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.

> > [...]
> > +/*
> > + * 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...
>
> > +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");
>
> 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.

Thanks for the review,
Neeraj

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


On Tue, Dec 07 2021, Neeraj Singh wrote:

> On Tue, Dec 7, 2021 at 5:01 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>>
>> On Tue, Dec 07 2021, Neeraj Singh via GitGitGadget wrote:
>>
>> > From: Neeraj Singh <neerajsi@microsoft.com>
>> >
>> > 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!
>>
>> > * 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.
>>
>
> 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".

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

>> > 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/xmqqr1dqzyl7.fsf@gitster.g/

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_...()

>>
>> > Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
>> > ---
>> >  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(-)
>> >
>> > diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
>> > index dbb134f7136..4f1747ec871 100644
>> > --- a/Documentation/config/core.txt
>> > +++ b/Documentation/config/core.txt
>> > @@ -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.
>> > ++
>> > +* `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.
>>
>
> 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/**.

>> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>> > index 857be7826f3..916c55d6ce9 100644
>> > --- a/builtin/pack-objects.c
>> > +++ b/builtin/pack-objects.c
>> > @@ -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...
>
> 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.

Anyway...

>> > [...]
>> > +/*
>> > + * 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...
>>
>> > +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");
>>
>> 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.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Wed, Dec 8, 2021 at 2:17 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> On Tue, Dec 07 2021, Neeraj Singh wrote:
>
> > On Tue, Dec 7, 2021 at 5:01 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >>
> >>
> >> On Tue, Dec 07 2021, Neeraj Singh via GitGitGadget wrote:
> >>
> >> > From: Neeraj Singh <neerajsi@microsoft.com>
> >> >
> >> > 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!
> >>
> >> > * 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.
> >>
> >
> > 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".
>
> > 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...

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.

> >> > 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/xmqqr1dqzyl7.fsf@gitster.g/
>
> 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?

> >>
> >> > Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
> >> > ---
> >> >  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(-)
> >> >
> >> > diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> >> > index dbb134f7136..4f1747ec871 100644
> >> > --- a/Documentation/config/core.txt
> >> > +++ b/Documentation/config/core.txt
> >> > @@ -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.
> >> > ++
> >> > +* `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.
> >>
> >
> > 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/**.
>

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?

> >> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> >> > index 857be7826f3..916c55d6ce9 100644
> >> > --- a/builtin/pack-objects.c
> >> > +++ b/builtin/pack-objects.c
> >> > @@ -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...
> >
> > 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.
>

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.

>
> >> > [...]
> >> > +/*
> >> > + * 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...
> >>
> >> > +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");
> >>
> >> 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.
>
> 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.

Thanks,
Neeraj

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Neeraj Singh <nksingh85@gmail.com> writes:

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

I haven't caught up with the discussion in this thread, even though
I have been meaning to think about it for some time---I just haven't
got around to it (sorry).  So I'll stop at giving a general guidance
and leave the decision if it applies to this particular discussion
to readers.

As the inventor of core.whitespace "list of values and its parser, I
am biased, but I would say that it works well for simple things that
do not need too much overriding.  The other side of the coin is that
it can become very awkward going forward if we use it to things that
have more complex needs than answering a simple question like "what
whitespace errors should be checked?".

More specifically, core.whitespace is pecuriar in a few ways.

 * It does follow the usual "the last one wins" rule, but in a
   strange way.  Notice the "unsigned rule = WS_DEFAULT_RULE"
   assignment at the beginning of ws.c::parse_whitespace_rule()?
   For each configuration "core.whitespace=<list>" we encounter,
   we start from the default, discarding everything we saw so far,
   and tweak that default value with tokens found on the list.

 * You cannot do "The system config gives one set of values, which
   you tweak with the personal config, which is further tweaked with
   the repository config" as the consequence.  This is blessing and
   is curse at the same time, as it makes inspection simpler when
   things go wrong (you only need to check whta the last one does),
   but it is harder to share the common things in more common file.

 * Its design relies on the choices being strings chosen from a
   fixed vocabulary to allow you to say "the value of the
   configuration variable is a list of tokens separated by a comma"
   and "the default has X bit set, but we disable that with -X".
   For a configuration variable whose value is an arbitrary string
   or a number, obviously that approach would not work.

If the need of the topic is simple enough that the above limitation
core.whitespace does not pose a problem going forward, it would be
fine, but we may regret the choice we make today if that is not the
case.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Wed, Dec 8, 2021 at 8:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> On Wed, Dec 08 2021, Neeraj Singh wrote:
>
> > On Wed, Dec 8, 2021 at 2:17 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >>
> >>
> >> On Tue, Dec 07 2021, Neeraj Singh wrote:
> >>
> >> > On Tue, Dec 7, 2021 at 5:01 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >> >>
> >> >>
> >> >> On Tue, Dec 07 2021, Neeraj Singh via GitGitGadget wrote:
> >> >>
> >> >> > From: Neeraj Singh <neerajsi@microsoft.com>
> >> >> >
> >> >> > 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!
> >> >>
> >> >> > * 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.
> >> >>
> >> >
> >> > 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".
> >>
> >> > 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...
> >
> > 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.
>

I see what you're saying for cases where someone would want to set a
core.fsync setting that's derived from the user's current config.  But
I'm guessing that the dominant use case is someone setting a new fsync
configuration that achieves some atomic goal with respect to a given
repository. Like "this is a throwaway, so sync nothing" or "this is
really important, so sync all objects and refs and the index".

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

My main point here is that there's precedent for patch's current exact
schema for a config in the same core config leaf of the Documentation.
It seems from your comments that we'd have to invent and document some
new convention for "reset" of a multi-valued config.  So you're
suggesting that I solve an extra set of problems to get this change
in.  Just want to remind you that my personal itch to scratch is to
get the underlying mechanism in so that git-for-windows can set its
default setting to batch mode. I'm not expecting many users to
actually configure this setting to any non-default value.

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

Yeah, I meant "key".  I conflated the config key with the underlying
global variable in git.

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

My reading of the code is that diff.wsErrorHighlight is a comma
separated list and not a multi-valued config.  Actually I haven't yet
found an existing multi-valued config (not sure how to grep for it).

> >> >> > 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/xmqqr1dqzyl7.fsf@gitster.g/
> >>
> >> 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?
>
> 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.

My batch mode perf tests (on github, not yet submitted to the list)
use `git -c core.fsync=<foo>` to set up a per-process config.  I
haven't used the `git config` writing support in a while, so I haven't
deeply thought about that.  However, I favor simplifying the use case
of "atomically" setting a new holistic core.fsync value versus the use
case of deriving a new core.fsync value from the preexisting value.

> 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.
>
> >> >> > Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
> >> >> > ---
> >> >> >  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(-)
> >> >> >
> >> >> > diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> >> >> > index dbb134f7136..4f1747ec871 100644
> >> >> > --- a/Documentation/config/core.txt
> >> >> > +++ b/Documentation/config/core.txt
> >> >> > @@ -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.
> >> >> > ++
> >> >> > +* `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.
> >> >>
> >> >
> >> > 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/**.
> >>
> >
> > 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...
>
> > 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.
>

I haven't seen any indication that POSIX requires an fsync for
visiblity within a running system.  I looked at the doc for open,
write, and fsync, and saw no indication that it's posix compliant to
require an fsync for visibility.  I think an OS that required fsync
for cross-process visiblity would fail to run Git for a myriad of
other reasons and would likely lose all its users.  I'm curious where
you've seen documentation that allows such unhelpful behavior?

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

Why? To be rude to such a hypothetical system, if a system were so
insanely designed, it would be nuts to support it.

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

This use-case is interesting (distinguishing remote refs from local
refs).  I think the difficulty of verifying (for even an advanced
user) that the right fsyncing is actually happening still puts me on
the side of having a carefully curated and documented set of syncable
things rather than a file-path-based mechanism.

Is this meaningful in the presumably nearby future world of the refsdb
backend?  Is that somehow split by remote versus local?

> >> >> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> >> >> > index 857be7826f3..916c55d6ce9 100644
> >> >> > --- a/builtin/pack-objects.c
> >> >> > +++ b/builtin/pack-objects.c
> >> >> > @@ -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...
> >> >
> >> > 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.
> >>
> >
> > 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.
>

The underlying fsync primitive does have a meaning on Windows for
pipes, but it's certainly not what Git would want to do. Also if
stdout is redirected to a file, I'm pretty sure that UNIX OSes would
respect the fsync call.  However it's not meaningful in the sense of
the git repository, since we don't know what the packfile is or why it
was created.

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

We're trying to be deliberate about what we're fsyncing.  Fsyncing an
unknown file created by the packfile code doesn't move us in that
direction.  In your taxonomy we don't know (1), "what is this category
of thing?"  Sure it's got the packfile format, but is not known to be
an actual packfile that's part of the repository.

> >>
> >> >> > [...]
> >> >> > +/*
> >> >> > + * 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...
> >> >>
> >> >> > +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");
> >> >>
> >> >> 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.
> >>
> >> 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.
>

Am I allowed to change the config value string in place? The
core.whitespace code is careful not to modify the string. I kind of
like the parse_ws_error_highlight code a little better now that I've
seen it, but I think the current code is fine too.

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

Thanks,
Neeraj

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

Hi Ævar,
Could you please respond to the parent message?
To summarize the main points and questions:
1) The current patch series design of core.fsync favors ease of
modification of the complete configuration as a single atomic config
step, at the cost of making it somewhat harder to derive a new
configuration from an existing one. See [1] where this facility is
used.

2) Is there any existing configuration that uses the multi-value
schema you proposed? The diff.wsErrorHighlight setting is actually
comma-separated.

3) Is there any system you can point at or any support in the POSIX
spec for requiring fsync for anything other than durability of data
across system crash?

4) I think string_list_split_in_place is not valid for splitting a
comma-separated list in the config code, since the value coming from
the configset code is in some global data structure that might be used
again. It seems like we could have subtle problems down the line if we
mutate it.

[1] https://github.com/neerajsi-msft/git/commit/7e9749a7e94d26c88789459588997329c5130792#diff-ee0307657f5a76b723c8973db0dbd5a2ca62e14b02711b897418b35d78fc6023R1327

Thanks,
Neeraj

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


On Wed, Dec 08 2021, Neeraj Singh wrote:

[Sorry about the late reply, and thanks for the downthread prodding]

> On Wed, Dec 8, 2021 at 8:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>>
>> On Wed, Dec 08 2021, Neeraj Singh wrote:
>>
>> > On Wed, Dec 8, 2021 at 2:17 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> >>
>> >>
>> >> On Tue, Dec 07 2021, Neeraj Singh wrote:
>> >>
>> >> > On Tue, Dec 7, 2021 at 5:01 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> >> >>
>> >> >>
>> >> >> On Tue, Dec 07 2021, Neeraj Singh via GitGitGadget wrote:
>> >> >>
>> >> >> > From: Neeraj Singh <neerajsi@microsoft.com>
>> >> >> >
>> >> >> > 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!
>> >> >>
>> >> >> > * 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.
>> >> >>
>> >> >
>> >> > 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".
>> >>
>> >> > 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...
>> >
>> > 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.
>>
>
> I see what you're saying for cases where someone would want to set a
> core.fsync setting that's derived from the user's current config.  But
> I'm guessing that the dominant use case is someone setting a new fsync
> configuration that achieves some atomic goal with respect to a given
> repository. Like "this is a throwaway, so sync nothing" or "this is
> really important, so sync all objects and refs and the index".

Whether it's multi-value or comma-separated you could do:

    -c core.fsync=[none|false]

To sync nothing, i.e. if we see "none/false" it doesn't matter if we saw
core.fsync=loose-object or whatever before, it would override it, ditto
for "all".

>> > 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).
>>
>
> My main point here is that there's precedent for patch's current exact
> schema for a config in the same core config leaf of the Documentation.
> It seems from your comments that we'd have to invent and document some
> new convention for "reset" of a multi-valued config.  So you're
> suggesting that I solve an extra set of problems to get this change
> in.  Just want to remind you that my personal itch to scratch is to
> get the underlying mechanism in so that git-for-windows can set its
> default setting to batch mode. I'm not expecting many users to
> actually configure this setting to any non-default value.

Me neither. I think people will most likely set this once in
~/.gitconfig or /etc/gitconfig.

We have some config keys that are multi-value and either comma-separated
or space-separated, e.g. core.alternateRefsPrefixes

Then we have e.g. blame.ignoreRevsFile which is multi-value, and further
has the convention that setting it to an empty value clears the
list. which would scratch the "override existing" itch.

format.notes, versionsort.suffix, transfer.hideRefs, branch.<name>.merge
are exmples of existing multi-value config.

>> > 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.
>
> Yeah, I meant "key".  I conflated the config key with the underlying
> global variable in git.

*nod*

>> 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.
>>
>
> My reading of the code is that diff.wsErrorHighlight is a comma
> separated list and not a multi-valued config.  Actually I haven't yet
> found an existing multi-valued config (not sure how to grep for it).

Yes, I think I conflated it with one of the ones above when I wrote
this.

>> >> >> > 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/xmqqr1dqzyl7.fsf@gitster.g/
>> >>
>> >> 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?
>>
>> 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.
>
> My batch mode perf tests (on github, not yet submitted to the list)
> use `git -c core.fsync=<foo>` to set up a per-process config.  I
> haven't used the `git config` writing support in a while, so I haven't
> deeply thought about that.  However, I favor simplifying the use case
> of "atomically" setting a new holistic core.fsync value versus the use
> case of deriving a new core.fsync value from the preexisting value.

If you implement it like blame.ignoreRevsFile you can have your cake and
eat it too, i.e.:

    -c core.fsync= core.fsync=loose-object

ensures only loose objects are synced, as with your single-value config,
but I'd think what you'd be more likely to actually mean would be:

    # With "core.fsync=pack" set in ~/.gitconfig
    -c core.fsync=loose-object

I.e. that the common case is "I want this to be synced here", not that
you'd like to declare sync policy from scratch every time.

In any case, on this general topic my main point is that the
git-config(1) command has pretty integration for multi-value if you do
it that way, and not for comma-delimited. I.e. you get --add, --unset,
--unset-all, --get, --get-all etc.

So I think for anything new it makes sense to lean into that, I think
most of these existing comma-delimited ones are ones we'd do differently
today on reflection.

And if you suppor the "empty resets" like blame.ignoreRevsFile it seems
to me you'll have your cake & eat it too.

>> 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.
>>
>> >> >> > Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
>> >> >> > ---
>> >> >> >  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(-)
>> >> >> >
>> >> >> > diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
>> >> >> > index dbb134f7136..4f1747ec871 100644
>> >> >> > --- a/Documentation/config/core.txt
>> >> >> > +++ b/Documentation/config/core.txt
>> >> >> > @@ -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.
>> >> >> > ++
>> >> >> > +* `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.
>> >> >>
>> >> >
>> >> > 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/**.
>> >>
>> >
>> > 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...
>>
>> > 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.
>>
>
> I haven't seen any indication that POSIX requires an fsync for
> visiblity within a running system.  I looked at the doc for open,
> write, and fsync, and saw no indication that it's posix compliant to
> require an fsync for visibility.  I think an OS that required fsync
> for cross-process visiblity would fail to run Git for a myriad of
> other reasons and would likely lose all its users.  I'm curious where
> you've seen documentation that allows such unhelpful behavior?

There's multiple unrelated and related things in this area. One is a
case where you'll e.g. write a file "foo" using stdio, spawn a program
to work on it in the same program, but it might not see it at all, or
see empty content, the latter being because you haven't flushed your I/O
buffers (which you can do via fsync()).

The former is that on *nix systems you're generally only guaranteed to
write to a fd, but not to have the associated metadata be synced for
you.

That is spelled out e.g. in the DESCRIPTION section of linux's fsync()
manpage: https://man7.org/linux/man-pages/man2/fdatasync.2.html

I don't know how much you follow non-Windows FS development, but there
was also a very well known "incident" early in ext4 where it leaned into
some permissive-by-POSIX behavior that caused data loss in practice on
buggy programs that didn't correctly use fsync() , since various tooling
had come to expect the stricter behavior of ext3:
https://lwn.net/Articles/328363/

That was explicitly in the area of fs metadata being discussed here.

Generally you can expect your VFS layer to be forgiving when it comes to
IPC, but even that is out the window when it comes to networked
filesystems, e.g. a shared git repository hosted on NFS.

>> 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.
>
> Why? To be rude to such a hypothetical system, if a system were so
> insanely designed, it would be nuts to support it.

Because we know that right now the system calls we're invoking aren't
guaranteed to store data persistently to disk portably, although they do
so in practice on most modern OS's.

We're portably to a lot of platforms, and also need to keep e.g. NFS in
mind, so being able to ask for a pedantic mode when you care about data
retention at the cost of performance would be nice.

And because the fsync config mode you're proposing is thoroughly
non-standard, but is known to me much faster by leaning into known
attributes of specific FS's on specific OS's, if we're not running on
those it would be sensible to fall back to a stricter mode of
operation. E.g. syncing all 100 loose objects we just wrote, not just
the last one.

>> > 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?
>>
>> 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.
>
> This use-case is interesting (distinguishing remote refs from local
> refs).  I think the difficulty of verifying (for even an advanced
> user) that the right fsyncing is actually happening still puts me on
> the side of having a carefully curated and documented set of syncable
> things rather than a file-path-based mechanism.
>
> Is this meaningful in the presumably nearby future world of the refsdb
> backend?  Is that somehow split by remote versus local?

There is the upcoming "reftable" work, but that's probably 2-3 years out
at the earliest for series production workloads in git.git.

>> >> >> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>> >> >> > index 857be7826f3..916c55d6ce9 100644
>> >> >> > --- a/builtin/pack-objects.c
>> >> >> > +++ b/builtin/pack-objects.c
>> >> >> > @@ -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...
>> >> >
>> >> > 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.
>> >>
>> >
>> > 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.
>>
>
> The underlying fsync primitive does have a meaning on Windows for
> pipes, but it's certainly not what Git would want to do. Also if
> stdout is redirected to a file, I'm pretty sure that UNIX OSes would
> respect the fsync call.  However it's not meaningful in the sense of
> the git repository, since we don't know what the packfile is or why it
> was created.

I suggested that because I think it's probably nonsensical, but it's
nonsense that POSIX seems to explicitly tell us that it'll handle
(probably by silently doing nothing). So in terms of our interface we
could lean into that and avoid our own special-casing.

>> 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...
>
> We're trying to be deliberate about what we're fsyncing.  Fsyncing an
> unknown file created by the packfile code doesn't move us in that
> direction.  In your taxonomy we don't know (1), "what is this category
> of thing?"  Sure it's got the packfile format, but is not known to be
> an actual packfile that's part of the repository.

We know it's a fd, isn't that sufficient? In any case, I'm fine with
also keeping it as is, I don't mean to split hairs here.

It just stuck out as an odd part of the interface, why treat some fd's
specially, instead of just throwing it all at the OS. Presumably the
first thing the OS will do is figure out if it's a syncable fd or not,
and act appropriately.

>> >>
>> >> >> > [...]
>> >> >> > +/*
>> >> >> > + * 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...
>> >> >>
>> >> >> > +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");
>> >> >>
>> >> >> 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.
>> >>
>> >> 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.
>>
>
> Am I allowed to change the config value string in place? The
> core.whitespace code is careful not to modify the string. I kind of
> like the parse_ws_error_highlight code a little better now that I've
> seen it, but I think the current code is fine too.

I don't remember offhand if that's safe, probably not. So you'll need a
copy here.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


On Tue, Jan 18 2022, Neeraj Singh wrote:

> Hi Ævar,
> Could you please respond to the parent message?

I did just now in
https://lore.kernel.org/git/220119.8635ljoidt.gmgdl@evledraar.gmail.com/;
sorry about the delay. This thread fell off my radar.

> To summarize the main points and questions:
> 1) The current patch series design of core.fsync favors ease of
> modification of the complete configuration as a single atomic config
> step, at the cost of making it somewhat harder to derive a new
> configuration from an existing one. See [1] where this facility is
> used.
>
> 2) Is there any existing configuration that uses the multi-value
> schema you proposed? The diff.wsErrorHighlight setting is actually
> comma-separated.

I replied to that. To add a bit to that the comma-delimited thing isn't
any sort of a "blocker" or whatever in my mind.

I just wanted to point out that you could get the same with multi-value
with some better integration, and we might have our cake & eat it too.

But at the end of the day if you disagree you're doing the work, and
that's ultimately bikeshedding of the config interface. I'm fine with it
either way.

> 3) Is there any system you can point at or any support in the POSIX
> spec for requiring fsync for anything other than durability of data
> across system crash?

Some examples in the linked...

> 4) I think string_list_split_in_place is not valid for splitting a
> comma-separated list in the config code, since the value coming from
> the configset code is in some global data structure that might be used
> again. It seems like we could have subtle problems down the line if we
> mutate it.

Replied to that too, hope it's all useful. Thanks again for working on
this, it's really nice to have someone move the state of data integrity
in git forward.

> [1] https://github.com/neerajsi-msft/git/commit/7e9749a7e94d26c88789459588997329c5130792#diff-ee0307657f5a76b723c8973db0dbd5a2ca62e14b02711b897418b35d78fc6023R1327

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Wed, Jan 19, 2022 at 10:27 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Wed, Dec 08 2021, Neeraj Singh wrote:
>
> [Sorry about the late reply, and thanks for the downthread prodding]

I also apologize for the late reply here.  I've been dealing with a
hospitalized parent this week.

>
> > On Wed, Dec 8, 2021 at 8:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >>
> >>
> >> On Wed, Dec 08 2021, Neeraj Singh wrote:
> >>
> >> > On Wed, Dec 8, 2021 at 2:17 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >> >>
> >> >>
> >> >> On Tue, Dec 07 2021, Neeraj Singh wrote:
> >> >>
> >> >> > On Tue, Dec 7, 2021 at 5:01 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >> >> >>
> >> >> >>
> >> >> >> On Tue, Dec 07 2021, Neeraj Singh via GitGitGadget wrote:
> >> >> >>
> >> >> >> > From: Neeraj Singh <neerajsi@microsoft.com>
> >> >> >> >
> >> >> >> > 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!
> >> >> >>
> >> >> >> > * 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.
> >> >> >>
> >> >> >
> >> >> > 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".
> >> >>
> >> >> > 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...
> >> >
> >> > 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.
> >>
> >
> > I see what you're saying for cases where someone would want to set a
> > core.fsync setting that's derived from the user's current config.  But
> > I'm guessing that the dominant use case is someone setting a new fsync
> > configuration that achieves some atomic goal with respect to a given
> > repository. Like "this is a throwaway, so sync nothing" or "this is
> > really important, so sync all objects and refs and the index".
>
> Whether it's multi-value or comma-separated you could do:
>
>     -c core.fsync=[none|false]
>
> To sync nothing, i.e. if we see "none/false" it doesn't matter if we saw
> core.fsync=loose-object or whatever before, it would override it, ditto
> for "all".
>
> >> > 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).
> >>
> >
> > My main point here is that there's precedent for patch's current exact
> > schema for a config in the same core config leaf of the Documentation.
> > It seems from your comments that we'd have to invent and document some
> > new convention for "reset" of a multi-valued config.  So you're
> > suggesting that I solve an extra set of problems to get this change
> > in.  Just want to remind you that my personal itch to scratch is to
> > get the underlying mechanism in so that git-for-windows can set its
> > default setting to batch mode. I'm not expecting many users to
> > actually configure this setting to any non-default value.
>
> Me neither. I think people will most likely set this once in
> ~/.gitconfig or /etc/gitconfig.
>
> We have some config keys that are multi-value and either comma-separated
> or space-separated, e.g. core.alternateRefsPrefixes
>
> Then we have e.g. blame.ignoreRevsFile which is multi-value, and further
> has the convention that setting it to an empty value clears the
> list. which would scratch the "override existing" itch.
>
> format.notes, versionsort.suffix, transfer.hideRefs, branch.<name>.merge
> are exmples of existing multi-value config.
>

Thanks for the examples.  I can see the benefit of mutli-value for
most of those settings, but for versionsort.suffix, I'd personally
have wanted a comma-separated list.

> >> > 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.
> >
> > Yeah, I meant "key".  I conflated the config key with the underlying
> > global variable in git.
>
> *nod*
>
> >> 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.
> >>
> >
> > My reading of the code is that diff.wsErrorHighlight is a comma
> > separated list and not a multi-valued config.  Actually I haven't yet
> > found an existing multi-valued config (not sure how to grep for it).
>
> Yes, I think I conflated it with one of the ones above when I wrote
> this.
>
> >> >> >> > 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/xmqqr1dqzyl7.fsf@gitster.g/
> >> >>
> >> >> 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?
> >>
> >> 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.
> >
> > My batch mode perf tests (on github, not yet submitted to the list)
> > use `git -c core.fsync=<foo>` to set up a per-process config.  I
> > haven't used the `git config` writing support in a while, so I haven't
> > deeply thought about that.  However, I favor simplifying the use case
> > of "atomically" setting a new holistic core.fsync value versus the use
> > case of deriving a new core.fsync value from the preexisting value.
>
> If you implement it like blame.ignoreRevsFile you can have your cake and
> eat it too, i.e.:
>
>     -c core.fsync= core.fsync=loose-object
>
> ensures only loose objects are synced, as with your single-value config,
> but I'd think what you'd be more likely to actually mean would be:
>
>     # With "core.fsync=pack" set in ~/.gitconfig
>     -c core.fsync=loose-object
>
> I.e. that the common case is "I want this to be synced here", not that
> you'd like to declare sync policy from scratch every time.
>
> In any case, on this general topic my main point is that the
> git-config(1) command has pretty integration for multi-value if you do
> it that way, and not for comma-delimited. I.e. you get --add, --unset,
> --unset-all, --get, --get-all etc.
>
> So I think for anything new it makes sense to lean into that, I think
> most of these existing comma-delimited ones are ones we'd do differently
> today on reflection.
>
> And if you suppor the "empty resets" like blame.ignoreRevsFile it seems
> to me you'll have your cake & eat it too.
>

I really don't like the multiple `-c core.fsync=` clauses.  If I want
to do packs and loose-objects as a single config, I'd have to do:
        * multi-value: `git -c core.fsync= -c core.fsync=pack -c
core.fsync=loose-object`
        * comma-sep `git -c core.fsync=pack,loose-object`

Multi-valued configs are stateful and more verbose to configure.
Last-one-wins with comma-separated values has an advantage for
achieving a desired final configuration without regard for the
previous configuration, which is the way I expect the feature to be
used.

> >> 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.
> >>
> >> >> >> > Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
> >> >> >> > ---
> >> >> >> >  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(-)
> >> >> >> >
> >> >> >> > diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> >> >> >> > index dbb134f7136..4f1747ec871 100644
> >> >> >> > --- a/Documentation/config/core.txt
> >> >> >> > +++ b/Documentation/config/core.txt
> >> >> >> > @@ -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.
> >> >> >> > ++
> >> >> >> > +* `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.
> >> >> >>
> >> >> >
> >> >> > 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/**.
> >> >>
> >> >
> >> > 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...
> >>
> >> > 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.
> >>
> >
> > I haven't seen any indication that POSIX requires an fsync for
> > visiblity within a running system.  I looked at the doc for open,
> > write, and fsync, and saw no indication that it's posix compliant to
> > require an fsync for visibility.  I think an OS that required fsync
> > for cross-process visiblity would fail to run Git for a myriad of
> > other reasons and would likely lose all its users.  I'm curious where
> > you've seen documentation that allows such unhelpful behavior?
>
> There's multiple unrelated and related things in this area. One is a
> case where you'll e.g. write a file "foo" using stdio, spawn a program
> to work on it in the same program, but it might not see it at all, or
> see empty content, the latter being because you haven't flushed your I/O
> buffers (which you can do via fsync()).
>

For stdio you need to use fflush(3), which just flushes the C
runtime's internal buffers.  You need to do the following to do a full
durable write using stdio:
```
FILE *fp;
...
fflush(fp);
fsync(fileno(fp))
```

> The former is that on *nix systems you're generally only guaranteed to
> write to a fd, but not to have the associated metadata be synced for
> you.
>
> That is spelled out e.g. in the DESCRIPTION section of linux's fsync()
> manpage: https://man7.org/linux/man-pages/man2/fdatasync.2.html
>
> I don't know how much you follow non-Windows FS development, but there
> was also a very well known "incident" early in ext4 where it leaned into
> some permissive-by-POSIX behavior that caused data loss in practice on
> buggy programs that didn't correctly use fsync() , since various tooling
> had come to expect the stricter behavior of ext3:
> https://lwn.net/Articles/328363/
>
> That was explicitly in the area of fs metadata being discussed here.
>
> Generally you can expect your VFS layer to be forgiving when it comes to
> IPC, but even that is out the window when it comes to networked
> filesystems, e.g. a shared git repository hosted on NFS.
>

Everything in the fsync(2) DESCRIPTION section is about what data and
metadata reaches the disk (versus just being cached in-memory).  I've
become a bit familiar with the ext3 vs ext4 (and delayed alloc)
behavior while researching this feature.  These behaviors are all
around the durability you get in the case of kernel crash,
power-failure, or other forms of dirty dismount.

NFS is a complex story.  I'm not intimately familiar with its
particular pitfalls, but from looking at the Linux NFS faq
(http://nfs.sourceforge.net/), it appears that a given single NFS
client will remain coherent with itself. Multiple NFS clients
accessing a single Git repo concurrently are probably going to see
some inconsistency.  In that kind of case, fsync would help, perhaps,
since it would force NFS clients to issue a COMMIT command to the
server.

> >> 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.
> >
> > Why? To be rude to such a hypothetical system, if a system were so
> > insanely designed, it would be nuts to support it.
>
> Because we know that right now the system calls we're invoking aren't
> guaranteed to store data persistently to disk portably, although they do
> so in practice on most modern OS's.
>
> We're portably to a lot of platforms, and also need to keep e.g. NFS in
> mind, so being able to ask for a pedantic mode when you care about data
> retention at the cost of performance would be nice.
>
> And because the fsync config mode you're proposing is thoroughly
> non-standard, but is known to me much faster by leaning into known
> attributes of specific FS's on specific OS's, if we're not running on
> those it would be sensible to fall back to a stricter mode of
> operation. E.g. syncing all 100 loose objects we just wrote, not just
> the last one.
>
> >> > 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?
> >>
> >> 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.
> >
> > This use-case is interesting (distinguishing remote refs from local
> > refs).  I think the difficulty of verifying (for even an advanced
> > user) that the right fsyncing is actually happening still puts me on
> > the side of having a carefully curated and documented set of syncable
> > things rather than a file-path-based mechanism.
> >
> > Is this meaningful in the presumably nearby future world of the refsdb
> > backend?  Is that somehow split by remote versus local?
>
> There is the upcoming "reftable" work, but that's probably 2-3 years out
> at the earliest for series production workloads in git.git.
>
> >> >> >> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> >> >> >> > index 857be7826f3..916c55d6ce9 100644
> >> >> >> > --- a/builtin/pack-objects.c
> >> >> >> > +++ b/builtin/pack-objects.c
> >> >> >> > @@ -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...
> >> >> >
> >> >> > 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.
> >> >>
> >> >
> >> > 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.
> >>
> >
> > The underlying fsync primitive does have a meaning on Windows for
> > pipes, but it's certainly not what Git would want to do. Also if
> > stdout is redirected to a file, I'm pretty sure that UNIX OSes would
> > respect the fsync call.  However it's not meaningful in the sense of
> > the git repository, since we don't know what the packfile is or why it
> > was created.
>
> I suggested that because I think it's probably nonsensical, but it's
> nonsense that POSIX seems to explicitly tell us that it'll handle
> (probably by silently doing nothing). So in terms of our interface we
> could lean into that and avoid our own special-casing.
>
> >> 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...
> >
> > We're trying to be deliberate about what we're fsyncing.  Fsyncing an
> > unknown file created by the packfile code doesn't move us in that
> > direction.  In your taxonomy we don't know (1), "what is this category
> > of thing?"  Sure it's got the packfile format, but is not known to be
> > an actual packfile that's part of the repository.
>
> We know it's a fd, isn't that sufficient? In any case, I'm fine with
> also keeping it as is, I don't mean to split hairs here.
>
> It just stuck out as an odd part of the interface, why treat some fd's
> specially, instead of just throwing it all at the OS. Presumably the
> first thing the OS will do is figure out if it's a syncable fd or not,
> and act appropriately.
>

I'll put the following comment in pack-objects.c:
/*
* We never fsync when writing to stdout since we may
* not be writing to a specific file. For instance, the
* upload-pack code passes a pipe here. Calling fsync
* on a pipe results in unnecessary synchronization with
* the reader on some platforms.
*/

> >> >>
> >> >> >> > [...]
> >> >> >> > +/*
> >> >> >> > + * 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...
> >> >> >>
> >> >> >> > +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");
> >> >> >>
> >> >> >> 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.
> >> >>
> >> >> 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.
> >>
> >
> > Am I allowed to change the config value string in place? The
> > core.whitespace code is careful not to modify the string. I kind of
> > like the parse_ws_error_highlight code a little better now that I've
> > seen it, but I think the current code is fine too.
>
> I don't remember offhand if that's safe, probably not. So you'll need a
> copy here.
>
> >> > 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...).

Thanks again for the review, I'll send an updated PR soon.

Thanks,
Neeraj

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

I am not quite sure if this is way too complex (e.g. what does it
mean that we do not care much about loose-object safety while we do
care about commit-graph files?) and at the same time it is too
limited (e.g. if it makes sense to say a class of items deserve more
protection than another class of items, don't we want to be able to
say "class X is ultra-precious so use method A on them, while class
Y is mildly precious and use method B on them, everything else are
not that important and doing the default thing is just fine").

If we wanted to allow the "matrix" kind of flexibility, I think the
way to do so would be

	fsync.<class>.method = <value>

e.g.

	[fsync "default"] method = none
	[fsync "loose-object"] method = fsync
	[fsync "pack-metadata"] method = writeout-only

Where do we expect users to take the core.fsync settings from?  Per
repository?  If it is from per user (i.e. $HOME/.gitconfig), do
people tend to share it across systems (not necessarily over NFS)
with the same contents?  If so, I am not sure if fsync.method that
is way too close to the actual "implementation" is a good idea to
begin with.  From end-user's point of view, it may be easier to
express "class X is ultra-precious, and class Y and Z are mildly
so", with something like fsync.<class>.level = <how-precious> and
let the Git implementation on each platform choose the appropriate
fsync method to protect the stuff at that precious-ness.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes:

> I am not quite sure if this is way too complex (e.g. what does it
> mean that we do not care much about loose-object safety while we do
> care about commit-graph files?) and at the same time it is too
> limited (e.g. if it makes sense to say a class of items deserve more
> protection than another class of items, don't we want to be able to
> say "class X is ultra-precious so use method A on them, while class
> Y is mildly precious and use method B on them, everything else are
> not that important and doing the default thing is just fine").
>
> If we wanted to allow the "matrix" kind of flexibility,...

To continue with the thinking aloud...

Sometimes configuration flexibility is truly needed, but often it is
just a sign of designer being lazy and not thinking it through as an
end-user facing problem.  In other words, "I am giving enough knobs
to you, so it is up to you to express your policy in whatever way
you want with the knobs provided" is a very irresponsible thing to
tell end-users.

And this one smells like the case of a lazy design.

It may be that it makes sense in some workflows to protect
commit-graph files less than object files and pack.idx files can be
corrupted as long as pack.pack files are adequately protected
because the former can be recomputed from the latter, but in no
workflows, the reverse would be true.  Yet the design gives such
needless flexibility, which makes it hard for lay end-users to
choose the best combination and allows them to protect .idx files
more than .pack files by mistake, for example.

I am wondering if the classification itself introduced by this step
actually can form a natural and linear progression of safe-ness.  By
default, we'd want _all_ classes of things to be equally safe, but
at one level down, there is "protect things that are not
recomputable, but recomputable things can be left to the system"
level, and there would be even riskier "protect packs as it would
hurt a _lot_ to lose them, but losing loose ones will typically lose
only the most recent work, and they are less valuable" level.

If we, as the Git experts, spend extra brain cycles to come up with
an easy to understand spectrum of performance vs durability
trade-off, end-users won't have to learn the full flexibility and
easily take the advice from experts.  They just need to say what
level of durability they want (or how much durability they can risk
in exchange for an additional throughput), and leave the rest to us.

On the core.fsyncMethod side, the same suggestion applies.

Once we know the desired level of performance vs durability
trade-off from the user, we, as the impolementors, should know the
best method, for each class of items, to achieve that durability on
each platform when writing it to the storage, without exposing the
low level details of the implementation that only the Git folks need
to be aware of.

So, from the end-user UI perspective, I'd very much prefer if we can
just come up with a single scalar variable, (say "fsync.durability"
that ranges from "conservative" to "performance") that lets our
users express the level of durability desired.  The combination of
core.fsyncMethod and core.fsync are one variable too many, and the
latter being a variable that takes a list of things as its value
makes it even worse to sell to the end users.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

Apologies in advance for the delayed reply.  I've finally been able to
return to Git after an absence.

On Tue, Feb 1, 2022 at 4:51 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +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.
> > ++
> > +* `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.
>
> I am not quite sure if this is way too complex (e.g. what does it
> mean that we do not care much about loose-object safety while we do
> care about commit-graph files?) and at the same time it is too
> limited (e.g. if it makes sense to say a class of items deserve more
> protection than another class of items, don't we want to be able to
> say "class X is ultra-precious so use method A on them, while class
> Y is mildly precious and use method B on them, everything else are
> not that important and doing the default thing is just fine").
>
> If we wanted to allow the "matrix" kind of flexibility, I think the
> way to do so would be
>
>         fsync.<class>.method = <value>
>
> e.g.
>
>         [fsync "default"] method = none
>         [fsync "loose-object"] method = fsync
>         [fsync "pack-metadata"] method = writeout-only
>

I don't believe it makes sense to offer a full matrix of what to fsync
and what method to use, since the method is a property of the
filesystem and OS the repo is running on, while the list of things to
fsync is more a selection of what the user values. So if I'm hosting
on APFS on macOS or NTFS on Windows, I'd want to set the fsyncMethod
to batch so that I can get good performance at the safety level I
choose.  If I'm working on my maintainer repo, I'd maybe not want to
fsync anything, but I'd want to fsync everything when working on my
developer repo.

> Where do we expect users to take the core.fsync settings from?  Per
> repository?  If it is from per user (i.e. $HOME/.gitconfig), do
> people tend to share it across systems (not necessarily over NFS)
> with the same contents?  If so, I am not sure if fsync.method that
> is way too close to the actual "implementation" is a good idea to
> begin with.  From end-user's point of view, it may be easier to
> express "class X is ultra-precious, and class Y and Z are mildly
> so", with something like fsync.<class>.level = <how-precious> and
> let the Git implementation on each platform choose the appropriate
> fsync method to protect the stuff at that precious-ness.
>

I expect the vast majority of users to have whatever setting is baked
into their build of Git.  For the users that want to do something
different, I expect them to have core.fsyncMethod and core.fsync
configured per-user for the majority of their repos. Some repos might
have custom settings that override the per-user settings: 1) Ephemeral
repos that don't contain unique data would probably want to set
core.fsync=none. 2) Repos hosting on NFS or on a different FS may have
a stricter core.fsyncmethod setting.

(More more text to follow in reply to your next email).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Tue, Feb 1, 2022 at 5:42 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I am not quite sure if this is way too complex (e.g. what does it
> > mean that we do not care much about loose-object safety while we do
> > care about commit-graph files?) and at the same time it is too
> > limited (e.g. if it makes sense to say a class of items deserve more
> > protection than another class of items, don't we want to be able to
> > say "class X is ultra-precious so use method A on them, while class
> > Y is mildly precious and use method B on them, everything else are
> > not that important and doing the default thing is just fine").
> >
> > If we wanted to allow the "matrix" kind of flexibility,...
>
> To continue with the thinking aloud...
>
> Sometimes configuration flexibility is truly needed, but often it is
> just a sign of designer being lazy and not thinking it through as an
> end-user facing problem.  In other words, "I am giving enough knobs
> to you, so it is up to you to express your policy in whatever way
> you want with the knobs provided" is a very irresponsible thing to
> tell end-users.
>
> And this one smells like the case of a lazy design.
>
> It may be that it makes sense in some workflows to protect
> commit-graph files less than object files and pack.idx files can be
> corrupted as long as pack.pack files are adequately protected
> because the former can be recomputed from the latter, but in no
> workflows, the reverse would be true.  Yet the design gives such
> needless flexibility, which makes it hard for lay end-users to
> choose the best combination and allows them to protect .idx files
> more than .pack files by mistake, for example.
>
> I am wondering if the classification itself introduced by this step
> actually can form a natural and linear progression of safe-ness.  By
> default, we'd want _all_ classes of things to be equally safe, but
> at one level down, there is "protect things that are not
> recomputable, but recomputable things can be left to the system"
> level, and there would be even riskier "protect packs as it would
> hurt a _lot_ to lose them, but losing loose ones will typically lose
> only the most recent work, and they are less valuable" level.
>
> If we, as the Git experts, spend extra brain cycles to come up with
> an easy to understand spectrum of performance vs durability
> trade-off, end-users won't have to learn the full flexibility and
> easily take the advice from experts.  They just need to say what
> level of durability they want (or how much durability they can risk
> in exchange for an additional throughput), and leave the rest to us.
>
> On the core.fsyncMethod side, the same suggestion applies.
>
> Once we know the desired level of performance vs durability
> trade-off from the user, we, as the impolementors, should know the
> best method, for each class of items, to achieve that durability on
> each platform when writing it to the storage, without exposing the
> low level details of the implementation that only the Git folks need
> to be aware of.
>
> So, from the end-user UI perspective, I'd very much prefer if we can
> just come up with a single scalar variable, (say "fsync.durability"
> that ranges from "conservative" to "performance") that lets our
> users express the level of durability desired.  The combination of
> core.fsyncMethod and core.fsync are one variable too many, and the
> latter being a variable that takes a list of things as its value
> makes it even worse to sell to the end users.

I see the value in simplifying the core.fsync configuration to a
single scalar knob of preciousness. The main motivation for this more
granular scheme is that I didn't think the current configuration
follows a sensible principle. We should be fsyncing the loose objects,
index, refs, and config files in addition to what we're already
syncing today.  On macOS, we should be doing a full hardware flush if
we've said we want to fsync.  But you expressed the notion in [1] that
we don't want to degrade the performance of the vast majority of users
who are happy with the current "unprincipled but mostly works"
configuration.  I agree with that sentiment, but it leads to a design
where we can express the current configuration, which does not follow
a scalar hierarchy.

The aggregate core.fsync options are meant to provide a way for us to
recommend a sensible configuration to the user without having to get
into the intricacies of repo layout. Maybe we can define and document
aggregate options that make sense in terms of a scalar level of
preciousness.

One reason to keep core.fsyncMethod separate from the core.fsync knob
is that it's more a property of the system and FS the repo is on,
rather than the user's value of the repo.  We could try to auto-detect
some known filesystems that might support batch mode using 'statfs',
but having a table like that in Git would go out of date over time.

Please let me know what you think about these justifications for the
current design.  I'd be happy to make a change if the current
constraint of "keep the default config the same" can be relaxed in
some way.  I'd also be happy to go back to some variation of
expressing 'core.fsyncObjectFiles = batch' and leaving the rest of
fsync story alone.

Thanks,
Neeraj

[1] https://lore.kernel.org/git/xmqqtuilyfls.fsf@gitster.g/

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Neeraj Singh <nksingh85@gmail.com> writes:

> The main motivation for this more granular scheme is that I didn't
> think the current configuration follows a sensible principle. We
> should be fsyncing the loose objects, index, refs, and config
> files in addition to what we're already syncing today.  On macOS,
> we should be doing a full hardware flush if we've said we want to
> fsync.

If the "robustness vs performance" trade-off is unevenly made in the
current code, then that is a very good problem to address first, and
such a change is very much justified on its own.

Perhaps "this is not a primary work repository but is used only to
follow external site to build, hence no fsync is fine" folks, who do
not have core.fsyncObjectFiles set to true, may appreciate if we
stopped doing fsync for packs and other things.  As the Boolean
core.fsyncObjectFiles is the only end-user visible knob to express
how the end-users express the trade-off, a good first step would be
to align other file accesses to the preference expressed by it, i.e.
others who say they want fsync in the current scheme would
appreciate if we start fsync in places like ref-files backend.

Making the choice more granular, from Boolean "yes/no", to linear
levels, would also be a good idea.  Doing both at the same time may
make it harder to explain and justify, but as long as at the end, if
"very performant" choice uniformly does not do any fsync while
"ultra durable" choice makes a uniform effort across subsystems to
make sure bits hit the platter, it would be a very good idea to do
them.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


On Wed, Mar 09 2022, Patrick Steinhardt wrote:

> [[PGP Signed Part:Undecided]]
> On Mon, Feb 14, 2022 at 09:17:31AM -0800, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> 
>> > To summarize my take: while the degree of durability may be something
>> > that's up for discussions, I think that the current defaults for
>> > atomicity are bad for users because they can and do lead to repository
>> > corruption.
>> 
>> Good summary.
>> 
>> If the user cares about fsynching loose object files in the right
>> way, we shouldn't leave loose ref files not following the safe
>> safety level, regardless of how this new core.fsync knobs would look
>> like.
>> 
>> I think we three are in agreement on that.
>
> Is there anything I can specifically do to help out with this topic? We
> have again hit data loss in production because we don't sync loose refs
> to disk before renaming them into place, so I'd really love to sort out
> this issue somehow so that I can revive my patch series which fixes the
> known repository corruption [1].
>
> Alternatively, can we maybe find a way forward with applying a version
> of my patch series without first settling the bigger question of how we
> want the overall design to look like? In my opinion repository
> corruption is a severe bug that needs to be fixed, and it doesn't feel
> sensible to block such a fix over a discussion that potentially will
> take a long time to settle.
>
> Patrick
>
> [1]: http://public-inbox.org/git/cover.1636544377.git.ps@pks.im/

I share that view. I was wondering how this topic fizzled out the other
day, but then promptly forgot about it.

I think the best thing at this point (hint hint!) would be for someone
in the know to (re-)submit the various patches appropriate to move this
forward. Whether that's just this series, part of it, or some/both of
those + patches from you and Eric and this point I don't know/remember.

But just to be explicitly clear, as probably the person most responsible
for pushing this towards the "bigger question of [...] overall
design".

I just wanted to facilitate a discussion that would result in the
various stakeholders who wanted to add some fsync-related config coming
up with something that's mutually compatible, and I think the design
from Neeraj in this series fits that purpose, is Good Enough etc.

I.e. the actually important and IMO blockers were all resolved, e.g. not
having an fsync configuration that older git versions would needlessly
die on, and not painting ourselves into a corner where
e.g. core.fsync=false or something was squatted on by something other
than a "no fsync, whatsoever" etc.

(But I haven't looked at it again just now, so...)

Anyway, just trying to be explicit that to whatever extent this was held
up by questions/comments of mine I'm very happy to see this go forward.
As you (basically) say we shouldn't lose sight of ongoing data loss in
this area because of some config bikeshedding :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Patrick Steinhardt <ps@pks.im> writes:

>> If the user cares about fsynching loose object files in the right
>> way, we shouldn't leave loose ref files not following the safe
>> safety level, regardless of how this new core.fsync knobs would look
>> like.
>> 
>> I think we three are in agreement on that.
>
> Is there anything I can specifically do to help out with this topic? We
> have again hit data loss in production because we don't sync loose refs
> to disk before renaming them into place, so I'd really love to sort out
> this issue somehow so that I can revive my patch series which fixes the
> known repository corruption [1].

How about doing a series to unconditionally sync loose ref creation
and modification?

Alternatively, we could link it to the existing configuration to
control synching of object files.

I do not think core.fsyncObjectFiles having "object" in its name is
a good reason not to think those who set it to true only care about
the loose object files and nothing else.  It is more sensible to
consider that those who set it to true cares about the repository
integrity more than those who set it to false, I would think.

But that (i.e. doing it conditionally and choose which knob to use)
is one extra thing that needs justification, so starting from
unconditional fsync_or_die() may be the best way to ease it in.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Wed, Mar 9, 2022 at 5:42 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Feb 14, 2022 at 09:17:31AM -0800, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > > To summarize my take: while the degree of durability may be something
> > > that's up for discussions, I think that the current defaults for
> > > atomicity are bad for users because they can and do lead to repository
> > > corruption.
> >
> > Good summary.
> >
> > If the user cares about fsynching loose object files in the right
> > way, we shouldn't leave loose ref files not following the safe
> > safety level, regardless of how this new core.fsync knobs would look
> > like.
> >
> > I think we three are in agreement on that.
>
> Is there anything I can specifically do to help out with this topic? We
> have again hit data loss in production because we don't sync loose refs
> to disk before renaming them into place, so I'd really love to sort out
> this issue somehow so that I can revive my patch series which fixes the
> known repository corruption [1].
>
> Alternatively, can we maybe find a way forward with applying a version
> of my patch series without first settling the bigger question of how we
> want the overall design to look like? In my opinion repository
> corruption is a severe bug that needs to be fixed, and it doesn't feel
> sensible to block such a fix over a discussion that potentially will
> take a long time to settle.
>
> Patrick
>
> [1]: http://public-inbox.org/git/cover.1636544377.git.ps@pks.im/

Hi Patrick,
Thanks for reviving this discussion.  I've updated the PR on
GitGitGadget with a rebase
onto the current 'main' branch and some minor build fixes. I've also
revamped the aggregate options
and documentation to be more inline with Junio's suggestion of having
'levels of safety' that we steer
the user towards. I'm still keeping the detailed options, but
hopefully the guidance is clear enough to
avoid confusion.

I'd be happy to make any point fixes as necessary to get that branch
into proper shape for
upstream, if we've gotten to the point where we don't want to change
the fundamental design.

I agree with Patrick that the detailed knobs are primarily for use by
hosters like GitLab and GitHub.

Please expect a v5 today.

Thanks,
Neeraj

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Wed, Mar 09, 2022 at 12:03:11PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> If the user cares about fsynching loose object files in the right
> >> way, we shouldn't leave loose ref files not following the safe
> >> safety level, regardless of how this new core.fsync knobs would look
> >> like.
> >> 
> >> I think we three are in agreement on that.
> >
> > Is there anything I can specifically do to help out with this topic? We
> > have again hit data loss in production because we don't sync loose refs
> > to disk before renaming them into place, so I'd really love to sort out
> > this issue somehow so that I can revive my patch series which fixes the
> > known repository corruption [1].
> 
> How about doing a series to unconditionally sync loose ref creation
> and modification?
> 
> Alternatively, we could link it to the existing configuration to
> control synching of object files.
> 
> I do not think core.fsyncObjectFiles having "object" in its name is
> a good reason not to think those who set it to true only care about
> the loose object files and nothing else.  It is more sensible to
> consider that those who set it to true cares about the repository
> integrity more than those who set it to false, I would think.
> 
> But that (i.e. doing it conditionally and choose which knob to use)
> is one extra thing that needs justification, so starting from
> unconditional fsync_or_die() may be the best way to ease it in.

I'd be happy to revive my old patch series, but this kind of depends on
where we're standing with the other patch series by Neeraj. If you say
that we'll likely not land his patch series for the upcoming release,
but a small patch series which only starts to sync loose refs may have a
chance, then I'd like to go down that path as a stop-gap solution.
Otherwise it probably wouldn't make a lot of sense.

Patrick

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Patrick Steinhardt <ps@pks.im> writes:

>> How about doing a series to unconditionally sync loose ref creation
>> and modification?
>> 
>> Alternatively, we could link it to the existing configuration to
>> control synching of object files.
>> 
>> I do not think core.fsyncObjectFiles having "object" in its name is
>> a good reason not to think those who set it to true only care about
>> the loose object files and nothing else.  It is more sensible to
>> consider that those who set it to true cares about the repository
>> integrity more than those who set it to false, I would think.
>> 
>> But that (i.e. doing it conditionally and choose which knob to use)
>> is one extra thing that needs justification, so starting from
>> unconditional fsync_or_die() may be the best way to ease it in.
>
> I'd be happy to revive my old patch series, but this kind of depends on
> where we're standing with the other patch series by Neeraj. If you say
> that we'll likely not land his patch series for the upcoming release,
> but a small patch series which only starts to sync loose refs may have a
> chance, then I'd like to go down that path as a stop-gap solution.
> Otherwise it probably wouldn't make a lot of sense.

The above was what I wrote before the revived series from Neeraj.
Now I've seen it, and more importantly, the most recent one from you
on top of that to add ref hardening as a new "component" or two, I
like the overall shape of the end result (except for the semantics
implemented by the configuration parser, which can be fixed without
affecting how the hardening components are implemented).  Hopefully
the base series of Neeraj can be solidified soon enough to make it
unnecessary for a stop-gap measure.  We'll see.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Neeraj Singh <neerajsi@microsoft.com>
>
> This commit introduces the `core.fsyncMethod` configuration
> knob, which can currently be set to `fsync` or `writeout-only`.
>
> The new writeout-only mode attempts to tell the operating system to
> flush its in-memory page cache to the storage hardware without issuing a
> CACHE_FLUSH command to the storage controller.
>
> Writeout-only fsync is significantly faster than a vanilla fsync on
> common hardware, since data is written to a disk-side cache rather than
> all the way to a durable medium. Later changes in this patch series will
> take advantage of this primitive to implement batching of hardware
> flushes.
>
> When git_fsync is called with FSYNC_WRITEOUT_ONLY, it may fail and the
> caller is expected to do an ordinary fsync as needed.
>
> On Apple platforms, the fsync system call does not issue a CACHE_FLUSH
> directive to the storage controller. This change updates fsync to do
> fcntl(F_FULLFSYNC) to make fsync actually durable. We maintain parity
> with existing behavior on Apple platforms by setting the default value
> of the new core.fsyncMethod option.
>
> Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
> ---

OK.  This seems to be quite reasonable in that the pieces of code
that use fsync_or_die() do not have to change at all, and all of
them will keep behaving the same way.  In other words, the "how" of
fsync is very much well isolated.

Nice.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +/*
> + * 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,
> +	FSYNC_COMPONENT_LOOSE_OBJECT		= 1 << 0,
> +	FSYNC_COMPONENT_PACK			= 1 << 1,
> +	FSYNC_COMPONENT_PACK_METADATA		= 1 << 2,
> +	FSYNC_COMPONENT_COMMIT_GRAPH		= 1 << 3,
> +};

OK, so the idea is that Patrick's "we need to fsync refs" will be
done by adding a new component to this list, and sprinkling a call
to fsync_component_or_die() in the code of ref-files backend?

I am wondering if fsync_or_die() interface is abstracted well
enough, or we need things like "the fd is inside this directory; in
addition to doing the fsync of the fd, please sync the parent
directory as well" support before we start adding more components
(if there is such a need, perhaps it comes before this step).

> +#define FSYNC_COMPONENTS_DEFAULT (FSYNC_COMPONENT_PACK | \
> +				  FSYNC_COMPONENT_PACK_METADATA | \
> +				  FSYNC_COMPONENT_COMMIT_GRAPH)

IOW, everything other than loose object, which already has a
separate core.fsyncObjectFiles knob to loosen.  Everything else we
currently sync unconditionally and the default keeps that
arrangement?

> +static inline void fsync_component_or_die(enum fsync_component component, int fd, const char *msg)
> +{
> +	if (fsync_components & component)
> +		fsync_or_die(fd, msg);
> +}

Do we have a compelling reason to have this as a static inline
function?  We are talking about concluding an I/O operation and
I doubt there is a good performance argument for it.

> +static const struct fsync_component_entry {
> +	const char *name;
> +	enum fsync_component component_bits;
> +} fsync_component_table[] = {

thing[] is an array of "thing" (and thing[4] is the "fourth" such
thing), but this is not an array of a table (it is a name-to-bit
mapping).

I wonder if this array works without "_table" suffix in its name.

> +	{ "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
> +	{ "pack", FSYNC_COMPONENT_PACK },
> +	{ "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
> +	{ "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
> +};
> +
> +static enum fsync_component parse_fsync_components(const char *var, const char *string)
> +{
> +	enum fsync_component output = 0;
> +
> +	if (!strcmp(string, "none"))
> +		return FSYNC_COMPONENT_NONE;
> +
> +	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(_("ignoring unknown core.fsync component '%s'"), component);
> +			free(component);
> +		}
> +
> +		string = ep;
> +	}
> +
> +	return output;
> +}

Hmph.  I would have expected, with built-in default of
pack,pack-metadata,commit-graph,

 - "none,pack" would choose only "pack" by first clearing the
   built-in default (or whatever was set in configuration files that
   are lower precedence than what we are reading) and then OR'ing
   the "pack" bit in.

 - "-pack" would choose "pack-metadata,commit-graph" by first
   starting from the built-in default and then CLR'ing the "pack"
   bit out.  If there were already changes made by the lower
   precedence configuration files like /etc/gitconfig, the result
   might be different and the only definite thing we can say is that
   the pack bit is cleared.

 - "loose-object" would choose all of the bits by first starting
   from the built-in default and then OR'ing the "loose-object" bit
   in.

Otherwise, parsing "none" is more or less pointless, as the above
parser always start from 0 and OR's in or CLR's out the named bit.
Whoever writes "none" can just write an empty string, no?

I wonder you'd rather want to do it this way?

parse_fsync_components(var, value, current) {
	enum fsync_component positive = 0, negative = 0;

	while (string) {
		int negated = 0;
		enum fsync_component bits;

		parse out a single component into <negated, bits>;

		if (bits == 0) { /* "none" given */
                	current = 0;
		} else if (negated) {
			negative |= bits;
		} else {
			positive |= bits;
		}
		advance <string> pointer;
	}

	return (current | positive) & ~negative;
}

And then ...

> +	if (!strcmp(var, "core.fsync")) {
> +		if (!value)
> +			return config_error_nonbool(var);
> +		fsync_components = parse_fsync_components(var, value);
> +		return 0;
> +	}
> +

... this part would pass the current value of fsync_components as
the third parameter to the parse_fsync_components().  The variable
would be initialized to the FSYNC_COMPONENTS_DEFAULT we saw earlier.


> @@ -1613,7 +1684,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"));

This is not deprecating but removing the support, which I am not
sure is a sensible thing to do.  Rather we should pretend that
core.fsync = "loose-object" (or "-loose-object") were found in the
configuration, shouldn't we?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Wed, Mar 9, 2022 at 4:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +/*
> > + * 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,
> > +     FSYNC_COMPONENT_LOOSE_OBJECT            = 1 << 0,
> > +     FSYNC_COMPONENT_PACK                    = 1 << 1,
> > +     FSYNC_COMPONENT_PACK_METADATA           = 1 << 2,
> > +     FSYNC_COMPONENT_COMMIT_GRAPH            = 1 << 3,
> > +};
>
> OK, so the idea is that Patrick's "we need to fsync refs" will be
> done by adding a new component to this list, and sprinkling a call
> to fsync_component_or_die() in the code of ref-files backend?
>

Yes. Patrick will need to add fsync_component_or_die wherever his
patch series has already added fsync_or_die.

If he follows Ævar's suggestion of treating remote refs differently
from local refs, he might want to define multiple components.

> I am wondering if fsync_or_die() interface is abstracted well
> enough, or we need things like "the fd is inside this directory; in
> addition to doing the fsync of the fd, please sync the parent
> directory as well" support before we start adding more components
> (if there is such a need, perhaps it comes before this step).
>

I think syncing the parent directory is a separate fsyncMethod that
would require changes across the codebase to obtain an appropriate
directory fd. I'd prefer to treat that as a separable concern.

> > +#define FSYNC_COMPONENTS_DEFAULT (FSYNC_COMPONENT_PACK | \
> > +                               FSYNC_COMPONENT_PACK_METADATA | \
> > +                               FSYNC_COMPONENT_COMMIT_GRAPH)
>
> IOW, everything other than loose object, which already has a
> separate core.fsyncObjectFiles knob to loosen.  Everything else we
> currently sync unconditionally and the default keeps that
> arrangement?
>

Yes, trying to keep default behavior identical on non-Windows
platforms.  Windows will be expected to adopt batch mode, and have
loose objects in this set.

> > +static inline void fsync_component_or_die(enum fsync_component component, int fd, const char *msg)
> > +{
> > +     if (fsync_components & component)
> > +             fsync_or_die(fd, msg);
> > +}
>
> Do we have a compelling reason to have this as a static inline
> function?  We are talking about concluding an I/O operation and
> I doubt there is a good performance argument for it.
>

Yeah, this is meant to optimize the case where the component isn't
being fsynced. I'll move this function to write-or-die.c below
fsync_or_die.

> > +static const struct fsync_component_entry {
> > +     const char *name;
> > +     enum fsync_component component_bits;
> > +} fsync_component_table[] = {
>
> thing[] is an array of "thing" (and thing[4] is the "fourth" such
> thing), but this is not an array of a table (it is a name-to-bit
> mapping).
>
> I wonder if this array works without "_table" suffix in its name.

This is modeled after whitespace_rule_names.  What if I change this to
the following?
static const struct fsync_component_name {
...
} fsync_component_names[]

>
> > +     { "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
> > +     { "pack", FSYNC_COMPONENT_PACK },
> > +     { "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
> > +     { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
> > +};
> > +
> > +static enum fsync_component parse_fsync_components(const char *var, const char *string)
> > +{
> > +     enum fsync_component output = 0;
> > +
> > +     if (!strcmp(string, "none"))
> > +             return FSYNC_COMPONENT_NONE;
> > +
> > +     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(_("ignoring unknown core.fsync component '%s'"), component);
> > +                     free(component);
> > +             }
> > +
> > +             string = ep;
> > +     }
> > +
> > +     return output;
> > +}
>
> Hmph.  I would have expected, with built-in default of
> pack,pack-metadata,commit-graph,
>

At the conclusion of this series, I defined 'default' as an aggregate
option that includes
the platform default.  I'd prefer not to have any statefulness of the
core.fsync setting so
that there is less confusion about the final fsync configuration. My
colleagues had a fair
amount of confusion internally when testing Git performance internally
with regards to
the core.fsyncObjectFiles setting.  Inline this is how your configs
would be written:

>  - "none,pack" would choose only "pack" by first clearing the
>    built-in default (or whatever was set in configuration files that
>    are lower precedence than what we are reading) and then OR'ing
>    the "pack" bit in.
>

"pack" would choose only "pack"

>  - "-pack" would choose "pack-metadata,commit-graph" by first
>    starting from the built-in default and then CLR'ing the "pack"
>    bit out.  If there were already changes made by the lower
>    precedence configuration files like /etc/gitconfig, the result
>    might be different and the only definite thing we can say is that
>    the pack bit is cleared.
>

"default,-pack" would be the platform default, but not packfiles.

>  - "loose-object" would choose all of the bits by first starting
>    from the built-in default and then OR'ing the "loose-object" bit
>    in.
>

"default,loose-object" would add loose objects to the platform config.

> Otherwise, parsing "none" is more or less pointless, as the above
> parser always start from 0 and OR's in or CLR's out the named bit.
> Whoever writes "none" can just write an empty string, no?

I think the empty string would be disallowed since "core.fsync=" would
be entirely
missing a value. But on testing this doesn't seem to be the case. I'll change
this to be more strict in that the user has to pass an explicit value,
such as 'none'.

>
> I wonder you'd rather want to do it this way?
>
> parse_fsync_components(var, value, current) {
>         enum fsync_component positive = 0, negative = 0;
>
>         while (string) {
>                 int negated = 0;
>                 enum fsync_component bits;
>
>                 parse out a single component into <negated, bits>;
>
>                 if (bits == 0) { /* "none" given */
>                         current = 0;
>                 } else if (negated) {
>                         negative |= bits;
>                 } else {
>                         positive |= bits;
>                 }
>                 advance <string> pointer;
>         }
>
>         return (current | positive) & ~negative;
> }
>
> And then ...
>
> > +     if (!strcmp(var, "core.fsync")) {
> > +             if (!value)
> > +                     return config_error_nonbool(var);
> > +             fsync_components = parse_fsync_components(var, value);
> > +             return 0;
> > +     }
> > +
>
> ... this part would pass the current value of fsync_components as
> the third parameter to the parse_fsync_components().  The variable
> would be initialized to the FSYNC_COMPONENTS_DEFAULT we saw earlier.
>

I'm afraid that this method would lead to statefulness between
different levels configuring
core.fsync.  I'd prefer that the user could know what will happen by
just inspecting the value
returned by `git config core.fsync`.

>
> > @@ -1613,7 +1684,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"));
>
> This is not deprecating but removing the support, which I am not
> sure is a sensible thing to do.  Rather we should pretend that
> core.fsync = "loose-object" (or "-loose-object") were found in the
> configuration, shouldn't we?
>

The problem I anticipate is that figuring out the final configuration
becomes pretty
complex in the face of conflicting configurations of fsyncObjectFiles
and core.fsync.
The user won't know what will happen without reading the Git code or
doing performance
experiments.  I thought we can avoid all of this complexity by just
having a simple warning
that pushes users toward the new configuration value.  Aside from
seeing a warning, a
user's actual usage of Git functionality shouldn't be affected.

Alternatively, what if we just silently ignore the old
core.fsyncObjectFiles setting?
If neither of these is an option, I'll put back some support for the
old setting.

Thanks,
Neeraj

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Neeraj Singh <nksingh85@gmail.com> writes:

> This is modeled after whitespace_rule_names.  What if I change this to
> the following?
> static const struct fsync_component_name {
> ...
> } fsync_component_names[]

That's better.

> At the conclusion of this series, I defined 'default' as an aggregate
> option that includes
> the platform default.  I'd prefer not to have any statefulness of the
> core.fsync setting so
> that there is less confusion about the final fsync configuration.

Then scratch your preference ;-) 

Our configuration files are designed to be "hierarchical" in that
system-wide default can be set in /etc/gitconfig, which can be
overridden by per-user default in $HOME/.gitconfig, which can in
turn be overridden by per-repository setting in .git/config, so
starting from the compiled-in default, reading/augmenting "the value
we tentatively decided based on what we have read so far" with what
we read from lower-precedence files to higher-precedence files is a
norm.

Don't make this little corner of the system different from
everything else; that will only confuse users.

The git_config() callback should expect to see the same var with
different values for that reason.  Always restarting from zero will
defeat it.

And always restarting from zero will mean "none" is meaningless,
while it would be a quite nice way to say "forget everything we have
read so far and start from scratch" when you really want to refuse
what the system default wants to give you.

>> > @@ -1613,7 +1684,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"));
>>
>> This is not deprecating but removing the support, which I am not
>> sure is a sensible thing to do.  Rather we should pretend that
>> core.fsync = "loose-object" (or "-loose-object") were found in the
>> configuration, shouldn't we?
>>
>
> The problem I anticipate is that figuring out the final configuration
> becomes pretty
> complex in the face of conflicting configurations of fsyncObjectFiles
> and core.fsync.

Don't start your thinking from too complex configuration that mixes
and matches.  Instead, think what happens to folks who are *NOT*
interested in the new way of doing this.  They aren't interested in
setting core.fsync, and they already have core.fsyncObjectFiles set.
You want to make sure their experience does not suck.

The quoted code simply _IGNORES_ their wish and forces whatever
default configuration core.fsync codepath happens to choose, which
is a grave regression from their point of view.

One way to handle this more gracefully is to delay the final
decision until the end of the configuraiton file processing, and
keep track of core.fsyncObjectFiles and core.fsync separately.  If
the latter is never set but the former is, then you are dealing with
such a user who hasn't migrated.  Give them a warning (the text
above is fine---we can tell them "that's deprecated and you should
use the other one instead"), but in the meantime, until deprecation
turns into removal of support, keep honoring their original wish.
If core.fsync is set to something, you can still give them a warning
when you see core.fsyncObjectFiles, saying "that's deprecated and
because you have core.fsync, we'll ignore the old one", and use the
new method exclusively, without having to worry about mixing.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Neeraj,

On Wed, 9 Mar 2022, Neeraj Singh wrote:

> On Wed, Mar 9, 2022 at 4:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> > I am wondering if fsync_or_die() interface is abstracted well enough,
> > or we need things like "the fd is inside this directory; in addition
> > to doing the fsync of the fd, please sync the parent directory as
> > well" support before we start adding more components (if there is such
> > a need, perhaps it comes before this step).
> >
>
> I think syncing the parent directory is a separate fsyncMethod that
> would require changes across the codebase to obtain an appropriate
> directory fd. I'd prefer to treat that as a separable concern.

That makes sense to me because I expect further abstraction to be
necessary here because Unix/Linux semantics differ quite a bit more from
Windows semantics when it comes to directory "file" descriptors than when
talking about files' file descriptors.

> > > +#define FSYNC_COMPONENTS_DEFAULT (FSYNC_COMPONENT_PACK | \
> > > +                               FSYNC_COMPONENT_PACK_METADATA | \
> > > +                               FSYNC_COMPONENT_COMMIT_GRAPH)
> >
> > IOW, everything other than loose object, which already has a
> > separate core.fsyncObjectFiles knob to loosen.  Everything else we
> > currently sync unconditionally and the default keeps that
> > arrangement?
> >
>
> Yes, trying to keep default behavior identical on non-Windows
> platforms.  Windows will be expected to adopt batch mode, and have
> loose objects in this set.

We already adopted an early version of this patch series:
https://github.com/git-for-windows/git/commit/98209a5f6e4

And yes, I will gladly adapt that to whatever lands in core Git.

Thank you _so_ much for working on this!
Dscho

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Neeraj Singh <nksingh85@gmail.com> writes:

>> I am wondering if fsync_or_die() interface is abstracted well
>> enough, or we need things like "the fd is inside this directory; in
>> addition to doing the fsync of the fd, please sync the parent
>> directory as well" support before we start adding more components
>> (if there is such a need, perhaps it comes before this step).
>>
>
> I think syncing the parent directory is a separate fsyncMethod that
> would require changes across the codebase to obtain an appropriate
> directory fd. I'd prefer to treat that as a separable concern.

Yeah, that would be a sensible direction to go.  If we never did the
"sync the parent" thing, we do not need it in the fsyncMethod world
immediately.  It can be added later.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Wed, Mar 9, 2022 at 11:19 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Neeraj Singh <nksingh85@gmail.com> writes:
>
> > This is modeled after whitespace_rule_names.  What if I change this to
> > the following?
> > static const struct fsync_component_name {
> > ...
> > } fsync_component_names[]
>
> That's better.
>
> > At the conclusion of this series, I defined 'default' as an aggregate
> > option that includes
> > the platform default.  I'd prefer not to have any statefulness of the
> > core.fsync setting so
> > that there is less confusion about the final fsync configuration.
>
> Then scratch your preference ;-)

Just to clarify, linguistically, by 'scratch' do you mean that I should drop
my preference (which I can do to get the important parts of the
series in), or are you saying that that your suggestion is in line with
my preference, and I'm just not seeing it properly?

> Our configuration files are designed to be "hierarchical" in that
> system-wide default can be set in /etc/gitconfig, which can be
> overridden by per-user default in $HOME/.gitconfig, which can in
> turn be overridden by per-repository setting in .git/config, so
> starting from the compiled-in default, reading/augmenting "the value
> we tentatively decided based on what we have read so far" with what
> we read from lower-precedence files to higher-precedence files is a
> norm.
>
> Don't make this little corner of the system different from
> everything else; that will only confuse users.
>
> The git_config() callback should expect to see the same var with
> different values for that reason.  Always restarting from zero will
> defeat it.
>

Consider core.whitespace. The parse_whitespace_rule code starts with
the compiled-in default every time it encounters a config value, and then
modifies it according to what the user passed in.  So the user could figure
out what's going to happen by just looking at the value returned by
`git config --get core.whitespace`.  The user doesn't need to call
`git config --get-all core.whitespace` and then reason about the entries
from top to bottom to figure out the actual state that Git will use.

> And always restarting from zero will mean "none" is meaningless,
> while it would be a quite nice way to say "forget everything we have
> read so far and start from scratch" when you really want to refuse
> what the system default wants to give you.
>

The intention, which I've implemented in my local v6 changes, is for an
empty list or empty string to be an illegal value of core.fsync.  It should be
set to one or more legal values.  But I see the advantage in always resetting to
the system default, like core.whitespace does, so that a set of unrecognized
values results in at least default behavior. An empty string would mean
'unconfigured', which will be meaningful when we integrate core.fsync with
core.fsyncObjectFiles.

I'll update the change to start from default, with none as a reset. I'm still in
favor of making it so that the most recent value of core.fsync in the
hierarchical configuration store stands alone without picking up state from
prior values.

> >> > @@ -1613,7 +1684,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"));
> >>
> >> This is not deprecating but removing the support, which I am not
> >> sure is a sensible thing to do.  Rather we should pretend that
> >> core.fsync = "loose-object" (or "-loose-object") were found in the
> >> configuration, shouldn't we?
> >>
> >
> > The problem I anticipate is that figuring out the final configuration
> > becomes pretty
> > complex in the face of conflicting configurations of fsyncObjectFiles
> > and core.fsync.
>
> Don't start your thinking from too complex configuration that mixes
> and matches.  Instead, think what happens to folks who are *NOT*
> interested in the new way of doing this.  They aren't interested in
> setting core.fsync, and they already have core.fsyncObjectFiles set.
> You want to make sure their experience does not suck.
>
> The quoted code simply _IGNORES_ their wish and forces whatever
> default configuration core.fsync codepath happens to choose, which
> is a grave regression from their point of view.
>
> One way to handle this more gracefully is to delay the final
> decision until the end of the configuraiton file processing, and
> keep track of core.fsyncObjectFiles and core.fsync separately.  If
> the latter is never set but the former is, then you are dealing with
> such a user who hasn't migrated.  Give them a warning (the text
> above is fine---we can tell them "that's deprecated and you should
> use the other one instead"), but in the meantime, until deprecation
> turns into removal of support, keep honoring their original wish.
> If core.fsync is set to something, you can still give them a warning
> when you see core.fsyncObjectFiles, saying "that's deprecated and
> because you have core.fsync, we'll ignore the old one", and use the
> new method exclusively, without having to worry about mixing.

Is there a well-defined place where we know that configuration processing
is complete?  The most obvious spot to me to integrate these two values would
be the first time we need to figure out the fsync state. Another spot would be
prepare_repo_settings.  Are there any other good candidates?

Once the right spot is picked, I'll implement the integration of the
settings as you
suggested.  For now I'll stick it in prepare_repo_settings.

Thanks for the review.  Please expect a v6 today.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Neeraj Singh <nksingh85@gmail.com> writes:

>> > At the conclusion of this series, I defined 'default' as an aggregate
>> > option that includes
>> > the platform default.  I'd prefer not to have any statefulness of the
>> > core.fsync setting so
>> > that there is less confusion about the final fsync configuration.
>>
>> Then scratch your preference ;-)
>
> Just to clarify, linguistically, by 'scratch' do you mean that I should drop
> my preference

Yes.

> Is there a well-defined place where we know that configuration processing
> is complete?  The most obvious spot to me to integrate these two values would
> be the first time we need to figure out the fsync state.

That sounds like a good place.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes:

> Neeraj Singh <nksingh85@gmail.com> writes:
>
>>> > At the conclusion of this series, I defined 'default' as an aggregate
>>> > option that includes
>>> > the platform default.  I'd prefer not to have any statefulness of the
>>> > core.fsync setting so
>>> > that there is less confusion about the final fsync configuration.
>>>
>>> Then scratch your preference ;-)
>>
>> Just to clarify, linguistically, by 'scratch' do you mean that I should drop
>> my preference
>
> Yes.

Let me take this part back.

I do not mind too deeply if this were "each occurrence of core.fsync
as a whole replaces whatever we saw earlier, i.e. last-one-wins".

But if we were going that route, instead of starting from an empty
set, I'd prefer to see it begin with the built-in default (i.e. the
one you defined to mimic the traditional behaviour before core.fsync
was introduced) and added or deleted by each (possibly '-' prefixed)
element on the comma-separated list, with an explicit way to clear
the built-in default.  E.g. "none,refs" would clear the components
traditionally fsync'ed by default and choose only "refs" component,
while "-pack-metadata" would mean the default ones minus
"pack-metadata" component are subject for fsync'ing.  An empty
string would naturally mean "By having this core.fsync entry, I am
telling you not to pay any attention to what lower-precedence
configuration files said.  But I want the built-in default, without
any additions or subtractions made by this entry, just the default,
please" in such a scheme, so do not forbid it.

Or, we can inherit from the previous configuration file to allow
/etc/gitconfig and the ones shipped by Git for Windows to augment
the built-in default before letting end-user configuration to
further customize the preference.

Either is fine by me.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Thu, Mar 10, 2022 at 11:57 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Neeraj Singh <nksingh85@gmail.com> writes:
> >
> >>> > At the conclusion of this series, I defined 'default' as an aggregate
> >>> > option that includes
> >>> > the platform default.  I'd prefer not to have any statefulness of the
> >>> > core.fsync setting so
> >>> > that there is less confusion about the final fsync configuration.
> >>>
> >>> Then scratch your preference ;-)
> >>
> >> Just to clarify, linguistically, by 'scratch' do you mean that I should drop
> >> my preference
> >
> > Yes.
>
> Let me take this part back.
>
> I do not mind too deeply if this were "each occurrence of core.fsync
> as a whole replaces whatever we saw earlier, i.e. last-one-wins".
>
> But if we were going that route, instead of starting from an empty
> set, I'd prefer to see it begin with the built-in default (i.e. the
> one you defined to mimic the traditional behaviour before core.fsync
> was introduced) and added or deleted by each (possibly '-' prefixed)
> element on the comma-separated list, with an explicit way to clear
> the built-in default.  E.g. "none,refs" would clear the components
> traditionally fsync'ed by default and choose only "refs" component,
> while "-pack-metadata" would mean the default ones minus
> "pack-metadata" component are subject for fsync'ing.  An empty
> string would naturally mean "By having this core.fsync entry, I am
> telling you not to pay any attention to what lower-precedence
> configuration files said.  But I want the built-in default, without
> any additions or subtractions made by this entry, just the default,
> please" in such a scheme, so do not forbid it.
>
> Or, we can inherit from the previous configuration file to allow
> /etc/gitconfig and the ones shipped by Git for Windows to augment
> the built-in default before letting end-user configuration to
> further customize the preference.
>
> Either is fine by me.
>
> Thanks.

Okay, I'll implement this version, since this is close to my preference.

Under this schema, 'default' isn't useful as an aggregate option, so
I'll eliminate
that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Neeraj Singh <nksingh85@gmail.com> writes:

> Under this schema, 'default' isn't useful as an aggregate option, so
> I'll eliminate
> that.

Yeah, the only difference is the starting point.  Either start with
default set of bits and give an option to clear, or start with an
empty set and give an option to set default.  The former may be a
bit less cumbersome to users but there isn't a huge difference.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jiang Xin wrote (reply to this):

On Sat, Mar 12, 2022 at 6:25 AM Neeraj Singh via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> @@ -1613,6 +1687,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
>         }
>
>         if (!strcmp(var, "core.fsyncobjectfiles")) {
> +               if (fsync_object_files < 0)
> +                       warning(_("core.fsyncobjectfiles is deprecated; use core.fsync instead"));

s/core.fsyncobjectfiles/core.fsyncObjectFiles/  to use bumpyCaps for
config variable in documentation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Mon, Mar 28, 2022 at 4:07 AM Jiang Xin <worldhello.net@gmail.com> wrote:
>
> On Sat, Mar 12, 2022 at 6:25 AM Neeraj Singh via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > @@ -1613,6 +1687,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
> >         }
> >
> >         if (!strcmp(var, "core.fsyncobjectfiles")) {
> > +               if (fsync_object_files < 0)
> > +                       warning(_("core.fsyncobjectfiles is deprecated; use core.fsync instead"));
>
> s/core.fsyncobjectfiles/core.fsyncObjectFiles/  to use bumpyCaps for
> config variable in documentation.

THanks for pointing this out.  I'll fix it.

errors. The default tab width is 8. Allowed values are 1 to 63.

core.fsync::
A comma-separated list of components of the repository that
should be hardened via the core.fsyncMethod when created or
modified. You can disable hardening of any component by
prefixing it with a '-'. Items that are not hardened may be
lost in the event of an unclean system shutdown. Unless you
have special requirements, it is recommended that you leave
this option empty or pick one of `committed`, `added`,
or `all`.
+
When this configuration is encountered, the set of components starts with
the platform default value, disabled components are removed, and additional
components are added. `none` resets the state so that the platform default
is ignored.
+
The empty string resets the fsync configuration to the platform
default. The default on most platforms is equivalent to
`core.fsync=committed,-loose-object`, which has good performance,
but risks losing recent work in the event of an unclean system shutdown.
+
* `none` clears the set of fsynced components.
* `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.
* `index` hardens the index when it is modified.
* `objects` is an aggregate option that is equivalent to
`loose-object,pack`.
* `derived-metadata` is an aggregate option that is equivalent to
`pack-metadata,commit-graph`.
* `committed` is an aggregate option that is currently equivalent to
`objects`. This mode sacrifices some performance to ensure that work
that is committed to the repository with `git commit` or similar commands
is hardened.
* `added` is an aggregate option that is currently equivalent to
`committed,index`. This mode sacrifices additional performance to
ensure that the results of commands like `git add` and similar operations
are hardened.
* `all` is an aggregate option that syncs all individual components above.

core.fsyncMethod::
A value indicating the strategy Git will use to harden repository data
using fsync and related primitives.
+
* `fsync` uses the fsync() system call or platform equivalents.
* `writeout-only` issues pagecache writeback requests, but depending on the
filesystem and storage hardware, data added to the repository may not be
durable in the event of a system crash. This is the default mode on macOS.

core.fsyncObjectFiles::
This boolean will enable 'fsync()' when writing object files.
This setting is deprecated. Use core.fsync instead.
+
This is a total waste of time and effort on a filesystem that orders
data writes properly, but can be useful for filesystems that do not use
journalling (traditional UNIX filesystems) or that only journal metadata
and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").
This setting affects data added to the Git repository in loose-object
form. When set to true, Git will issue an fsync or similar system call
to flush caches so that loose-objects remain consistent in the face
of a unclean system shutdown.

core.preloadIndex::
Enable parallel index preload for operations like 'git diff'
Expand Down
6 changes: 6 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,8 @@ all::
#
# Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC.
#
# Define HAVE_SYNC_FILE_RANGE if your platform has sync_file_range.
#
# Define NEEDS_LIBRT if your platform requires linking with librt (glibc version
# before 2.17) for clock_gettime and CLOCK_MONOTONIC.
#
Expand Down Expand Up @@ -1897,6 +1899,10 @@ ifdef HAVE_CLOCK_MONOTONIC
BASIC_CFLAGS += -DHAVE_CLOCK_MONOTONIC
endif

ifdef HAVE_SYNC_FILE_RANGE
BASIC_CFLAGS += -DHAVE_SYNC_FILE_RANGE
endif

ifdef NEEDS_LIBRT
EXTLIBS += -lrt
endif
Expand Down
2 changes: 1 addition & 1 deletion builtin/fast-import.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,7 @@ static void end_packfile(void)
struct tag *t;

close_pack_windows(pack_data);
finalize_hashfile(pack_file, cur_pack_oid.hash, 0);
finalize_hashfile(pack_file, cur_pack_oid.hash, FSYNC_COMPONENT_PACK, 0);
fixup_pack_header_footer(pack_data->pack_fd, pack_data->hash,
pack_data->pack_name, object_count,
cur_pack_oid.hash, pack_size);
Expand Down
4 changes: 2 additions & 2 deletions builtin/index-pack.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1290,7 +1290,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,
Expand Down Expand Up @@ -1512,7 +1512,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"));
Expand Down
24 changes: 17 additions & 7 deletions builtin/pack-objects.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1199,16 +1199,26 @@ static void write_pack_file(void)
display_progress(progress_state, written);
}

/*
* Did we write the wrong # entries in the header?
* If so, rewrite it like in fast-import
*/
if (pack_to_stdout) {
finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE);
/*
* We never fsync when writing to stdout since we may
* not be writing to an actual pack file. For instance,
* the upload-pack code passes a pipe here. Calling
* fsync on a pipe results in unnecessary
* synchronization with the reader on some platforms.
*/
finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE,
CSUM_HASH_IN_STREAM | CSUM_CLOSE);
} else if (nr_written == nr_remaining) {
finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK,
CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
} else {
int fd = finalize_hashfile(f, hash, 0);
/*
* If we wrote the wrong number of entries in the
* header, rewrite it like in fast-import.
*/

int fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0);
fixup_pack_header_footer(fd, hash, pack_tmp_name,
nr_written, hash, offset);
close(fd);
Expand Down
5 changes: 3 additions & 2 deletions bulk-checkin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)
unlink(state->pack_tmp_name);
goto clear_exit;
} else if (state->nr_written == 1) {
finalize_hashfile(state->f, hash, CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK,
CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
} else {
int fd = finalize_hashfile(state->f, hash, 0);
int fd = finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK, 0);
fixup_pack_header_footer(fd, hash, state->pack_tmp_name,
state->nr_written, hash,
state->offset);
Expand Down
48 changes: 48 additions & 0 deletions cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -993,8 +993,54 @@ void reset_shared_repository(void);
extern int read_replace_refs;
extern char *git_replace_ref_base;

/*
* 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,
FSYNC_COMPONENT_LOOSE_OBJECT = 1 << 0,
FSYNC_COMPONENT_PACK = 1 << 1,
FSYNC_COMPONENT_PACK_METADATA = 1 << 2,
FSYNC_COMPONENT_COMMIT_GRAPH = 1 << 3,
FSYNC_COMPONENT_INDEX = 1 << 4,
};

#define FSYNC_COMPONENTS_OBJECTS (FSYNC_COMPONENT_LOOSE_OBJECT | \
FSYNC_COMPONENT_PACK)

#define FSYNC_COMPONENTS_DERIVED_METADATA (FSYNC_COMPONENT_PACK_METADATA | \
FSYNC_COMPONENT_COMMIT_GRAPH)

#define FSYNC_COMPONENTS_DEFAULT (FSYNC_COMPONENTS_OBJECTS | \
FSYNC_COMPONENTS_DERIVED_METADATA | \
~FSYNC_COMPONENT_LOOSE_OBJECT)

#define FSYNC_COMPONENTS_COMMITTED (FSYNC_COMPONENTS_OBJECTS)

#define FSYNC_COMPONENTS_ADDED (FSYNC_COMPONENTS_COMMITTED | \
FSYNC_COMPONENT_INDEX)

#define FSYNC_COMPONENTS_ALL (FSYNC_COMPONENT_LOOSE_OBJECT | \
FSYNC_COMPONENT_PACK | \
FSYNC_COMPONENT_PACK_METADATA | \
FSYNC_COMPONENT_COMMIT_GRAPH | \
FSYNC_COMPONENT_INDEX)

/*
* A bitmask indicating which components of the repo should be fsynced.
*/
extern enum fsync_component fsync_components;
extern int fsync_object_files;
extern int use_fsync;

enum fsync_method {
FSYNC_METHOD_FSYNC,
FSYNC_METHOD_WRITEOUT_ONLY
};

extern enum fsync_method fsync_method;
extern int core_preload_index;
extern int precomposed_unicode;
extern int protect_hfs;
Expand Down Expand Up @@ -1700,6 +1746,8 @@ int copy_file_with_time(const char *dst, const char *src, int mode);

void write_or_die(int fd, const void *buf, size_t count);
void fsync_or_die(int fd, const char *);
int fsync_component(enum fsync_component component, int fd);
void fsync_component_or_die(enum fsync_component component, int fd, const char *msg);

ssize_t read_in_full(int fd, void *buf, size_t count);
ssize_t write_in_full(int fd, const void *buf, size_t count);
Expand Down
3 changes: 2 additions & 1 deletion commit-graph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1942,7 +1942,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
}

close_commit_graph(ctx->r->objects);
finalize_hashfile(f, file_hash, CSUM_HASH_IN_STREAM | CSUM_FSYNC);
finalize_hashfile(f, file_hash, FSYNC_COMPONENT_COMMIT_GRAPH,
CSUM_HASH_IN_STREAM | CSUM_FSYNC);
free_chunkfile(cf);

if (ctx->split) {
Expand Down
3 changes: 3 additions & 0 deletions compat/mingw.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
28 changes: 28 additions & 0 deletions compat/win32/flush.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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, NTAPI, NtFlushBuffersFileEx,
HANDLE FileHandle, ULONG Flags, PVOID Parameters, ULONG ParameterSize,
PIO_STATUS_BLOCK IoStatusBlock);

if (!INIT_PROC_ADDR(NtFlushBuffersFileEx)) {
errno = ENOSYS;
return -1;
}

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;
}
5 changes: 0 additions & 5 deletions compat/winansi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,6 @@

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Including NTSecAPI.h in git-compat-util.h causes build errors in any
> other file that includes winternl.h. That file was included in order to
> get access to the RtlGenRandom cryptographically secure PRNG. This
> change scopes the inclusion of all PRNG headers to just the wrapper.c
> file, which is the only place it is really needed.

It is true that wrapper.c is the only thing that needs these headers
included as part of its implementation detail of csprng_bytes(), and
I think I very much like this change for that reason.

Having said that, if it true that including these two header files
in the same file will lead to compilation failure?  That sounds like
either (1) they represent two mutually incompatible APIs that will
cause breakage when code that use them, perhaps in two separate
files to avoid compilation failures, are linked together, or (2)
these system header files are simply broken.  Or something else?

> -/*
> - * Including the appropriate header file for RtlGenRandom causes MSVC to see a
> - * redefinition of types in an incompatible way when including headers below.
> - */
> -#undef HAVE_RTLGENRANDOM

This comment hints it is more like (1) above?  A type used in one
part of the system is defined differently in other parts of the
system?  I cannot imagine anything but bad things happen when a
piece of code uses one definition of the type to declare a function,
and another piece of code uses the other definition of the same type
to declare a variable and passes it as a parameter to that function.

I do not know this patch makes the situation worse, and I am not a
Windows person with boxes to dig deeper to begin with.  Hence I do
not mind the change itself, but justifying the change primarily as a
workaround for some obscure header type clashes on a single system
leaves a bad taste.  If the first sentence weren't there, I wouldn't
have spent this many minutes wondering if this is a good change ;-)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Wed, Mar 9, 2022 at 3:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Including NTSecAPI.h in git-compat-util.h causes build errors in any
> > other file that includes winternl.h. That file was included in order to
> > get access to the RtlGenRandom cryptographically secure PRNG. This
> > change scopes the inclusion of all PRNG headers to just the wrapper.c
> > file, which is the only place it is really needed.
>
> It is true that wrapper.c is the only thing that needs these headers
> included as part of its implementation detail of csprng_bytes(), and
> I think I very much like this change for that reason.
>
> Having said that, if it true that including these two header files
> in the same file will lead to compilation failure?  That sounds like
> either (1) they represent two mutually incompatible APIs that will
> cause breakage when code that use them, perhaps in two separate
> files to avoid compilation failures, are linked together, or (2)
> these system header files are simply broken.  Or something else?
>
> > -/*
> > - * Including the appropriate header file for RtlGenRandom causes MSVC to see a
> > - * redefinition of types in an incompatible way when including headers below.
> > - */
> > -#undef HAVE_RTLGENRANDOM
>
> This comment hints it is more like (1) above?  A type used in one
> part of the system is defined differently in other parts of the
> system?  I cannot imagine anything but bad things happen when a
> piece of code uses one definition of the type to declare a function,
> and another piece of code uses the other definition of the same type
> to declare a variable and passes it as a parameter to that function.
>
> I do not know this patch makes the situation worse, and I am not a
> Windows person with boxes to dig deeper to begin with.  Hence I do
> not mind the change itself, but justifying the change primarily as a
> workaround for some obscure header type clashes on a single system
> leaves a bad taste.  If the first sentence weren't there, I wouldn't
> have spent this many minutes wondering if this is a good change ;-)

This is (2), these system header files are simply broken.  I've been
looking deeper into why, but haven't bottomed out yet.

Thanks,
Neeraj

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, "brian m. carlson" wrote (reply to this):

On 2022-03-09 at 23:03:14, Neeraj Singh via GitGitGadget wrote:
> From: Neeraj Singh <neerajsi@microsoft.com>
> 
> Including NTSecAPI.h in git-compat-util.h causes build errors in any
> other file that includes winternl.h. That file was included in order to
> get access to the RtlGenRandom cryptographically secure PRNG. This
> change scopes the inclusion of all PRNG headers to just the wrapper.c
> file, which is the only place it is really needed.

We generally prefer to do system includes in git-compat-util.h because
it allows us to paper over platform incompatibilities in one place and
to deal with the various ordering problems that can happen on certain
systems.

It may be that Windows needs additional help here; I don't know, because
I don't use Windows.  I personally find it unsavoury that Windows ships
with multiple incompatible header files like this, since such problems
are typically avoided by suitable include guards, whose utility has been
well known for several decades.  However, if that's the case, let's move
only the Windows changes there, and leave the Unix systems, which lack
this problem, alone.

It would also be helpful to explain the problem that Windows has in more
detail here, including any references to documentation that explains
this incompatibility, so those of us who are not Windows users can more
accurately reason about why we need to be so careful when including
header files there and why this is the best solution (and not, say,
providing our own include guards in a compat file).
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Wed, Mar 9, 2022 at 5:26 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2022-03-09 at 23:03:14, Neeraj Singh via GitGitGadget wrote:
> > From: Neeraj Singh <neerajsi@microsoft.com>
> >
> > Including NTSecAPI.h in git-compat-util.h causes build errors in any
> > other file that includes winternl.h. That file was included in order to
> > get access to the RtlGenRandom cryptographically secure PRNG. This
> > change scopes the inclusion of all PRNG headers to just the wrapper.c
> > file, which is the only place it is really needed.
>
> We generally prefer to do system includes in git-compat-util.h because
> it allows us to paper over platform incompatibilities in one place and
> to deal with the various ordering problems that can happen on certain
> systems.
>
> It may be that Windows needs additional help here; I don't know, because
> I don't use Windows.  I personally find it unsavoury that Windows ships
> with multiple incompatible header files like this, since such problems
> are typically avoided by suitable include guards, whose utility has been
> well known for several decades.  However, if that's the case, let's move
> only the Windows changes there, and leave the Unix systems, which lack
> this problem, alone.
>
> It would also be helpful to explain the problem that Windows has in more
> detail here, including any references to documentation that explains
> this incompatibility, so those of us who are not Windows users can more
> accurately reason about why we need to be so careful when including
> header files there and why this is the best solution (and not, say,
> providing our own include guards in a compat file).
> --
> brian m. carlson (he/him or they/them)
> Toronto, Ontario, CA

I wasn't able to find any documentation from other people who hit this problem.

The root cause is that NtSecAPI.h has a typedef like this:
```
#ifndef _NTDEF_
typedef LSA_UNICODE_STRING UNICODE_STRING, *PUNICODE_STRING;
typedef LSA_STRING STRING, *PSTRING ;
#endif
```
That's not really appropriate since NtSecAPI.h isn't the correct place
to define this core primitive NT type.  It should be including
winternl.h or a similar header.

I'll update the change to only move the Windows definition to the .c file.

#undef NOGDI

/*
* Including the appropriate header file for RtlGenRandom causes MSVC to see a
* redefinition of types in an incompatible way when including headers below.
*/
#undef HAVE_RTLGENRANDOM
#include "../git-compat-util.h"
#include <wingdi.h>
#include <winreg.h>
Expand Down
94 changes: 94 additions & 0 deletions config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1323,6 +1323,79 @@ static int git_parse_maybe_bool_text(const char *value)
return -1;
}

static const struct fsync_component_name {
const char *name;
enum fsync_component component_bits;
} fsync_component_names[] = {
{ "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
{ "pack", FSYNC_COMPONENT_PACK },
{ "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
{ "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
{ "index", FSYNC_COMPONENT_INDEX },
{ "objects", FSYNC_COMPONENTS_OBJECTS },
{ "derived-metadata", FSYNC_COMPONENTS_DERIVED_METADATA },
{ "committed", FSYNC_COMPONENTS_COMMITTED },
{ "added", FSYNC_COMPONENTS_ADDED },
{ "all", FSYNC_COMPONENTS_ALL },
};

static enum fsync_component parse_fsync_components(const char *var, const char *string)
{
enum fsync_component current = FSYNC_COMPONENTS_DEFAULT;
enum fsync_component positive = 0, negative = 0;

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 (!strcmp(string, "none")) {
current = FSYNC_COMPONENT_NONE;
goto next_name;
}

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_names); ++i) {
const struct fsync_component_name *n = &fsync_component_names[i];

if (strncmp(n->name, string, len))
continue;

found = 1;
if (negated)
negative |= n->component_bits;
else
positive |= n->component_bits;
}

if (!found) {
char *component = xstrndup(string, len);
warning(_("ignoring unknown core.fsync component '%s'"), component);
free(component);
}

next_name:
string = ep;
}

return (current & ~negative) | positive;
}

int git_parse_maybe_bool(const char *value)
{
int v = git_parse_maybe_bool_text(value);
Expand Down Expand Up @@ -1600,7 +1673,28 @@ 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);
if (!strcmp(value, "fsync"))
fsync_method = FSYNC_METHOD_FSYNC;
else if (!strcmp(value, "writeout-only"))
fsync_method = FSYNC_METHOD_WRITEOUT_ONLY;
else
warning(_("ignoring unknown core.fsyncMethod value '%s'"), value);

}

if (!strcmp(var, "core.fsyncobjectfiles")) {
if (fsync_object_files < 0)
warning(_("core.fsyncobjectfiles is deprecated; use core.fsync instead"));
fsync_object_files = git_config_bool(var, value);
return 0;
}
Expand Down
Loading