[PATCH] dirstate: add/improve method docstrings and some comments

[Matt Mackall]

On Mon, 2009-09-28 at 11:42 +0200, Adrian Buehlmann wrote:
> On 28.09.2009 09:49, Dirkjan Ochtman wrote:
> > On Mon, Sep 28, 2009 at 09:43, Adrian Buehlmann <adrian at cadifra.com> wrote:
> >> I'll include your comment changes (without the double spaces after period, if
> >> you don't mind) and offer to
> > 
> > Yay single spaces!
> > 
> >> * create a complete patch
> >> * apply it to crew tip
> >> * run the testsuite on FreeBSD with it
> >> * do some additional manual testing, especially on Windows. That is, test it on FAT
> >>  to confirm it works as expected for the two second resolution case.
> >> * send it to the list for final review
> >>
> >> _provided_ that at least someone of crew or Matt tells me that such a patch
> >> has good chances for inclusion (with that unusual amount of comment).
> > 
> > It definitely looks like an improvement to me, although I think you
> > might want Matt to comment anyway.
> > 
> 
> Patch with updated comments (taking Greg's changes into account):
> 
> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
> --- a/mercurial/dirstate.py
> +++ b/mercurial/dirstate.py
> @@ -373,13 +373,9 @@ class dirstate(object):
>              return
>          st = self._opener("dirstate", "w", atomictemp=True)
> 
> -        try:
> -            gran = int(self._ui.config('dirstate', 'granularity', 1))
> -        except ValueError:
> -            gran = 1
> -        if gran > 0:
> -            hlimit = util.fstat(st).st_mtime
> -            llimit = hlimit - gran
> +        # use the modification time of the newly created temporary file as the
> +        # filesystem's notion of 'now'
> +        now = util.fstat(st).st_mtime
> 
>          cs = cStringIO.StringIO()
>          copymap = self._copymap
> @@ -389,9 +385,19 @@ class dirstate(object):
>          for f, e in self._map.iteritems():
>              if f in copymap:
>                  f = "%s\0%s" % (f, copymap[f])
> -            if gran > 0 and e[0] == 'n' and llimit < e[3] <= hlimit:
> -                # file was updated too recently, ignore stat data
> -                e = (e[0], 0, -1, -1)
> +
> +            if e[0] == 'n' and e[3] == now:
> +                # The file was last modified "simultaneously" with the current
> +                # write to dirstate (i.e. within the same second for file-
> +                # systems with a granularity of 1 sec). This commonly happens
> +                # for at least a couple of files on 'update'.
> +                # The user could change the file without changing its size
> +                # within the same second. Invalidate the file's stat data in
> +                # dirstate, forcing future 'status' calls to compare the
> +                # contents of the file. This prevents mistakenly treating such
> +                # files as clean.
> +                e = (e[0], 0, -1, -1)   # mark entry as 'unset'
> +
>              e = pack(_format, e[0], e[1], e[2], e[3], len(f))
>              write(e)
>              write(f)

That's a little worrisome. I think we should keep the explicit range
comparison. I've known of filesystem bugs where the kernel would cache
nanosecond-resolution timestamps but only actually record at second
resolution. Throw networked filesystems into the mix and cross-machine
clock skew and it gets even dicier. If Python decides to hand back a
floating point timestamp, exact comparisons are likely to be
problematic. But we can change it to now - 1 < e[3] < now + 1.

-- 
http://selenic.com : development and support for Mercurial and Linux