Re: [PATCH 1/3] Add read_cache_from() and discard_cache()

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 2, 2006, 08:51 UTC
Message-ID: <Pine.LNX.4.63.0607021043550.29667@wbgn013.biozentrum.uni-wuerzburg.de>
In-Reply-To: <7v64ihdupr.fsf@assigned-by-dhcp.cox.net>

Hi,

On Sat, 1 Jul 2006, Junio C Hamano wrote:

Show 16 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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?

Show 15 quoted lines

> >>  - 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