Ævar Arnfjörð Bjarmason [off-list ref] writes:
The way the "config decides" patch series deals with this is that if
you have the UC information in the index and the configuration is set
to core.untrackedCache=false the UC will be removed from the index.
Otherwise you would indeed easily end up with a stale cache,...
Yeah, that's "correctness" thing; it goes without saying that it
would be unacceptable if the series did not get this right.
I still have a problem with the approach from "design cleanliness"
point of view, primarily that the index already has a bit to tell us
if the user already said that she wants to use the feature, but
because you want to make config win, the code needs to always read
the config to allow it to disable the feature in the index, just in
case the data that is already in the index says otherwise, and the
code has to keep doing that even after the data is removed [*1*].
Unfortunately, I do not think you can solve the "design cleanliness"
problem unless you give up "we want /etc/gitconfig override".
In any case I think we already have agreed to disagree on this
point, so there is no use discussing it any longer from my side. I
am not closing the door to this series, but I am not convinced,
either. At least not yet.
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.
Well, the fix to that is merely just a one-shot thing away, so it
would not be too much of a hassle, no [*2*]? So I do not think it
would be such a big issue.
We *could* make even that use-case work by detecting the legacy marker
for the UC in the index (the uname info), then we'd do a one-time "git
config --local core.untrackedCache true" and remove the marker.
I do not think we want to go there---that would mean you would need
to revamp the repository format version because the old tools would
be now unusable on the index/config combo your version mucked with.
[Footnote]
*1* I also do not want to see that design pattern imitated and used
in other parts of the system, and this gives a precedent for
people to copy.
*2* Yet, those who are broken by this series may say "it is
unacceptable that we have to survey all existing repositories
and selectively add the configuration to the ones that have
enabled the feature before updating.", the same way you complain
against the "index already knows" approach.
On Tue, Dec 15, 2015 at 8:40 PM, Junio C Hamano [off-list ref] wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
I still have a problem with the approach from "design cleanliness"
point of view[...]
In any case I think we already have agreed to disagree on this
point, so there is no use discussing it any longer from my side. I
am not closing the door to this series, but I am not convinced,
either. At least not yet.
In general the fantastic thing about the git configuration facility is
that it provides both systems administrators and normal users with
what they want. It's possible to configure things system-wide and
override those on a user or repository basis.
Of course hindsight is 20/20, but I think that given what's been
covered in this thread it's been established that it's categorically
better that if we introduce features like these that they be
configured through the normal configuration facility rather than the
configuration being sticky to the index. It gives you everything that
the per-index configuration gives you and more.
So assuming that's the case, how do we migrate something that's
configured via the index towards being configured through git-config?
I think there's no general answer to that, but in this case the worst
case scenario with accepting this series as-is is that we downgrade
some users who've opted in to it to pre-v2.5.0 "git status"
performance.
Since the change in performance really isn't noticeable except on
really large repositories, which are more likely to have someone
involved watching the changelog on upgrades I think that's OK.
On Wed, Dec 16, 2015 at 4:53 AM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
On Tue, Dec 15, 2015 at 8:40 PM, Junio C Hamano [off-list ref] wrote:
quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:
I still have a problem with the approach from "design cleanliness"
point of view[...]
In any case I think we already have agreed to disagree on this
point, so there is no use discussing it any longer from my side. I
am not closing the door to this series, but I am not convinced,
either. At least not yet.
In general the fantastic thing about the git configuration facility is
that it provides both systems administrators and normal users with
what they want. It's possible to configure things system-wide and
override those on a user or repository basis.
Of course hindsight is 20/20, but I think that given what's been
covered in this thread it's been established that it's categorically
better that if we introduce features like these that they be
configured through the normal configuration facility rather than the
configuration being sticky to the index.
A minor note for implementers. We need to check that config is loaded
first. read-cache.c, being part of the core, does not bother itself
with config loading. And I think so far it has not used any config
vars. If a command forgets (*) to load the config, the cache may be
deleted (if we do it the safe way).
(*) is there any command deliberately avoid loading config? git-clone
and git-init are special, but for this particular case it's probably
fine.
--
Duy
On Thu, Dec 17, 2015 at 1:36 PM, Duy Nguyen [off-list ref] wrote:
On Wed, Dec 16, 2015 at 4:53 AM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
On Tue, Dec 15, 2015 at 8:40 PM, Junio C Hamano [off-list ref] wrote:
quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:
I still have a problem with the approach from "design cleanliness"
point of view[...]
In any case I think we already have agreed to disagree on this
point, so there is no use discussing it any longer from my side. I
am not closing the door to this series, but I am not convinced,
either. At least not yet.
In general the fantastic thing about the git configuration facility is
that it provides both systems administrators and normal users with
what they want. It's possible to configure things system-wide and
override those on a user or repository basis.
Of course hindsight is 20/20, but I think that given what's been
covered in this thread it's been established that it's categorically
better that if we introduce features like these that they be
configured through the normal configuration facility rather than the
configuration being sticky to the index.
A minor note for implementers. We need to check that config is loaded
first. read-cache.c, being part of the core, does not bother itself
with config loading. And I think so far it has not used any config
vars. If a command forgets (*) to load the config, the cache may be
deleted (if we do it the safe way).
(*) is there any command deliberately avoid loading config? git-clone
and git-init are special, but for this particular case it's probably
fine.
Thanks for this note.
Looking at the current patch, the global variable in which the value
of the core.untrackedCache config var is stored is
"use_untracked_cache".
It is used in the following places:
- wt_status_collect_untracked() in wt-status.c which is called only by
"git status" and "git commit" after the config has been loaded.
- cmd_update_index() in builtin/update-index.c which loads the config
before using it
- validate_untracked_cache() in dir.c where it is used in:
if (use_untracked_cache != 1 && !ident_in_untracked(dir->untracked)) {
warning(_("Untracked cache is disabled on this system."));
return NULL;
}
but this "if" and its contents are removed by patch 10/10 in v2.
So at the end of this patch series, there is no risk of
use_untracked_cache not being properly setup.