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

Matt Mackall mpm at selenic.com
Tue Sep 29 14:50:43 CDT 2009


On Tue, 2009-09-29 at 21:36 +0200, Adrian Buehlmann wrote:
> On 28.09.2009 20:04, Matt Mackall wrote:
> > 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.
> > 
> 
> I think we should just apply int(..) to the float for 'now':
> 
> 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 = int(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)
> 
> 
> Example: doing a 'hg up' that updates file 'a'
> 
>   #  time[s]  event
>   1   8.6     'hg up' called
>   2   8.7     dirstate.normal('a'): _map['a'][3] is set to int(8.7) = 8
>   3   9.2     dirstate.write(): st_mtime for tempfile returns 9.2 -> now is 9
>                                 -> 9 != 8 -> _map['a'] is not changed
>   4   9.3     'hg up' finished
>   5   9.4     user modifies 'a' without changing size
>   6  15.0     'hg status' called
>                  -> st_mtime for 'a' returns 9.4 -> int(9.4) = 9 != 8
>                  -> file not compared -> returns "M a"
> 
> We already do int(..) in dirstate.normal and dirstate.status for the floats
> that st_mtime may return.

Sure. We'll eventually need to tackle the nanosecond issue, but that can
wait.

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




More information about the Mercurial-devel mailing list