In 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@xxxxxxxxxxxxxxxxxxxx>
---
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))
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html