Linus Torvalds [off-list ref] writes:
Quite frankly, I'd like for us to at least think about removing CE_VALID.
This is heavy. I personally do not use this flag, nor know anybody who
does, but deviating from the original purpose of CE_VALID, which was to
avoid lstat() on slow filesystems, people have advised/advocated its use
for "narrow checkout". These people may not even have a file checked out
to the CE_VALID path in the work tree, and they depend on us not running
lstat() on them and instead always answering that the work tree has the
necessary blob.
I wonder if we have other places where we have optimized away the lstat()
just because we decided that it was already up-to-date - without
realizing that something could have been marked up-to-date just because
it was marked CE_VALID.
That is a very valid concern, but I think fixing them may break the
"narrow checkout" people.
We may need to add ce_uptodate(ce) check instead of doing lstat() in
some places (like the one you modified in this patch), not because we want
to avoid lstat(), but because we do not want to lstat() paths that are
marked as CE_VALID.
There are some mechanisms, such as REFRESH_REALLY flag, to give an escape
hatch to break out of the CE_VALID illusion, but I have to admit that when
we did CE_VALID we did not quite clarified its ramifications with respect
to merging and branch switching.
On Tue, 14 Jul 2009, Junio C Hamano wrote:
Linus Torvalds [off-list ref] writes:
quoted
Quite frankly, I'd like for us to at least think about removing CE_VALID.
This is heavy. I personally do not use this flag, nor know anybody who
does, but deviating from the original purpose of CE_VALID, which was to
avoid lstat() on slow filesystems, people have advised/advocated its use
for "narrow checkout".
Ahh. I kind of was aware of that, but had totally forgotten.
And in that case, I guess it's also fine. In fact, for that case CE_VALID
would tend to really mean "always assume CE_UPTODATE", so then the patch I
sent out doesn't really necessarily need the whole "known_uptodate()"
thing, and could just use
if (o->reset && !ce_valid(old)) {
instead.
Which also makes my other worries go away.
Linus
In our 'oneway_merge()' we always do an 'lstat()' to see if we might
need to mark the entry for updating.
But we really shouldn't need to do that when the cache entry is already
marked as being ce_uptodate(), and this makes us do unnecessary lstat()
calls if we have index preloading enabled.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
As Junio noticed, I meant "ce_uptodate()", not "ce_valid()", but this is
basically the simplified version that replaces the one that cared about
CE_VALID.
On Tue, 14 Jul 2009, Linus Torvalds wrote:
And in that case, I guess it's also fine. In fact, for that case CE_VALID
would tend to really mean "always assume CE_UPTODATE", so then the patch I
sent out doesn't really necessarily need the whole "known_uptodate()"
thing, and could just use
if (o->reset && !ce_valid(old)) {
instead.
Which also makes my other worries go away.
Linus
unpack-trees.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index f9d12aa..48d862d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1004,7 +1004,7 @@ int oneway_merge(struct cache_entry **src, struct unpack_trees_options *o)
if (old && same(old, a)) {
int update = 0;
- if (o->reset) {
+ if (o->reset && !ce_uptodate(old)) {
struct stat st;
if (lstat(old->name, &st) ||
ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID))