[PATCH 1 of 4] histedit: make verification configurable

[Yuya Nishihara]

On Thu, 12 Nov 2015 17:45:56 -0800, Mateusz Kwapich wrote:
> # HG changeset patch
> # User Mateusz Kwapich <mitrandir at fb.com>
> # Date 1447375340 28800
> #      Thu Nov 12 16:42:20 2015 -0800
> # Node ID 659c8c265d8516a5685ac226abc91aaa79028202
> # Parent  f9984f76fd90e439221425d751e29bae17bec995
> histedit: make verification configurable
> 
> Before we can add a 'base' action to histedit need to change verification
> so that action can specify which steps of verification should run for it.
> 
> Also it's everything we need for the exec and stop actions implementation.
> 
> I thought about baking verification into each histedit action (so each
> of them is responsible for verifying its constraints) but it felt wrong
> because:
>  - every action would need to know its context (eg. the list of all other
>    actions)
>  - a lot of duplicated work will be added - each action will iterate through
>    all others
>  - the steps of the verification would need to be extracted and named anyway
>    in order to be reused
> 
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -334,6 +334,24 @@
>              raise error.Abort(_('unknown changeset %s listed') % rulehash[:12])
>          return cls(state, node)
>  
> +    def constraints(self):
> +        """Return a set of constrains that this action should be verified for
> +
> +           Available constraints:
> +               noduplicates - aborts if there are multiple rules for one node
> +               noother - abort if the node doesn't belong to edited stack
> +        """
> +
> +        return set(['noduplicates', '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."""
> @@ -807,7 +825,7 @@
>              f.close()
>          rules = [l for l in (r.strip() for r in rules.splitlines())
>                   if l and not l.startswith('#')]
> -        rules = verifyrules(rules, repo, [repo[c] for [_a, c] in state.rules])
> +        rules = verifyrules(rules, state, [repo[c] for [_a, c] in state.rules])
>          state.rules = rules
>          state.write()
>          return
> @@ -888,7 +906,7 @@
>              f.close()
>          rules = [l for l in (r.strip() for r in rules.splitlines())
>                   if l and not l.startswith('#')]
> -        rules = verifyrules(rules, repo, ctxs)
> +        rules = verifyrules(rules, state, ctxs)
>  
>          parentctxnode = repo[root].parents()[0].node()
>  
> @@ -1038,7 +1056,7 @@
>  
>      return rules
>  
> -def verifyrules(rules, repo, ctxs):
> +def verifyrules(rules, state, ctxs):
>      """Verify that there exists exactly one edit rule per given changeset.
>  
>      Will abort if there are to many or too few rules, a malformed rule,
> @@ -1050,22 +1068,25 @@
>      for r in rules:
>          if ' ' not in r:
>              raise error.Abort(_('malformed line "%s"') % r)
> -        action, rest = r.split(' ', 1)
> -        ha = rest.strip().split(' ', 1)[0]
> -        try:
> -            ha = repo[ha].hex()
> -        except error.RepoError:
> -            raise error.Abort(_('unknown changeset %s listed') % ha[:12])
> -        if ha not in expected:
> -            raise error.Abort(
> -                _('may not use changesets other than the ones listed'))
> -        if ha in seen:
> -            raise error.Abort(_('duplicated command for changeset %s') %
> -                    ha[:12])
> -        seen.add(ha)
> -        if action not in actiontable or action.startswith('_'):
> -            raise error.Abort(_('unknown action "%s"') % action)
> -        parsed.append([action, ha])
> +        verb, rest = r.split(' ', 1)
> +
> +        if verb not in actiontable or verb.startswith('_'):
> +            raise error.Abort(_('unknown action "%s"') % verb)
> +        action = actiontable[verb].fromrule(state, rest)
> +        constraints = action.constraints()
> +        nodetoverify = action.nodetoverify()
> +        if nodetoverify is not None:
> +            ha = node.hex(nodetoverify)
> +            if 'noother' in constraints and ha not in expected:
> +                raise error.Abort(
> +                    _('may not use "%s" with changesets '
> +                      'other than the ones listed' % verb))
> +            if 'noduplicates' in constraints and ha in seen:
> +                raise error.Abort(_('duplicated command for changeset %s') %
> +                        ha[:12])

Can we detect unhandled constraints in some way? I think that is a common
mistake we do when we (ab)use a string as an enum-like constant.