Re: [PATCH v5 3/5] core.fsync: introduce granular fsync control

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2022, 07:19 UTC
Message-ID: <xmqqcziugtpw.fsf@gitster.g>
In-Reply-To: <CANQDOddU_WXD-6ncDGBrgpsuKT-XDGC=SeaaQTNQFdODFZ7TkQ@mail.gmail.com>

Neeraj Singh <nksingh85@gmail.com> writes:

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 17 quoted lines

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