Junio C Hamano [off-list ref] writes:
Duy Nguyen [off-list ref] writes:
quoted
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:
quoted
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.
On Thu, Dec 24, 2015 at 8:56 AM, Junio C Hamano [off-list ref] wrote:
Junio C Hamano [off-list ref] writes:
quoted
Duy Nguyen [off-list ref] writes:
quoted
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:
quoted
quoted
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.
Sounds good, except..
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.
Do we really need this? If the index is created fresh from the core,
first write will not have UNTR. Next time it's loaded up, UNTR is
added if config is true, based on the above rules. The new UNTR
extension (should?) marks the index as dirty as writing out is
required.
Orthogonal to the "config overrides the existing users' practice"
issue,
Yeah it looks orthogonal to me too. There should be a separate config
file like /etc/gitconfig.lockdown, that will override user config vars
(but we still can't undefine user config vars yet, known missing
feature).
--
Duy
On Thu, Dec 24, 2015 at 2:56 AM, Junio C Hamano [off-list ref] wrote:
Junio C Hamano [off-list ref] writes:
quoted
Duy Nguyen [off-list ref] writes:
quoted
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:
quoted
quoted
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.