Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Jeff King <peff@peff.net>]

On Wed, Jun 26, 2013 at 10:45:48PM +0100, Ramsay Jones wrote:

> > This patch adds some *extra* cache invalidation that was heretofore
> > missing.  If stat() is broken it could
> > 
> > (a) cause a false positive, resulting in some unnecessary cache
> > invalidation and re-reading of packed-refs, which will hurt performance
> > but not correctness; or
> > 
> > (b) cause a false negative, in which case the stale cache might be used
> > for reading (but not writing), just as was *always* the case before this
> > patch.
> > 
> > As far as I understand, the concern for cygwin is (a).  I will leave it
> > to others to measure and/or decide whether the performance loss is too
> > grave to endure until the cygwin stat() situation is fixed.
> 
> Hmm, I'm not sure I understand ... However, I can confirm that the
> 'mh/ref-races' branch in next is broken on cygwin. (i.e. it is not
> just a speed issue; it provokes fatal errors).

I think Michael's assessment above is missing one thing. It is true that
a false positive is just a performance problem in most cases, as we
unnecessarily reload the file, thinking it has changed.

However, when we have taken a lock on the file, there is an additional
safety measure: if we find the file is changed, we abort, as that should
never happen (it means somebody else modified the file while we had it
locked). But of course Cygwin's false positive here triggers the safety
valve, and we die without even realizing that nothing changed.

In theory we can drop the safety valve; it should never actually happen.
But I'd like to keep it there for working systems. Perhaps it is worth
doing something like this:

diff --git a/refs.c b/refs.c
index 4302206..7cac42d 100644
--- a/refs.c
+++ b/refs.c
@@ -1080,6 +1080,16 @@ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)
 		packed_refs_file = git_path("packed-refs");
 
 	if (refs->packed &&
+#ifdef STAT_VALIDITY_GIVES_FALSE_POSITIVES
+	    /*
+	     * If we get false positives from our stat calls on this platform,
+	     * then we must assume that once we have locked the packed-refs
+	     * file it does not change. Otherwise it looks like somebody else
+	     * has changed it out from under us (while we have it locked!), and
+	     * we die().
+	     */
+	    !refs->packed->lock &&
+#endif
 	    !stat_validity_check(&refs->packed->validity, packed_refs_file))
 		clear_packed_ref_cache(refs);
 

and then we can add that define to Cygwin in the Makefile. I verified
the issue on Linux by "breaking" stat_validity_check to always return 0
(i.e., to always give a false positive that the file has changed, which
is what Cygwin is doing).

I am curious how often Cygwin gives us the false positive. If it is
every time, then the check is not doing much good at all. Is it possible
for you to instrument stat_validity_check to report how often it does or
does not do anything useful?

-Peff