[PATCH 3 of 6 resolve-ux] merge: implement mergestate.__len__ and mergestate.__nonzero__

[Mads Kiilerich]

On 05/02/2014 07:47 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc at gmail.com>
> # Date 1397872888 25200
> #      Fri Apr 18 19:01:28 2014 -0700
> # Branch stable
> # Node ID b4d95342be6de3e3a229e1536720d22d6de6eb13
> # Parent  812bb7bf59e8ff0e2e60c1d73857206c6efd8b32
> merge: implement mergestate.__len__ and mergestate.__nonzero__
>
> An upcoming patch will test if mergestate is present. Until this patch,
> there was no easy way to do this without looking at files on disk (which
> are part of mergestate's implementation details).
>
> This patch allows consumers to differentiate between no mergestate,
> empty mergestate, and mergestate with entries. It does this by
> implementing __nonzero__ and __len__. |if ms| will now be true if the
> mergestate has entries or it has records on disk. |if len(ms)| will now
> be true iff mergestate has entries.
>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -46,16 +46,17 @@ class mergestate(object):
>       F: a file to be merged entry
>       '''
>       statepathv1 = "merge/state"
>       statepathv2 = "merge/state2"
>   
>       def __init__(self, repo):
>           self._repo = repo
>           self._dirty = False
> +        self._recordpresent = False

This value seems a bit odd to me. The name do not seem to explain its 
purpose to me. I assume it represents one of the 3 states you mention in 
the description? After trying to grok the code, I think something like 
_ongoing or _active would make more sense to me. But it also seems like 
we already have this information in ._local and ._other being None?

Anyway, shouldn't it be initialized in .reset()?

>           self._read()
>   
>       def reset(self, node=None, other=None):
>           self._state = {}
>           if node:
>               self._local = node
>               self._other = other
>           shutil.rmtree(self._repo.join("merge"), True)
> @@ -136,16 +137,17 @@ class mergestate(object):
>           try:
>               f = self._repo.opener(self.statepathv1)
>               for i, l in enumerate(f):
>                   if i == 0:
>                       records.append(('L', l[:-1]))
>                   else:
>                       records.append(('F', l[:-1]))
>               f.close()
> +            self._recordpresent = True

This is in a method that is a pretty pure function on the object and 
returns a value. It do not seem nice that it modifies the state. 
Instead, I would suggest returning None for 'no ongoing merge' and a 
(possibly empty) list for ongoing merges.

>           except IOError, err:
>               if err.errno != errno.ENOENT:
>                   raise
>           return records
>   
>       def _readrecordsv2(self):
>           """read on disk merge state for version 2 file
>   
> @@ -161,16 +163,17 @@ class mergestate(object):
>                   rtype = data[off]
>                   off += 1
>                   length = _unpack('>I', data[off:(off + 4)])[0]
>                   off += 4
>                   record = data[off:(off + length)]
>                   off += length
>                   records.append((rtype, record))
>               f.close()
> +            self._recordpresent = True
>           except IOError, err:
>               if err.errno != errno.ENOENT:
>                   raise
>           return records
>   
>       def commit(self):
>           """Write current state on disk (if necessary)"""
>           if self._dirty:
> @@ -181,16 +184,17 @@ class mergestate(object):
>                   records.append(("F", "\0".join([d] + v)))
>               self._writerecords(records)
>               self._dirty = False
>   
>       def _writerecords(self, records):
>           """Write current state on disk (both v1 and v2)"""
>           self._writerecordsv1(records)
>           self._writerecordsv2(records)
> +        self._recordpresent = True
>   
>       def _writerecordsv1(self, records):
>           """Write current state on disk in a version 1 file"""
>           f = self._repo.opener(self.statepathv1, "w")
>           irecords = iter(records)
>           lrecords = irecords.next()
>           assert lrecords[0] == 'L'
>           f.write(hex(self._local) + "\n")
> @@ -220,16 +224,22 @@ class mergestate(object):
>           hash = util.sha1(fcl.path()).hexdigest()
>           self._repo.opener.write("merge/" + hash, fcl.data())
>           self._state[fd] = ['u', hash, fcl.path(),
>                              fca.path(), hex(fca.filenode()),
>                              fco.path(), hex(fco.filenode()),
>                              fcl.flags()]
>           self._dirty = True
>   
> +    def __nonzero__(self):
> +        return self._recordpresent or bool(self._state)

and later:

      ms = mergemod.mergestate(repo)
+
+    if not ms:
+        raise util.Abort(_('no merge in progress; '
+                           'resolve command not applicable'))


I do not think the mergestate.__nonzero__ helps here. The code here 
would be more readable with an explict   if not ms.ongoing()   or 
.active() or something like that.


> +
> +    def __len__(self):
> +        return len(self._state)

AFAICS, this is not used yet. If I saw   len(ms)   (or an iterator over 
ms), I guess I would expect it to show the unresolved files. It would be 
ambiguous and it would be better to be explicit with a .states() and a 
.unresolved() or a .gimme(all=True).

/Mads