[PATCH 1 of 4] histedit: make verification configurable

[Augie Fackler]

On Mon, Nov 16, 2015 at 11:10:42PM +0900, Yuya Nishihara wrote:
> 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.

Yes, we should hard-fail if there's a constraint that we don't know about.

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel