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

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:42:31

Johannes Schindelin [off-list ref] writes:
+int discard_cache()
+{
+	int ret;
+	
+	if (cache_mmap == NULL)
+		return 0;
+	ret = munmap(cache_mmap, cache_mmap_size);
+	cache_mmap = NULL;
+	cache_mmap_size = 0;
+	active_nr = active_cache_changed = 0;
+	/* no need to throw away allocated active_cache */
+	return ret;
+}
+
I haven't been following the details of the patches in this
thread while they are being cooked actively, but two things to
look out for are:

 - I am guessing you run discard_cache() because you want to
   read in a new cache (or start from a clean slate).  I am not
   sure what you are doing with the old cache tree data
   structure.  If you are starting from a clean slate
   (i.e. there is no read_cache_from() after you call
   discard_cache), you would probably need to discard the old
   cache tree; otherwise your next write-tree may produce an
   incorrect index file.  If you keep the old one and later
   swap it in, the problem might be even more severe.

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

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

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:42:32

Hi,

On Fri, 30 Jun 2006, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
+int discard_cache()
+{
+	int ret;
+	
+	if (cache_mmap == NULL)
+		return 0;
+	ret = munmap(cache_mmap, cache_mmap_size);
+	cache_mmap = NULL;
+	cache_mmap_size = 0;
+	active_nr = active_cache_changed = 0;
+	/* no need to throw away allocated active_cache */
+	return ret;
+}
+
I haven't been following the details of the patches in this
thread while they are being cooked actively, but two things to
look out for are:

 - I am guessing you run discard_cache() because you want to
   read in a new cache (or start from a clean slate).  I am not
   sure what you are doing with the old cache tree data
   structure.  If you are starting from a clean slate
   (i.e. there is no read_cache_from() after you call
   discard_cache), you would probably need to discard the old
   cache tree; otherwise your next write-tree may produce an
   incorrect index file.  If you keep the old one and later
   swap it in, the problem might be even more severe.
True, I missed that one. But it is just a call to 
cache_tree_free(active_cache_tree); in discard_cache(), right?
 - 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;

If there is more to it, please don't let me die dumb.

Ciao,
Dscho
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help