| 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
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
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?
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)