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

[Christian Couder <christian.couder@gmail.com>]

On Thu, Dec 24, 2015 at 2:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Great!

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

Ok. The first RFC patch series about this had a tristate and I
switched to a boolean as it seemed that people prefered a boolean, but
you are right that a tristate is more backward compatible.

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

My current patch series does these changes in
wt_status_collect_untracked() because currently the UNTR is mostly
used only in git status, so it feels safer to me to not affect other
code paths.

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

This has been changed in v3, see patch 09/11, and I think it should
now work as you suggest.

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

Great! I am preparing a v4 that I hope to send in a few days.
By the way the v3 does not pass its own tests due to a bug introduced
in last minute changes.

Thanks,
Christian.