Patchwork [1,of,8,cleanup] histedit: drop the 'nodetoverify' method

login
register
mail settings
Submitter Pierre-Yves David
Date Aug. 26, 2016, 7:35 p.m.
Message ID <971ddb8990c6617925d3.1472240126@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/16461/
State Accepted
Headers show

Comments

Pierre-Yves David - Aug. 26, 2016, 7:35 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1472236293 -7200
#      Fri Aug 26 20:31:33 2016 +0200
# Node ID 971ddb8990c6617925d3403dd4865ca77be3db59
# Parent  4435d4c951ec2185d92cbe0041852767dda584c8
# EXP-Topic histedit.constraint
histedit: drop the 'nodetoverify' method

That method is just returning self.node and is never overridden. We just use
the attribute directly instead and get rid of the method.

This is the beginning of series to simplify and unify verification of constrains
for actions.
Augie Fackler - Aug. 26, 2016, 7:54 p.m.
> On Aug 26, 2016, at 15:35, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1472236293 -7200
> #      Fri Aug 26 20:31:33 2016 +0200
> # Node ID 971ddb8990c6617925d3403dd4865ca77be3db59
> # Parent  4435d4c951ec2185d92cbe0041852767dda584c8
> # EXP-Topic histedit.constraint
> histedit: drop the 'nodetoverify' method

I believe this hook is in place so that exec can work. Mateusz, Durham?

> 
> That method is just returning self.node and is never overridden. We just use
> the attribute directly instead and get rid of the method.
> 
> This is the beginning of series to simplify and unify verification of constrains
> for actions.
> 
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -439,14 +439,6 @@ class histeditaction(object):
>         """
>         return set([_constraints.noduplicates, _constraints.noother])
> 
> -    def nodetoverify(self):
> -        """Returns a node associated with the action that will be used for
> -        verification purposes.
> -
> -        If the action doesn't correspond to node it should return None
> -        """
> -        return self.node
> -
>     def run(self):
>         """Runs the action. The default behavior is simply apply the action's
>         rulectx onto the current parentctx."""
> @@ -1199,8 +1191,8 @@ def _edithisteditplan(ui, repo, state, r
>     else:
>         rules = _readfile(rules)
>     actions = parserules(rules, state)
> -    ctxs = [repo[act.nodetoverify()] \
> -            for act in state.actions if act.nodetoverify()]
> +    ctxs = [repo[act.node] \
> +            for act in state.actions if act.node]
>     warnverifyactions(ui, repo, actions, state, ctxs)
>     state.actions = actions
>     state.write()
> @@ -1397,7 +1389,7 @@ def verifyactions(actions, state, ctxs):
>                 raise error.ParseError(_('unknown constraint "%s"') %
>                         constraint)
> 
> -        nodetoverify = action.nodetoverify()
> +        nodetoverify = action.node
>         if nodetoverify is not None:
>             ha = node.hex(nodetoverify)
>             if _constraints.noother in constraints and ha not in expected:
> @@ -1597,8 +1589,8 @@ def stripwrapper(orig, ui, repo, nodelis
>     if os.path.exists(os.path.join(repo.path, 'histedit-state')):
>         state = histeditstate(repo)
>         state.read()
> -        histedit_nodes = set([action.nodetoverify() for action
> -                             in state.actions if action.nodetoverify()])
> +        histedit_nodes = set([action.node for action
> +                             in state.actions if action.node])
>         strip_nodes = set([repo[n].node() for n in nodelist])
>         common_nodes = histedit_nodes & strip_nodes
>         if common_nodes:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
timeless - Aug. 26, 2016, 7:57 p.m.
That sounds familiar. If that's the reason, it needs to be documented,
otherwise someone will eventually perform this cleanup.

On Fri, Aug 26, 2016 at 3:54 PM, Augie Fackler <raf@durin42.com> wrote:
>
>> On Aug 26, 2016, at 15:35, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1472236293 -7200
>> #      Fri Aug 26 20:31:33 2016 +0200
>> # Node ID 971ddb8990c6617925d3403dd4865ca77be3db59
>> # Parent  4435d4c951ec2185d92cbe0041852767dda584c8
>> # EXP-Topic histedit.constraint
>> histedit: drop the 'nodetoverify' method
>
> I believe this hook is in place so that exec can work. Mateusz, Durham?
>
>>
>> That method is just returning self.node and is never overridden. We just use
>> the attribute directly instead and get rid of the method.
>>
>> This is the beginning of series to simplify and unify verification of constrains
>> for actions.
>>
>> diff --git a/hgext/histedit.py b/hgext/histedit.py
>> --- a/hgext/histedit.py
>> +++ b/hgext/histedit.py
>> @@ -439,14 +439,6 @@ class histeditaction(object):
>>         """
>>         return set([_constraints.noduplicates, _constraints.noother])
>>
>> -    def nodetoverify(self):
>> -        """Returns a node associated with the action that will be used for
>> -        verification purposes.
>> -
>> -        If the action doesn't correspond to node it should return None
>> -        """
>> -        return self.node
>> -
>>     def run(self):
>>         """Runs the action. The default behavior is simply apply the action's
>>         rulectx onto the current parentctx."""
>> @@ -1199,8 +1191,8 @@ def _edithisteditplan(ui, repo, state, r
>>     else:
>>         rules = _readfile(rules)
>>     actions = parserules(rules, state)
>> -    ctxs = [repo[act.nodetoverify()] \
>> -            for act in state.actions if act.nodetoverify()]
>> +    ctxs = [repo[act.node] \
>> +            for act in state.actions if act.node]
>>     warnverifyactions(ui, repo, actions, state, ctxs)
>>     state.actions = actions
>>     state.write()
>> @@ -1397,7 +1389,7 @@ def verifyactions(actions, state, ctxs):
>>                 raise error.ParseError(_('unknown constraint "%s"') %
>>                         constraint)
>>
>> -        nodetoverify = action.nodetoverify()
>> +        nodetoverify = action.node
>>         if nodetoverify is not None:
>>             ha = node.hex(nodetoverify)
>>             if _constraints.noother in constraints and ha not in expected:
>> @@ -1597,8 +1589,8 @@ def stripwrapper(orig, ui, repo, nodelis
>>     if os.path.exists(os.path.join(repo.path, 'histedit-state')):
>>         state = histeditstate(repo)
>>         state.read()
>> -        histedit_nodes = set([action.nodetoverify() for action
>> -                             in state.actions if action.nodetoverify()])
>> +        histedit_nodes = set([action.node for action
>> +                             in state.actions if action.node])
>>         strip_nodes = set([repo[n].node() for n in nodelist])
>>         common_nodes = histedit_nodes & strip_nodes
>>         if common_nodes:
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - Aug. 26, 2016, 7:58 p.m.
> On Aug 26, 2016, at 15:57, timeless <timeless@gmail.com> wrote:
> 
> That sounds familiar. If that's the reason, it needs to be documented,
> otherwise someone will eventually perform this cleanup.

Agreed, though it might be time to move exec into core histedit as well, which would also resolve this.

> 
> On Fri, Aug 26, 2016 at 3:54 PM, Augie Fackler <raf@durin42.com> wrote:
>> 
>>> On Aug 26, 2016, at 15:35, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>> 
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>>> # Date 1472236293 -7200
>>> #      Fri Aug 26 20:31:33 2016 +0200
>>> # Node ID 971ddb8990c6617925d3403dd4865ca77be3db59
>>> # Parent  4435d4c951ec2185d92cbe0041852767dda584c8
>>> # EXP-Topic histedit.constraint
>>> histedit: drop the 'nodetoverify' method
>> 
>> I believe this hook is in place so that exec can work. Mateusz, Durham?
>> 
>>> 
>>> That method is just returning self.node and is never overridden. We just use
>>> the attribute directly instead and get rid of the method.
>>> 
>>> This is the beginning of series to simplify and unify verification of constrains
>>> for actions.
>>> 
>>> diff --git a/hgext/histedit.py b/hgext/histedit.py
>>> --- a/hgext/histedit.py
>>> +++ b/hgext/histedit.py
>>> @@ -439,14 +439,6 @@ class histeditaction(object):
>>>        """
>>>        return set([_constraints.noduplicates, _constraints.noother])
>>> 
>>> -    def nodetoverify(self):
>>> -        """Returns a node associated with the action that will be used for
>>> -        verification purposes.
>>> -
>>> -        If the action doesn't correspond to node it should return None
>>> -        """
>>> -        return self.node
>>> -
>>>    def run(self):
>>>        """Runs the action. The default behavior is simply apply the action's
>>>        rulectx onto the current parentctx."""
>>> @@ -1199,8 +1191,8 @@ def _edithisteditplan(ui, repo, state, r
>>>    else:
>>>        rules = _readfile(rules)
>>>    actions = parserules(rules, state)
>>> -    ctxs = [repo[act.nodetoverify()] \
>>> -            for act in state.actions if act.nodetoverify()]
>>> +    ctxs = [repo[act.node] \
>>> +            for act in state.actions if act.node]
>>>    warnverifyactions(ui, repo, actions, state, ctxs)
>>>    state.actions = actions
>>>    state.write()
>>> @@ -1397,7 +1389,7 @@ def verifyactions(actions, state, ctxs):
>>>                raise error.ParseError(_('unknown constraint "%s"') %
>>>                        constraint)
>>> 
>>> -        nodetoverify = action.nodetoverify()
>>> +        nodetoverify = action.node
>>>        if nodetoverify is not None:
>>>            ha = node.hex(nodetoverify)
>>>            if _constraints.noother in constraints and ha not in expected:
>>> @@ -1597,8 +1589,8 @@ def stripwrapper(orig, ui, repo, nodelis
>>>    if os.path.exists(os.path.join(repo.path, 'histedit-state')):
>>>        state = histeditstate(repo)
>>>        state.read()
>>> -        histedit_nodes = set([action.nodetoverify() for action
>>> -                             in state.actions if action.nodetoverify()])
>>> +        histedit_nodes = set([action.node for action
>>> +                             in state.actions if action.node])
>>>        strip_nodes = set([repo[n].node() for n in nodelist])
>>>        common_nodes = histedit_nodes & strip_nodes
>>>        if common_nodes:
>>> _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel@mercurial-scm.org
>>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> 
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Aug. 26, 2016, 8:26 p.m.
On 08/26/2016 09:54 PM, Augie Fackler wrote:
>
>> On Aug 26, 2016, at 15:35, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1472236293 -7200
>> #      Fri Aug 26 20:31:33 2016 +0200
>> # Node ID 971ddb8990c6617925d3403dd4865ca77be3db59
>> # Parent  4435d4c951ec2185d92cbe0041852767dda584c8
>> # EXP-Topic histedit.constraint
>> histedit: drop the 'nodetoverify' method
>
> I believe this hook is in place so that exec can work. Mateusz, Durham?

There is not documentation about that nor obvious reason why exec could 
not managed without the method.
Durham Goode - Aug. 27, 2016, 6:15 p.m.
On 8/26/16 1:26 PM, Pierre-Yves David wrote:
>
> On 08/26/2016 09:54 PM, Augie Fackler wrote:
>>
>>> On Aug 26, 2016, at 15:35, Pierre-Yves David 
>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>>> # Date 1472236293 -7200
>>> #      Fri Aug 26 20:31:33 2016 +0200
>>> # Node ID 971ddb8990c6617925d3403dd4865ca77be3db59
>>> # Parent  4435d4c951ec2185d92cbe0041852767dda584c8
>>> # EXP-Topic histedit.constraint
>>> histedit: drop the 'nodetoverify' method
>>
>> I believe this hook is in place so that exec can work. Mateusz, Durham?
>
> There is not documentation about that nor obvious reason why exec 
> could not managed without the method.
>
Yes, this is for exec.  So we can have actions in the list that don't 
have node's associated with them.  I can send a patch adding that 
comment to the function description, or you can do it in your stack if 
you're already in the area.
Pierre-Yves David - Aug. 28, 2016, 1:14 a.m.
On 08/27/2016 08:15 PM, Durham Goode wrote:
> On 8/26/16 1:26 PM, Pierre-Yves David wrote:
>>
>> On 08/26/2016 09:54 PM, Augie Fackler wrote:
>>>
>>>> On Aug 26, 2016, at 15:35, Pierre-Yves David
>>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>>>> # Date 1472236293 -7200
>>>> #      Fri Aug 26 20:31:33 2016 +0200
>>>> # Node ID 971ddb8990c6617925d3403dd4865ca77be3db59
>>>> # Parent  4435d4c951ec2185d92cbe0041852767dda584c8
>>>> # EXP-Topic histedit.constraint
>>>> histedit: drop the 'nodetoverify' method
>>>
>>> I believe this hook is in place so that exec can work. Mateusz, Durham?
>>
>> There is not documentation about that nor obvious reason why exec
>> could not managed without the method.
>>
> Yes, this is for exec.  So we can have actions in the list that don't
> have node's associated with them.  I can send a patch adding that
> comment to the function description, or you can do it in your stack if
> you're already in the area.

This change does not make it harder for exec. If action.node is None, 
the code will behave as today.

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -439,14 +439,6 @@  class histeditaction(object):
         """
         return set([_constraints.noduplicates, _constraints.noother])
 
-    def nodetoverify(self):
-        """Returns a node associated with the action that will be used for
-        verification purposes.
-
-        If the action doesn't correspond to node it should return None
-        """
-        return self.node
-
     def run(self):
         """Runs the action. The default behavior is simply apply the action's
         rulectx onto the current parentctx."""
@@ -1199,8 +1191,8 @@  def _edithisteditplan(ui, repo, state, r
     else:
         rules = _readfile(rules)
     actions = parserules(rules, state)
-    ctxs = [repo[act.nodetoverify()] \
-            for act in state.actions if act.nodetoverify()]
+    ctxs = [repo[act.node] \
+            for act in state.actions if act.node]
     warnverifyactions(ui, repo, actions, state, ctxs)
     state.actions = actions
     state.write()
@@ -1397,7 +1389,7 @@  def verifyactions(actions, state, ctxs):
                 raise error.ParseError(_('unknown constraint "%s"') %
                         constraint)
 
-        nodetoverify = action.nodetoverify()
+        nodetoverify = action.node
         if nodetoverify is not None:
             ha = node.hex(nodetoverify)
             if _constraints.noother in constraints and ha not in expected:
@@ -1597,8 +1589,8 @@  def stripwrapper(orig, ui, repo, nodelis
     if os.path.exists(os.path.join(repo.path, 'histedit-state')):
         state = histeditstate(repo)
         state.read()
-        histedit_nodes = set([action.nodetoverify() for action
-                             in state.actions if action.nodetoverify()])
+        histedit_nodes = set([action.node for action
+                             in state.actions if action.node])
         strip_nodes = set([repo[n].node() for n in nodelist])
         common_nodes = histedit_nodes & strip_nodes
         if common_nodes: