Patchwork [4,of,4,shelve-ext,v2] shelve: rename nodestoprune to nodestoremove

login
register
mail settings
Submitter Kostia Balytskyi
Date April 10, 2017, 4:04 p.m.
Message ID <52a03106bbabb94fb62d.1491840292@devvm1416.lla2.facebook.com>
Download mbox | patch
Permalink /patch/20074/
State Changes Requested
Headers show

Comments

Kostia Balytskyi - April 10, 2017, 4:04 p.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1491839131 25200
#      Mon Apr 10 08:45:31 2017 -0700
# Node ID 52a03106bbabb94fb62d69d4a3eb76ef9614c134
# Parent  5c5d69830d176d7eb096c6ccb2f72377e13ace97
shelve: rename nodestoprune to nodestoremove

As per feedback from the community.
Ryan McElroy - April 10, 2017, 7:05 p.m.
On 4/10/17 5:04 PM, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1491839131 25200
> #      Mon Apr 10 08:45:31 2017 -0700
> # Node ID 52a03106bbabb94fb62d69d4a3eb76ef9614c134
> # Parent  5c5d69830d176d7eb096c6ccb2f72377e13ace97
> shelve: rename nodestoprune to nodestoremove

We really should rename this before introducing the key-value state 
file. Can you change the name after patch 1? Then there will never be a 
code point where the "broken" name is intserted into a statefile.

Overall, the direction of these patches makes sense, but there are 
enough little issues that I'll mark these as "changes requested" in 
patchwork.

In a v3, I'd like to see:

* typos cleaned up
* rename "prune" to "remove" before introducing key-value statefile
* "st" renamed to "state"

The rest was just random questions I had along the way.

This looks like a good improvement overall, I'm looking forward to v3.

