[PATCH 1 of 4 V2] histedit: make verification configurable
Durham Goode
durham at fb.com
Fri Nov 20 14:51:04 CST 2015
On 11/17/15 5:55 PM, Mateusz Kwapich wrote:
> # HG changeset patch
> # User Mateusz Kwapich <mitrandir at fb.com>
> # Date 1447807046 28800
> # Tue Nov 17 16:37:26 2015 -0800
> # Node ID 2aa95a3b1dcb9921c65a74228306fc1693dbf1d2
> # Parent 2da6a2dbfc42bdec4bcaf47da947c64ff959a59c
> 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
>
> The verifyrules function grows too big now. I plan to refator it in one of
> the next series.
>
> 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'])
Can we make these into constants? Since we're now referring to them in
three places and it's likely extension might also need to reference them.
> +
> + 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,34 +1056,42 @@
>
> 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,
> or a rule on a changeset outside of the user-given range.
> """
> + known_constraints = ['noother', 'noduplicates']
This could be global as well, so extensions can modify it.
> parsed = []
> expected = set(c.hex() for c in ctxs)
> seen = set()
> 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()
> + for constraint in constraints:
> + if constraint not in known_constraints:
> + error.Abort(_('uknown constraint "%s"') % constraint)
I think you're missing a 'raise' here. Also s/uknown/unknown/
> +
> + 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])
> + seen.add(ha)
> + rest = ha
> + parsed.append([verb, rest])
> missing = sorted(expected - seen) # sort to stabilize output
> if missing:
> raise error.Abort(_('missing rules for changeset %s') %
> diff --git a/tests/test-histedit-arguments.t b/tests/test-histedit-arguments.t
> --- a/tests/test-histedit-arguments.t
> +++ b/tests/test-histedit-arguments.t
> @@ -175,7 +175,7 @@
> > pick c8e68270e35a 3 four
> > pick 08d98a8350f3 4 five
> > EOF
> - abort: may not use changesets other than the ones listed
> + abort: may not use "pick" with changesets other than the ones listed
> [255]
>
> Test malformed line
> diff --git a/tests/test-histedit-commute.t b/tests/test-histedit-commute.t
> --- a/tests/test-histedit-commute.t
> +++ b/tests/test-histedit-commute.t
> @@ -281,7 +281,7 @@
> > pick de71b079d9ce e
> > pick 38b92f448761 c
> > EOF
> - abort: may not use changesets other than the ones listed
> + abort: may not use "pick" with changesets other than the ones listed
> $ hg log --graph
> @ changeset: 7:803ef1c6fcfd
> | tag: tip
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
More information about the Mercurial-devel
mailing list