Re: do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Mar 26 2022, Neeraj Singh wrote:

> On Sat, Mar 26, 2022 at 8:34 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Fri, Mar 25 2022, Neeraj Singh wrote:
>>
>> > On Fri, Mar 25, 2022 at 5:33 PM Ævar Arnfjörð Bjarmason
>> > <avarab@gmail.com> wrote:
[...]
>> > I want to make a comment about the Index here.  Syncing the index is
>> > strictly required for the "added" level of consistency, so that we
>> > don't lose stuff that leaves the work tree but is staged.  But my
>> > Windows enlistment has an index that's 266MB, which would be painful
>> > to sync even with all the optimizations.  Maybe with split-index, this
>> > wouldn't be so bad, but I just wanted to call out that some advanced
>> > users may really care about the configurability.
>>
>> So for that use-case you'd like to fsync the loose objects (if any), but
>> not the index? So the FS will "flush" up to the index, and then queue
>> the index for later syncing to platter?
>>
>>
>> But even in that case don't the settings need to be tied to one another,
>> because in the method=bulk sync=index && sync=!loose case wouldn't we be
>> syncing "loose" in any case?
>>
>> > As Git's various database implementations improve, the fsync stuff
>> > will hopefully be more optimal and self-tuning.  But as that happens,
>> > Git could just start ignoring settings that lose meaning without tying
>> > anyones hands.
>>
>> Yeah that would alleviate most of my concerns here, but the docs aren't
>> saying anything like that. Since you added them & they just landed, do
>> you mind doing a small follow-up where we e.g. say that these new
>> settings are "EXPERIMENTAL" or whatever, and subject to drastic change?
>
> The doc is already pretty prescriptive.  It has this line at the end
> of the first  paragraph:
> "Unless you
> have special requirements, it is recommended that you leave
> this option empty or pick one of `committed`, `added`,
> or `all`."
>
> Those values are already designed to change as Git changes.

I'm referring to the documentation as it stands not being marked as
experimental in the sense that we might decide to re-do this to a large
extent, i.e. something like the diff I suggested upthread in
https://lore.kernel.org/git/220323.86fsn8ohg8.gmgdl@evledraar.gmail.com/

So yes, I agree that it e.g. clearly states that you can add a new
core.git=foobar or whatever down the line, but it clearly doesn't
suggest that e.g. core.fsync might have boolean semantics in some later
version, or that the rest might simply be ignored, even if that
e.g. means that we wouldn't sync loose objects on
core.fsync=loose-object, as we'd just warn with a "we don't provide this
anymore".

Or do you disagree with that? IOW I mean that we'd do something like
this, either in docs or code:

diff --git a/config.c b/config.c
index 3c9b6b589ab..94548566073 100644
--- a/config.c
+++ b/config.c
@@ -1675,6 +1675,9 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "core.fsync")) {
+		if (!the_repository->settings.feature_experimental)
+			warning(_("the '%s' configuration option is EXPERIMENTAL. opt-in to use it with feature.experimental=true"),
+				var);
 		if (!value)
 			return config_error_nonbool(var);
 		fsync_components = parse_fsync_components(var, value);
@@ -1682,6 +1685,9 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "core.fsyncmethod")) {
+		if (!the_repository->settings.feature_experimental)
+			warning(_("the '%s' configuration option is EXPERIMENTAL. opt-in to use it with feature.experimental=true"),
+				var);
 		if (!value)
 			return config_error_nonbool(var);
 		if (!strcmp(value, "fsync"))
diff --git a/repo-settings.c b/repo-settings.c
index b4fbd16cdcc..f949b65b91e 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -31,6 +31,7 @@ void prepare_repo_settings(struct repository *r)
 	/* Booleans config or default, cascades to other settings */
 	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
 	repo_cfg_bool(r, "feature.experimental", &experimental, 0);
+	r->settings.feature_experimental = experimental;
 
 	/* Defaults modified by feature.* */
 	if (experimental) {
diff --git a/repository.h b/repository.h
index e29f361703d..db8f99a8989 100644
--- a/repository.h
+++ b/repository.h
@@ -28,6 +28,7 @@ enum fetch_negotiation_setting {
 struct repo_settings {
 	int initialized;
 
+	int feature_experimental;
 	int core_commit_graph;
 	int commit_graph_read_changed_paths;
 	int gc_write_commit_graph;