>
> As per feedback from the community.
>
> diff --git a/hgext/shelve.py b/hgext/shelve.py
> --- a/hgext/shelve.py
> +++ b/hgext/shelve.py
> @@ -194,7 +194,7 @@ class shelvedstate(object):
>       def load(cls, repo):
>           # order is important, please do not change
>           keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
> -                'nodestoprune', 'branchtorestore', 'keep', 'activebook']
> +                'nodestoremove', 'branchtorestore', 'keep', 'activebook']
>           st = {}
>           if cls.iskeyvaluebased(repo):
>               st = scmutil.simplekeyvaluefile(repo.vfs, cls._filename).read()
> @@ -216,8 +216,8 @@ class shelvedstate(object):
>               st['pendingctx'] = nodemod.bin(st.get('pendingctx'))
>               st['parents'] = [nodemod.bin(h)
>                                for h in st.get('parents').split(' ')]
> -            st['nodestoprune'] = [nodemod.bin(h)
> -                                  for h in st.get('nodestoprune').split(' ')]
> +            st['nodestoremove'] = [nodemod.bin(h)
> +                                  for h in st.get('nodestoremove').split(' ')]
>           except (ValueError, TypeError, AttributeError) as err:
>               raise error.CorruptedState(str(err))
>   
> @@ -227,7 +227,7 @@ class shelvedstate(object):
>               obj.wctx = repo[st.get('originalwctx')]
>               obj.pendingctx = repo[st.get('pendingctx')]
>               obj.parents = st.get('parents')
> -            obj.nodestoprune = st.get('nodestoprune')
> +            obj.nodestoremove = st.get('nodestoremove')
>               obj.branchtorestore = st.get('branchtorestore')
>               obj.keep = st.get('keep') == cls._keep
>               obj.activebookmark = ''
> @@ -239,7 +239,7 @@ class shelvedstate(object):
>           return obj
>   
>       @classmethod
> -    def save(cls, repo, name, originalwctx, pendingctx, nodestoprune,
> +    def save(cls, repo, name, originalwctx, pendingctx, nodestoremove,
>                branchtorestore, keep=False, activebook=''):
>           if not repo.ui.configbool('shelve', 'oldstatefile', False):
>               info = {
> @@ -249,8 +249,8 @@ class shelvedstate(object):
>                   "pendingctx": nodemod.hex(pendingctx.node()),
>                   "parents": ' '.join([nodemod.hex(p)
>                                        for p in repo.dirstate.parents()]),
> -                "nodestoprune": ' '.join([nodemod.hex(n)
> -                                          for n in nodestoprune]),
> +                "nodestoremove": ' '.join([nodemod.hex(n)
> +                                           for n in nodestoremove]),
>                   "branchtorestore": branchtorestore,
>                   "keep": cls._keep if keep else cls._nokeep,
>                   "activebook": activebook or cls._noactivebook
> @@ -265,7 +265,7 @@ class shelvedstate(object):
>           fp.write('%s\n' %
>                    ' '.join([nodemod.hex(p) for p in repo.dirstate.parents()]))
>           fp.write('%s\n' %
> -                 ' '.join([nodemod.hex(n) for n in nodestoprune]))
> +                 ' '.join([nodemod.hex(n) for n in nodestoremove]))
>           fp.write('%s\n' % branchtorestore)
>           fp.write('%s\n' % (cls._keep if keep else cls._nokeep))
>           fp.write('%s\n' % (activebook or cls._noactivebook))
> @@ -277,7 +277,7 @@ class shelvedstate(object):
>   
>       def removenodes(self, ui, repo):
>           """Cleanup temporary nodes from the repo"""
> -        repair.strip(ui, repo, self.nodestoprune, backup=False,
> +        repair.strip(ui, repo, self.nodestoremove, backup=False,
>                        topic='shelve')
>   
>   def cleanupoldbackups(repo):
> @@ -695,7 +695,7 @@ def unshelvecontinue(ui, repo, state, op
>               shelvectx = state.pendingctx
>           else:
>               # only strip the shelvectx if the rebase produced it
> -            state.nodestoprune.append(shelvectx.node())
> +            state.nodestoremove.append(shelvectx.node())
>   
>           mergefiles(ui, repo, state.wctx, shelvectx)
>           restorebranch(ui, repo, state.branchtorestore)
> @@ -753,9 +753,9 @@ def _rebaserestoredcommit(ui, repo, opts
>       except error.InterventionRequired:
>           tr.close()
>   
> -        nodestoprune = [repo.changelog.node(rev)
> -                        for rev in xrange(oldtiprev, len(repo))]
> -        shelvedstate.save(repo, basename, pctx, tmpwctx, nodestoprune,
> +        nodestoremove = [repo.changelog.node(rev)
> +                         for rev in xrange(oldtiprev, len(repo))]
> +        shelvedstate.save(repo, basename, pctx, tmpwctx, nodestoremove,
>                             branchtorestore, opts.get('keep'), activebookmark)
>   
>           repo.vfs.rename('rebasestate', 'unshelverebasestate')
>
Kostia Balytskyi - April 10, 2017, 11:14 p.m.
> -----Original Message-----

> From: Ryan McElroy

> Sent: Monday, 10 April, 2017 20:05

> To: Kostia Balytskyi <ikostia@fb.com>; mercurial-devel@mercurial-scm.org

> Subject: Re: [PATCH 4 of 4 shelve-ext v2] shelve: rename nodestoprune to

> nodestoremove

> 

> On 4/10/17 5:04 PM, Kostia Balytskyi wrote:

> > # HG changeset patch

> > # User Kostia Balytskyi <ikostia@fb.com> # Date 1491839131 25200

> > #      Mon Apr 10 08:45:31 2017 -0700

> > # Node ID 52a03106bbabb94fb62d69d4a3eb76ef9614c134

> > # Parent  5c5d69830d176d7eb096c6ccb2f72377e13ace97

> > shelve: rename nodestoprune to nodestoremove

> 

> We really should rename this before introducing the key-value state file. Can

> you change the name after patch 1? Then there will never be a code point

> where the "broken" name is intserted into a statefile.

> 

> Overall, the direction of these patches makes sense, but there are enough

> little issues that I'll mark these as "changes requested" in patchwork.

> 

> In a v3, I'd like to see:

> 

> * typos cleaned up

> * rename "prune" to "remove" before introducing key-value statefile

> * "st" renamed to "state"

I don't want to use "state", it's overloaded. The class itself is a "shelvestate". I will use a "d = {}", to indicate that this is just a generic dictionary.
> 

> The rest was just random questions I had along the way.

> 

> This looks like a good improvement overall, I'm looking forward to v3.

> 

> >

> > As per feedback from the community.

> >

> > diff --git a/hgext/shelve.py b/hgext/shelve.py

> > --- a/hgext/shelve.py

> > +++ b/hgext/shelve.py

> > @@ -194,7 +194,7 @@ class shelvedstate(object):

> >       def load(cls, repo):

> >           # order is important, please do not change

> >           keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',

> > -                'nodestoprune', 'branchtorestore', 'keep', 'activebook']

> > +                'nodestoremove', 'branchtorestore', 'keep',

> > + 'activebook']

> >           st = {}

> >           if cls.iskeyvaluebased(repo):

> >               st = scmutil.simplekeyvaluefile(repo.vfs,

> > cls._filename).read() @@ -216,8 +216,8 @@ class shelvedstate(object):

> >               st['pendingctx'] = nodemod.bin(st.get('pendingctx'))

> >               st['parents'] = [nodemod.bin(h)

> >                                for h in st.get('parents').split(' ')]

> > -            st['nodestoprune'] = [nodemod.bin(h)

> > -                                  for h in st.get('nodestoprune').split(' ')]

> > +            st['nodestoremove'] = [nodemod.bin(h)

> > +                                  for h in

> > + st.get('nodestoremove').split(' ')]

> >           except (ValueError, TypeError, AttributeError) as err:

> >               raise error.CorruptedState(str(err))

> >

> > @@ -227,7 +227,7 @@ class shelvedstate(object):

> >               obj.wctx = repo[st.get('originalwctx')]

> >               obj.pendingctx = repo[st.get('pendingctx')]

> >               obj.parents = st.get('parents')

> > -            obj.nodestoprune = st.get('nodestoprune')

> > +            obj.nodestoremove = st.get('nodestoremove')

> >               obj.branchtorestore = st.get('branchtorestore')

> >               obj.keep = st.get('keep') == cls._keep

> >               obj.activebookmark = ''

> > @@ -239,7 +239,7 @@ class shelvedstate(object):

> >           return obj

> >

> >       @classmethod

> > -    def save(cls, repo, name, originalwctx, pendingctx, nodestoprune,

> > +    def save(cls, repo, name, originalwctx, pendingctx,

> > + nodestoremove,

> >                branchtorestore, keep=False, activebook=''):

> >           if not repo.ui.configbool('shelve', 'oldstatefile', False):

> >               info = {

> > @@ -249,8 +249,8 @@ class shelvedstate(object):

> >                   "pendingctx": nodemod.hex(pendingctx.node()),

> >                   "parents": ' '.join([nodemod.hex(p)

> >                                        for p in repo.dirstate.parents()]),

> > -                "nodestoprune": ' '.join([nodemod.hex(n)

> > -                                          for n in nodestoprune]),

> > +                "nodestoremove": ' '.join([nodemod.hex(n)

> > +                                           for n in nodestoremove]),

> >                   "branchtorestore": branchtorestore,

> >                   "keep": cls._keep if keep else cls._nokeep,

> >                   "activebook": activebook or cls._noactivebook @@

> > -265,7 +265,7 @@ class shelvedstate(object):

> >           fp.write('%s\n' %

> >                    ' '.join([nodemod.hex(p) for p in repo.dirstate.parents()]))

> >           fp.write('%s\n' %

> > -                 ' '.join([nodemod.hex(n) for n in nodestoprune]))

> > +                 ' '.join([nodemod.hex(n) for n in nodestoremove]))

> >           fp.write('%s\n' % branchtorestore)

> >           fp.write('%s\n' % (cls._keep if keep else cls._nokeep))

> >           fp.write('%s\n' % (activebook or cls._noactivebook)) @@

> > -277,7 +277,7 @@ class shelvedstate(object):

> >

> >       def removenodes(self, ui, repo):

> >           """Cleanup temporary nodes from the repo"""

> > -        repair.strip(ui, repo, self.nodestoprune, backup=False,

> > +        repair.strip(ui, repo, self.nodestoremove, backup=False,

> >                        topic='shelve')

> >

> >   def cleanupoldbackups(repo):

> > @@ -695,7 +695,7 @@ def unshelvecontinue(ui, repo, state, op

> >               shelvectx = state.pendingctx

> >           else:

> >               # only strip the shelvectx if the rebase produced it

> > -            state.nodestoprune.append(shelvectx.node())

> > +            state.nodestoremove.append(shelvectx.node())

> >

> >           mergefiles(ui, repo, state.wctx, shelvectx)

> >           restorebranch(ui, repo, state.branchtorestore) @@ -753,9

> > +753,9 @@ def _rebaserestoredcommit(ui, repo, opts

> >       except error.InterventionRequired:

> >           tr.close()

> >

> > -        nodestoprune = [repo.changelog.node(rev)

> > -                        for rev in xrange(oldtiprev, len(repo))]

> > -        shelvedstate.save(repo, basename, pctx, tmpwctx, nodestoprune,

> > +        nodestoremove = [repo.changelog.node(rev)

> > +                         for rev in xrange(oldtiprev, len(repo))]

> > +        shelvedstate.save(repo, basename, pctx, tmpwctx,

> > + nodestoremove,

> >                             branchtorestore, opts.get('keep'),

> > activebookmark)

> >

> >           repo.vfs.rename('rebasestate', 'unshelverebasestate')

> >

>

Patch

diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -194,7 +194,7 @@  class shelvedstate(object):
     def load(cls, repo):
         # order is important, please do not change
         keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
-                'nodestoprune', 'branchtorestore', 'keep', 'activebook']
+                'nodestoremove', 'branchtorestore', 'keep', 'activebook']
         st = {}
         if cls.iskeyvaluebased(repo):
             st = scmutil.simplekeyvaluefile(repo.vfs, cls._filename).read()
@@ -216,8 +216,8 @@  class shelvedstate(object):
             st['pendingctx'] = nodemod.bin(st.get('pendingctx'))
             st['parents'] = [nodemod.bin(h)
                              for h in st.get('parents').split(' ')]
-            st['nodestoprune'] = [nodemod.bin(h)
-                                  for h in st.get('nodestoprune').split(' ')]
+            st['nodestoremove'] = [nodemod.bin(h)
+                                  for h in st.get('nodestoremove').split(' ')]
         except (ValueError, TypeError, AttributeError) as err:
             raise error.CorruptedState(str(err))
 
@@ -227,7 +227,7 @@  class shelvedstate(object):
             obj.wctx = repo[st.get('originalwctx')]
             obj.pendingctx = repo[st.get('pendingctx')]
             obj.parents = st.get('parents')
-            obj.nodestoprune = st.get('nodestoprune')
+            obj.nodestoremove = st.get('nodestoremove')
             obj.branchtorestore = st.get('branchtorestore')
             obj.keep = st.get('keep') == cls._keep
             obj.activebookmark = ''
@@ -239,7 +239,7 @@  class shelvedstate(object):
         return obj
 
     @classmethod
-    def save(cls, repo, name, originalwctx, pendingctx, nodestoprune,
+    def save(cls, repo, name, originalwctx, pendingctx, nodestoremove,
              branchtorestore, keep=False, activebook=''):
         if not repo.ui.configbool('shelve', 'oldstatefile', False):
             info = {
@@ -249,8 +249,8 @@  class shelvedstate(object):
                 "pendingctx": nodemod.hex(pendingctx.node()),
                 "parents": ' '.join([nodemod.hex(p)
                                      for p in repo.dirstate.parents()]),
-                "nodestoprune": ' '.join([nodemod.hex(n)
-                                          for n in nodestoprune]),
+                "nodestoremove": ' '.join([nodemod.hex(n)
+                                           for n in nodestoremove]),
                 "branchtorestore": branchtorestore,
                 "keep": cls._keep if keep else cls._nokeep,
                 "activebook": activebook or cls._noactivebook
@@ -265,7 +265,7 @@  class shelvedstate(object):
         fp.write('%s\n' %
                  ' '.join([nodemod.hex(p) for p in repo.dirstate.parents()]))
         fp.write('%s\n' %
-                 ' '.join([nodemod.hex(n) for n in nodestoprune]))
+                 ' '.join([nodemod.hex(n) for n in nodestoremove]))
         fp.write('%s\n' % branchtorestore)
         fp.write('%s\n' % (cls._keep if keep else cls._nokeep))
         fp.write('%s\n' % (activebook or cls._noactivebook))
@@ -277,7 +277,7 @@  class shelvedstate(object):
 
     def removenodes(self, ui, repo):
         """Cleanup temporary nodes from the repo"""
-        repair.strip(ui, repo, self.nodestoprune, backup=False,
+        repair.strip(ui, repo, self.nodestoremove, backup=False,
                      topic='shelve')
 
 def cleanupoldbackups(repo):
@@ -695,7 +695,7 @@  def unshelvecontinue(ui, repo, state, op
             shelvectx = state.pendingctx
         else:
             # only strip the shelvectx if the rebase produced it
-            state.nodestoprune.append(shelvectx.node())
+            state.nodestoremove.append(shelvectx.node())
 
         mergefiles(ui, repo, state.wctx, shelvectx)
         restorebranch(ui, repo, state.branchtorestore)
@@ -753,9 +753,9 @@  def _rebaserestoredcommit(ui, repo, opts
     except error.InterventionRequired:
         tr.close()
 
-        nodestoprune = [repo.changelog.node(rev)
-                        for rev in xrange(oldtiprev, len(repo))]
-        shelvedstate.save(repo, basename, pctx, tmpwctx, nodestoprune,
+        nodestoremove = [repo.changelog.node(rev)
+                         for rev in xrange(oldtiprev, len(repo))]
+        shelvedstate.save(repo, basename, pctx, tmpwctx, nodestoremove,
                           branchtorestore, opts.get('keep'), activebookmark)
 
         repo.vfs.rename('rebasestate', 'unshelverebasestate')