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

[Adrian Buehlmann]

On 27.09.2009 01:07, Greg Ward wrote:
> On Fri, Sep 25, 2009 at 7:09 PM, Adrian Buehlmann <adrian at cadifra.com> wrote:
>> Ugh. Does that help?
> 
> Very much, thank you.  I'm inclined to agree with Martin, that this
> particular bit of code is sufficiently tricky and subtle that it needs
> further explanation.  If the code had been completely understandable
> as it is, then I wouldn't have been asking questions about it.
> 
> (I should say that in general, reading the Mercurial source has taught
> me two things: clear code doesn't need many comments, and never use
> one-letter variable names.  So yeah, in general I agree with your
> stance on comments, but this is an exceptional case.  Furthermore,
> it's a case that spills out of dirstate and affects other areas:
> anyone calling dirstate.status() has to be aware of this.   My
> proposed docstring for dirstate.status() should help there, I think.)
> 
> I'll see if I can distill your lengthy explanation down to something
> concise, correct, and still useful, and the resend my patch.  I like a
> good challenge.  ;-)
> 

I have come to the conclusion that the granularity setting
is pointless up to the level that it starts harming. It doesn't
harm program execution but it does harm understanding of the code.

It should be removed. We simply don't need it, it complicates
matters for no good reason.

The "blind-second" case is completely covered by testing for
equality. The code already uses the filesystem's notion of
'now', which is perfect.

I'm proposing the following change, which also introduces considerable
amount of new comment:

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
+        # taking 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 modified at "the same moment" (e.g. within the
+                # same second for filesystems like ext3) as we write dirstate.
+                # This commonly happens for at least a couple of files on
+                # update. The user could change the file without changing its
+                # size immediately after the close of the transaction but still
+                # within the same second.
+                # Avoid to mistreat such files as clean in future status calls
+                # by invalidating the file's stat data in dirstate, forcing a
+                # file compare on future status calls.
+                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)