Re: [PATCH 7/8] config: add core.untrackedCache

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

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

> Duy Nguyen <pclouds@gmail.com> writes:
>
>> In that case we can just check config once in read_index_from and
>> destroy UNTR extension. Or the middle ground, we check config once in
>> that place, make a note in struct index_state, and make invalidate_*
>> check that note instead of config file. The middle ground has an
>> advantage over destroying UNTR: (probably) many operations will touch
>> index but do not add or remove entries.
>
> Or we can even teach read_index_from() to skip reading the extension
> without even parsing when the configuration tells it that the
> feature is force-disabled.  It can also add an empty one when the
> configuration tells it that the feature is force-enabled and there
> is no UNTR extension yet.

Thinking about this a bit more, I am starting to feel that the
config that can be used to optionally override the presence of
in-index UNTR extension does not have to be too bad a thing,
provided if it is done with a few tweaks to the design I read in
Christian & Ævar's messages.

One tweak is to address the following from Ævar's message:

>> Once this series is applied and git is shipped with it existing
>> users that have set "git update-index --untracked-cache" will have
>> their UC feature disabled. This is because we fall back to "oh no
>> config here? Must have been disabled, rm it from the index" clause
>> which keeps our UC from going stale in the face of config
>> flip-flopping.

The above would happen only if the configuration is a boolean that
defaults to false.  I'd think we can have this a tristate instead.
That is, config.untrackedCache can be explicitly set to 'true',
'false', or 'keep'.  And make a missing config.untrackedCache
default to 'keep'.

When read_index_from() reads an existing index:

    When the configuration is set to 'true':
        an existing UNTR is kept, otherwise a new UNTR gets added.
    When the configuration is set to 'false:
        an existing UNTR is dropped.
    When the configuration is set to 'keep':
        an existing UNTR is kept, otherwise nothing happens.

When write_index() writes out an index that wasn't initialized from
the filesystem, a new UNTR gets added only when the configuration is
set to 'true' and there is no in-core UNTR already.

That way, those who use /etc/gitconfig to force the feature over
their users would be able to set it to 'true', those who have been
using the feature in some but not all of their repositories won't
see any different behaviour until they muck with the configuration
(and if they are paranoid and want to opt out of their administrator's
setting, they can set it to 'keep' in their $HOME/.gitconfig to make
sure their repositories are not affected).

Orthogonal to the "config overrides the existing users' practice"
issue, I still think that [PATCH v2 10/10] goes too far to remove
the safety based on the working tree location.  Checking kernel
version and other thing may be too much, but the check based on the
working tree location is a cheap way to make sure that those who do
unusual things (namely, use $GIT_DIR/$GIT_WORK_TREE or their
equivalents to tell Git that the working tree for this invocation is
at a place different from what UNTR data was prepared for) are not
harmed by incorrectly reusing the cached data for an unrelated
location.  So another tweak I'd feel better to see is to resurrect
that safety.

I wouldn't have as much issue with such a scheme as I had with the
earlier description of the design in the Christian's series.

Thanks.