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

login
register
mail settings
Submitter Gregory Szorc
Date May 2, 2014, 5:47 a.m.
Message ID <b4d95342be6de3e3a229.1399009658@vm-ubuntu-main.gateway.sonic.net>
Download mbox | patch
Permalink /patch/4488/
State Superseded
Headers show

Comments

Gregory Szorc - May 2, 2014, 5:47 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@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.
Mads Kiilerich - May 2, 2014, 1:15 p.m.
On 05/02/2014 07:47 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@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
Gregory Szorc - May 3, 2014, 9:29 p.m.
I agree with the points raised in this review. I've resent the patch
series with this patch refactored into mergestate.active().

On 5/2/2014 6:15 AM, Mads Kiilerich wrote:
> On 05/02/2014 07:47 AM, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@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

Patch

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
         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
         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)
+
+    def __len__(self):
+        return len(self._state)
+
     def __contains__(self, dfile):
         return dfile in self._state
 
     def __getitem__(self, dfile):
         return self._state[dfile][0]
 
     def __iter__(self):
         l = self._state.keys()