From: Ben Peart Date: Thu, 21 Sep 2017 02:06:00 GMT Subject: Re: [PATCH v7 03/12] update-index: add a new --force-write-index option Message-ID: <28891f82-32e5-dc46-961c-a44e86e84989@gmail.com> In-Reply-To: On 9/20/2017 9:46 PM, Junio C Hamano wrote: > Ben Peart 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);