Hi, On Sat, 1 Jul 2006, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes: > > > True, I missed that one. But it is just a call to > > cache_tree_free(active_cache_tree); in discard_cache(), right? > > On the codepath to write out the new index file, calling > cache_free_tree(&active_cache_tree) before write_cache() is all that > should be needed. When "active_cache_tree == NULL", write_cache() would > write out an index file without the cached tree information. > > Currently not many things take advantage of cached tree information to > optimize its operation. But I'd like to change that. For example, tree > merges by read-tree should be able to take advantage of the fact that a > cached tree read from the index and three trees being read all match for > a subdirectory and do the merge of the directory without descending into > it. Together with my argument, that a library function should make life easier for you, not harder, you are making a fine point that the _clean() version of get_merge_bases() should clean the active_cache_tree, too. Besides, is it not better to clean up now-bogus memory anyway? > >> - index_timestamp is left as the old value in this patch when > >> you switch cache using read_cache_from() directly. I have a > >> suspicion you may be bitten by "Racy Git" problem, especially > >> because the operations are supposed to happen quickly thanks > >> to the effort of you two ;-) increasing the risks that the > >> file timestamp of the working tree file and the cached entry > >> match. > > > > Yes. Again, just one line to discard_cache(), right? > > > > index_file_timestamp = 0; > > This one I am not sure. Read the comment in ce_match_stat() and > see the problematic sequence of events that this variable tries > to help resolve applies to your use. Okay. After reading the comment, I am quite certain we can just set the index_file_timestamp to 0. Either we start with an empty cache; In that case it is obviously correct to set the timestamp to 0. Or, after we discard the cache, we have to read a new cache before working on it. Now, reading the cache means calling read_cache() (or read_cache_from()), and -- alas -- line number 12 in the body of that function sets the timestamp to 0 _anyway_, to be reset to the correct value later. So, I still think that these two lines should be in the cleanup part of get_merge_bases(). BTW personally, I prefer the one-function approach, i.e. a flag which says if it is okay not to clean up. Ciao, Dscho - : 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