Subject: Re: [PATCH v2 2/3] core.fsync: introduce granular fsync control

From: Junio C Hamano <[email-masked]>
Date: Wed, 08 Dec 2021 16:44:01 -0800

Neeraj Singh <[email-masked]> 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.