On 9/20/2017 9:46 PM, Junio C Hamano wrote:
Ben Peart <peartben@xxxxxxxxx> writes:
Lets see how my ascii art skills do at describing this:
Your ascii art is fine. If you said upfront that the capital
letters signify points in time, lower letters are file-touching
events, and time flows from left to right, it would have been
perfect ;-)
Rats, so close and yet... ;-)
There is no real concern about accumulating too many changes as 1) the
processing cost for additional modified files is fairly trivial and 2)
the index ends up getting written out pretty frequently anyway as
files are added/removed/staged/etc which updates the
fsmonitor_last_update time.
I still see some impedance mismatch here. The optimization
described is valid but the code to do the optimization would avoid
writing the index file out when the in-core index is dirty only
because the status reported by fsmonitor changed--if there were
other changes (e.g. the code added a new index entry), even with the
optimization, you would still want to write the index out, right?
Yes, that is exactly how it works. FSMONITOR_CHANGED is only set when
the fsmonitor index extension is added or removed but all the other
logic to flag the index dirty (CE_ENTRY_CHANGED/REMOVED/ADDED,
UNTRACKED_CHANGED, etc) still exists and will still trigger the index to
be written out as it always has. It's not to say some of these other
changes could not do the same optimization (I haven't looked) if
recomputing is cheaper than writing out the index.
With or without the need for forced flush to help debugging, would
that suggest that you need two bits now, instead of just a single
'active-cache-changed' bit?
By keeping track of that new bit that tells us if we have
fsmonitor-only changes that we _could_ flush, this patch can further
reduce the need for forced flushing (i.e. if we know we didn't get
fsmonitor induced dirtyness, force_write can still be no-op), no?
Yes, I suppose we _could_ add a 2nd bit (and then add the logic to set
that bit every time a fsmonitor change was made) but I don't see that it
really buys us anything useful. The force write flag in update-index is
off by default and the only scenario we have that someone would set it
is for test cases where the perf of writing out the index when it is not
needed just doesn't matter.
The challenge came when it was time to test that the changes to the
index were correct. Since they are lazily written by default, I
needed a way to force the write so that I could verify the index on
disk was correct. Hence, this patch.
OPT_END()
};
@@ -1147,7 +1150,7 @@ int cmd_update_index(int argc, const char
**argv, const char *prefix)
die("BUG: bad untracked_cache value: %d", untracked_cache);
}
- if (active_cache_changed) {
+ if (active_cache_changed || force_write) {
if (newfd < 0) {
if (refresh_args.flags & REFRESH_QUIET)
exit(128);
