[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