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

[Junio C Hamano <gitster@pobox.com>]

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.