[PATCH STABLE?] commit: avoid a dirstate race with multiple commits in the same process

Adrian Buehlmann adrian at cadifra.com
Wed Mar 16 03:12:13 CDT 2011



On 2011-03-16 03:19, Greg Ward wrote:
> On 16 March 2011, Adrian Buehlmann said:
>>> +    def getfstime(self, dir):
>>> +        '''Return the current time to filesystem resolution.'''
>>
>> I somehow have a hard time with the grammar and semantic in this doc
>> sentence. Maybe:
>>
>>            '''Return the filesystem's notion of "now", rounded down
>>            to the dirstate's one second resolution'''
> 
> That's true on filesystems with one second resolution (e.g. most POSIX
> filesystems).  But on legacy MS-DOS filesystems, the resolution is 2
> seconds.  Or at least I *assume* that is why dirstate.write() uses
> stat() + mtime to get the current time instead of just time.time().

Sure. That's why I say

   .. the filesystem's notion of "now"...

in the first part. For vfat (let's not use the term MS-DOS please), that
value just *happens* to be already rounded to the dirstate's resolution,
because the dirstate resolution is finer grained than vfat's in that
case. But the dirstate is portable across filesystems.

Now to the second step...

The reason why there is a "int(..)" python call is because that time
value needs to be rounded down to the next integral second precisely
_because_ poor dirstate's resolution is only one second. For vfat, that
int(..) call is a NOP. That int(..) call is there for filesystems with
sub-second time resolution (e.g. NTFS).

I think you haven't busted my wording of this docstring yet. Feel free
to keep trying :-)

>>> +
>>> +    def maybelookup(self, f, now):
>>
>> I'm still pondering about a better name for this function. Proposals for
>> bikeshedding follow below:
>>
>>        def validatemtime(self, f, now):
> 
> Meh.  maybelookup() isn't perfect, but I prefer it to validatemtime().
> Keep trying... ;-)

Ditto Meh :-). But what are you looking up there? Nothing!

validatemtime is still my preferred one, but let's throw some more in
the ring in order to avoid maybelookup: checkmtime, verifymtime,
invalidateiftooclose (but please let's not do 'maybeinvalidate').

Any other ideas? (usually Matt is the champion...)

>>> +        '''Examine f to determine if it needs more work to determine its
>>> +        true status, or whether it can be considered normal.  If more
>>> +        work needed, set the in-memory state to lookup; otherwise, leave
>>> +        it alone. Thus, maybelookup() can affect the next call to
>>> +        status(). now must be the current time to filesystem resolution
>>> +        (see getfstime()).'''
>>
>> I think something like
>>
>>            '''Examine the entry for file f and invalidate it, if its
>>            mtime is too close to "now" to be useful.'''
>>
>> should be enough together with (<cough>) my preexisting fat comment inside.
> 
> Ahh yes, now you are hitting the nail on the head.  Much better
> docstring.  Let's see what Matt thinks of my split-and-resend; if I
> need to send it again, I'll incorporate this docstring.
> 
> And, BTW, your fat comment is still extremely useful.  This stuff is
> not even remotely obvious; every little bit helps.

Yeah. I'm quite happy with that comment. It helped me to understand what
you are doing there.

FWIW, I agree with Matt that the patch should be split. I think a first
patch step could be to introduce that function validatemtime (or
whatever name we come up with in the end).


More information about the Mercurial-devel mailing list