Patchwork [v2] histedit: use one editor when multiple folds happen in a row (issue3524) (BC)

login
register
mail settings
Submitter Augie Fackler
Date Sept. 1, 2015, 8:02 p.m.
Message ID <CAPLqtW+Go_1k=3UDir0d+8i8YUJZgJGtA+1toE=XQDmVVBZTXw@mail.gmail.com>
Download mbox | patch
Permalink /patch/10357/
State Superseded
Commit bf81b696b8f4967a4b9b65c842f456cc8a3ca9b4
Delegated to: Augie Fackler
Headers show

Comments

Augie Fackler - Sept. 1, 2015, 8:02 p.m.
On Tue, Sep 1, 2015 at 3:59 PM, Augie Fackler <raf@durin42.com> wrote:
> On Tue, Sep 1, 2015 at 1:23 PM, Durham Goode <durham@fb.com> wrote:
>> I agree, it's a little beyond what we need, but given histedit's history, I
>> think it'd be worth doing.
>>
>> It might also enable other interesting things from extensions.  For
>> instance, if we wanted to remove the need to specify 'drop' lines, we could
>> have the drop action class preprocess the list to add 'drop' entries when
>> the user has deleted lines.  Or if we wanted to add a 'review' action, the
>> action could be implemented as a preprocessor that replaces 'review' actions
>> with 'exec run_my_review_tool'
>
> So, I've started working on this, and I'm of the opinion that it's
> something we should do later. Reason being, it's underspecified:
>
> How do I know what order to run the preprocessors in? How do I know
> that my preprocessor is going to run late enough in the stack to do
> what I want? Or early enough?
>
> I think we should wait until there's a concrete second user to
> actually do this refactor. Note that I'm *fine* with that second user
> being out of tree, I just can't quite figure out what the Right API is
> going to be today because my crystal ball is still broken.

More concretely, here's the patch. I'm just unconvinced of the value
of this design because it offers near-zero guarantees to future
implementors, which suggests to me we'd have to do meaningful
refactoring *anyway*, so I'd rather leave the patch as it stands in v2
unless you see a clear path forward on the extra complexity that's
likely to not require an extra round of refactoring anyway.

# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1441137665 14400
#      Tue Sep 01 16:01:05 2015 -0400
# Node ID 89f41d5c7aaa22947065bd5fb5e95a112198034d
# Parent  6d4eb98524de846543455102dfe4aaa3c8c8af45
histedit: move rule preprocessing into a classmethod

This theoretically opens the door to some neat ideas in extensions
around mutating the ruleset before the histedit actually starts, but
at present would have no users.

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -330,6 +330,11 @@  class histeditaction(object):
             raise util.Abort(_('unknown changeset %s listed') % rulehash[:12])
         return cls(state, node)

+    @classmethod
+    def preprocess(cls, rules):
+        """Preprocess rules and potentially transform the ruleset."""
+        return rules
+
     def run(self):
         """Runs the action. The default behavior is simply apply the action's
         rulectx onto the current parentctx."""
@@ -616,6 +621,20 @@  class _multifold(fold):
     def skipprompt(self):
         return True

+    @classmethod
+    def preprocess(cls, rules):
+        """Preprocess rules and potentially transform the ruleset.
+
+        Note that at present there are no guarantees around the
+        ordering of the preprocessing actions taken.
+        """
+        newrules = rules[:]
+        for idx, ((action, ha), (nextact, unused)) in enumerate(
+                zip(rules, rules[1:] + [(None, None)])):
+            if action == 'fold' and nextact == 'fold':
+                newrules[idx] = '_multifold', ha
+        rules[:] = newrules[:]
+
 class rollup(fold):
     def mergedescs(self):
         return False
@@ -869,13 +888,8 @@  def _histedit(ui, repo, state, *freeargs
                                         'histedit')
         state.backupfile = backupfile

-    # preprocess rules so that we can hide inner folds from the user
-    # and only show one editor
-    rules = state.rules[:]
-    for idx, ((action, ha), (nextact, unused)) in enumerate(
-            zip(rules, rules[1:] + [(None, None)])):
-        if action == 'fold' and nextact == 'fold':
-            state.rules[idx] = '_multifold', ha
+    for ruletype in set(actiontable.values()):
+        ruletype.preprocess(state.rules)

     while state.rules:
         state.write()