Re: [PATCH 00/12] Fix some reference-related races

From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Jun 18, 2013, 18:13 UTC
Message-ID: <51C0A33A.7060802@ramsay1.demon.co.uk>
In-Reply-To: <51BD5232.20205@alum.mit.edu>

Michael Haggerty wrote:

Show 5 quoted lines

> Thanks for all of the information.
> 
> On 06/15/2013 10:13 PM, Ramsay Jones wrote:
>> Michael Haggerty wrote:
>>> *This patch series must be built on top of mh/reflife.*

[ ... ]

Show 6 quoted lines

>> You may be wondering why clear_packed_ref_cache() is called? Well, that
>> is because stat_validity_check() *incorrectly* indicates that the
>> packed-refs file has changed. Why does it do that? Well, this is one
>> more example of the problems caused by the cygwin schizophrenic stat()
>> functions. :( [ARGHHHHHHHHH]
>>

[ ... ]

> So if I understand correctly, all of the above is *without* the
> refcounting changes introduced in mh/ref-races?  Is so, then it is not
> surprising, as this is exactly the sort of problem that the reference
> counting is meant to solve.

Yes, as I said, this describes the old (non refcounted) series. This particular problem (the segmentation fault) is fixed by the new series (as noted below). [Note, however, that the packed-refs file will still be re-read more often than needed.]

>> Now, turning to the new code, t3211-peel-ref.sh test #7 now works, but
>> test #8 still fails...

[ ... ]

Show 12 quoted lines

> These "internal error: packed-ref cache cleared while locked" failures
> result from an internal consistency check that clear_packed_ref_cache()
> is not called while the write lock is held on the packed-refs file.  A
> call to c_p_r_c() could result from
> 
> * a programming error
> 
> * a determination based on the packed-refs file stat that the file needs
> to be re-read
> 
> Judging from what you said about cygwin, I assume that the latter is
> happening.

Indeed.

>             It should be impossible, because the current process is
> holding packed-refs.lock, and therefore other git processes should
> refuse to change the packed-refs file.

:-P You are assuming that a single process can't lie to itself ...

[ ... ]

> Yikes!  ECYGWINFAIL.

Ah, NO, this should read ECYGWINGITFAIL. This is a self-inflicted wound; it has nothing much to do with cygwin.

I should not have assumed that you knew what I meant by "schizophrenic stat() functions" above; sorry about that! If you are interested, then the following commits may be useful reading: adbc0b6, 7faee6b, 7974843, 05bab3ea, 924aaf3e and b8a97333.

[ ... ]

Show 5 quoted lines

>> I haven't checked the remaining test failures to see if they are
>> caused by this code (I don't think so, but ...), but this failure
>> is clearly a cygwin specific issue.
> 
> Thanks again for the testing and analysis,

So, unless you feel the need to fix this yourself, you can probably ignore this issue for now. I will hopefully find time to fix it up before this topic progresses to next. (Although I don't have any feeling for the time-frame of this topic).

HTH

ATB, Ramsay Jones