Linus TorvaldsIn our 'oneway_merge()' we always do an 'lstat()' to see if we might need to mark the entry for updating.
We really shouldn't need to do that when the cache entry is already marked as being ce_uptodate(). However, since this function will actually match the lstat() information with the CE_MATCH_IGNORE_VALID, we need to be a bit more careful: it's not sufficient to check ce_uptodate(), we need to also double-check that the ce_uptodate() isn't because of CE_VALID (which will be "uptodate" regardless of whether the lstat() matches or not).
This explains why it's not sufficient to check _just_ the CE_UPTODATE bit.
But if both CE_UPTODATE is set, and CE_VALID is clear, then we know that the lstat() will always match the cache entry information, and there's no point in doing another one.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> ---
Quite frankly, I'd like for us to at least think about removing CE_VALID.
Does anybody use it (and "core.ignorestat" that turns it on, along with the "--assume-unchanged" flag to update-index)
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.
The original reasoning behind CE_VALID was to avoid the cost of lstat's on operating systems where it is slow, but we are now so good at optimizing them away (thanks to CE_UPTODATE) that I wonder whether it's not better to just always rely on CE_UPTODATE.
In fact, a quick grep made me just look at "verify_uptodate()", and how it does ie_match_stat(CE_MATCH_IGNORE_VALID). But look at how we've already short-circuited this if ce_uptodate() is true, and we never get there if we've preloaded the index or done any other operation that might have already validated the entry?
I dunno. This at least doesn't make things worse, since I started thinking about this _after_ having first done a patch that just did the "ce_uptodate()" test.
unpack-trees.c | 14 +++++++++++++- 1 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c index f9d12aa..9920a6f 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -985,6 +985,18 @@ int bind_merge(struct cache_entry **src, } /* + * Do we _know_ the stat information will match? + * + * Note that if CE_VALID is set, then we might have a + * uptodate cache entry even if the stat info might not + * match. + */ +static inline int known_uptodate(struct cache_entry *ce) +{ + return ce_uptodate(ce) && !(ce->ce_flags & CE_VALID); +} + +/* * One-way merge. * * The rule is: @@ -1004,7 +1016,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 && !known_uptodate(old)) { struct stat st; if (lstat(old->name, &st) || ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID))
