Patchwork [1,of,6] mergestate: consistently set variables to None

login
register
mail settings
Submitter Gregory Szorc
Date May 9, 2014, 1:46 a.m.
Message ID <a3f7aa832261e32c5fc3.1399599979@vm-ubuntu-main.gateway.sonic.net>
Download mbox | patch
Permalink /patch/4691/
State Accepted
Headers show

Comments

Gregory Szorc - May 9, 2014, 1:46 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1399592908 25200
#      Thu May 08 16:48:28 2014 -0700
# Node ID a3f7aa832261e32c5fc3f8b88285034a8329fc38
# Parent  62a2749895e4151f766a4243fa870b1ddd7386d0
mergestate: consistently set variables to None

Some code branches and exceptional circumstances such as empty
mergestate files could result in mergestate._local and
mergestate._other not being defined or reset to None. These variables
are now correctly set to None when they should be.
Pierre-Yves David - May 9, 2014, 1:53 a.m.
On 05/08/2014 06:46 PM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1399592908 25200
> #      Thu May 08 16:48:28 2014 -0700
> # Node ID a3f7aa832261e32c5fc3f8b88285034a8329fc38
> # Parent  62a2749895e4151f766a4243fa870b1ddd7386d0
> mergestate: consistently set variables to None
>
> Some code branches and exceptional circumstances such as empty
> mergestate files could result in mergestate._local and
> mergestate._other not being defined or reset to None. These variables
> are now correctly set to None when they should be.
>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -57,8 +57,11 @@ class mergestate(object):
>           self._state = {}
>           if node:
>               self._local = node
>               self._other = other
> +        else:
> +            self._local = None
> +            self._other = None

The prefered style within the mercurial code base is:

variable = defaultvalue
if special case:
     variable = special value
Gregory Szorc - May 9, 2014, 2:04 a.m.
On 5/8/2014 6:53 PM, Pierre-Yves David wrote:
> 
> 
> On 05/08/2014 06:46 PM, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1399592908 25200
>> #      Thu May 08 16:48:28 2014 -0700
>> # Node ID a3f7aa832261e32c5fc3f8b88285034a8329fc38
>> # Parent  62a2749895e4151f766a4243fa870b1ddd7386d0
>> mergestate: consistently set variables to None
>>
>> Some code branches and exceptional circumstances such as empty
>> mergestate files could result in mergestate._local and
>> mergestate._other not being defined or reset to None. These variables
>> are now correctly set to None when they should be.
>>
>> diff --git a/mercurial/merge.py b/mercurial/merge.py
>> --- a/mercurial/merge.py
>> +++ b/mercurial/merge.py
>> @@ -57,8 +57,11 @@ class mergestate(object):
>>           self._state = {}
>>           if node:
>>               self._local = node
>>               self._other = other
>> +        else:
>> +            self._local = None
>> +            self._other = None
> 
> The prefered style within the mercurial code base is:
> 
> variable = defaultvalue
> if special case:
>     variable = special value

Noted.

Is this something I need to correct and respam to the list? Or is it
something a reviewer will correct before checkin?
Pierre-Yves David - May 9, 2014, 2:08 a.m.
On 05/08/2014 07:04 PM, Gregory Szorc wrote:
> On 5/8/2014 6:53 PM, Pierre-Yves David wrote:
>>
>>
>> On 05/08/2014 06:46 PM, Gregory Szorc wrote:
>>> # HG changeset patch
>>> # User Gregory Szorc <gregory.szorc@gmail.com>
>>> # Date 1399592908 25200
>>> #      Thu May 08 16:48:28 2014 -0700
>>> # Node ID a3f7aa832261e32c5fc3f8b88285034a8329fc38
>>> # Parent  62a2749895e4151f766a4243fa870b1ddd7386d0
>>> mergestate: consistently set variables to None
>>>
>>> Some code branches and exceptional circumstances such as empty
>>> mergestate files could result in mergestate._local and
>>> mergestate._other not being defined or reset to None. These variables
>>> are now correctly set to None when they should be.
>>>
>>> diff --git a/mercurial/merge.py b/mercurial/merge.py
>>> --- a/mercurial/merge.py
>>> +++ b/mercurial/merge.py
>>> @@ -57,8 +57,11 @@ class mergestate(object):
>>>            self._state = {}
>>>            if node:
>>>                self._local = node
>>>                self._other = other
>>> +        else:
>>> +            self._local = None
>>> +            self._other = None
>>
>> The prefered style within the mercurial code base is:
>>
>> variable = defaultvalue
>> if special case:
>>      variable = special value
>
> Noted.
>
> Is this something I need to correct and respam to the list? Or is it
> something a reviewer will correct before checkin?

depends of the reviewer mood and complexity in the series.

In that case I fixed it myself.

I should have clearly stated the unnecessary of a resend, sorry.

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -57,8 +57,11 @@  class mergestate(object):
         self._state = {}
         if node:
             self._local = node
             self._other = other
+        else:
+            self._local = None
+            self._other = None
         shutil.rmtree(self._repo.join("merge"), True)
         self._dirty = False
 
     def _read(self):
@@ -67,8 +70,10 @@  class mergestate(object):
         This function process "record" entry produced by the de-serialization
         of on disk file.
         """
         self._state = {}
+        self._local = None
+        self._other = None
         records = self._readrecords()
         for rtype, record in records:
             if rtype == 'L':
                 self._local = bin(record)