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, 4:56 a.m.
Message ID <6d4eb98524de84654345.1441083388@imladris.local>
Download mbox | patch
Permalink /patch/10339/
State Superseded
Commit bf81b696b8f4967a4b9b65c842f456cc8a3ca9b4
Delegated to: Augie Fackler
Headers show

Comments

Augie Fackler - Sept. 1, 2015, 4:56 a.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1440701186 14400
#      Thu Aug 27 14:46:26 2015 -0400
# Node ID 6d4eb98524de846543455102dfe4aaa3c8c8af45
# Parent  049005de325ea400893f45bd6221215cc9b26db0
histedit: use one editor when multiple folds happen in a row (issue3524) (BC)

This was the first ever feature request for histedit, originally filed
back on April 4, 2009. Finally fixed.
Durham Goode - Sept. 1, 2015, 5:13 p.m.
On 8/31/15 9:56 PM, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1440701186 14400
> #      Thu Aug 27 14:46:26 2015 -0400
> # Node ID 6d4eb98524de846543455102dfe4aaa3c8c8af45
> # Parent  049005de325ea400893f45bd6221215cc9b26db0
> histedit: use one editor when multiple folds happen in a row (issue3524) (BC)
>
> This was the first ever feature request for histedit, originally filed
> back on April 4, 2009. Finally fixed.
>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -559,6 +559,9 @@ class fold(histeditaction):
>       def skipprompt(self):
>           return False
>   
> +    def mergedescs(self):
> +        return True
> +
>       def finishfold(self, ui, repo, ctx, oldctx, newnode, internalchanges):
>           parent = ctx.parents()[0].node()
>           hg.update(repo, parent)
> @@ -566,7 +569,7 @@ class fold(histeditaction):
>           commitopts = {}
>           commitopts['user'] = ctx.user()
>           # commit message
> -        if self.skipprompt():
> +        if not self.mergedescs():
>               newmessage = ctx.description()
>           else:
>               newmessage = '\n***\n'.join(
> @@ -601,7 +604,22 @@ class fold(histeditaction):
>               replacements.append((ich, (n,)))
>           return repo[n], replacements
>   
> +class _multifold(fold):
> +    """fold subclass used for when multiple folds happen in a row
> +
> +    We only want to fire the editor for the folded message once when
> +    (say) four CLs are folded down into a single change. This is
> +    similar to rollup, but we should preserve both messages so that
> +    when the last fold operation runs we can show the user all the
> +    commit messages in their editor.
> +    """
> +    def skipprompt(self):
> +        return True
> +
>   class rollup(fold):
> +    def mergedescs(self):
> +        return False
> +
>       def skipprompt(self):
>           return True
>   
> @@ -644,6 +662,7 @@ actiontable = {'p': pick,
>                  'edit': edit,
>                  'f': fold,
>                  'fold': fold,
> +               '_multifold': _multifold,
>                  'r': rollup,
>                  'roll': rollup,
>                  'd': drop,
> @@ -850,6 +869,14 @@ 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
This is moving us a bit back towards the world where there are hard 
coded special cases all over the place.  Can we add a preprocess() 
function to each histeditaction class and put this logic inside the 
multifold class?  Then just call preprocess on every action class.
> +
>       while state.rules:
>           state.write()
>           action, ha = state.rules.pop(0)
> @@ -995,7 +1022,7 @@ def verifyrules(rules, repo, ctxs):
>               raise util.Abort(_('duplicated command for changeset %s') %
>                       ha[:12])
>           seen.add(ha)
> -        if action not in actiontable:
> +        if action not in actiontable or action.startswith('_'):
>               raise util.Abort(_('unknown action "%s"') % action)
>           parsed.append([action, ha])
>       missing = sorted(expected - seen)  # sort to stabilize output
> diff --git a/tests/test-histedit-fold.t b/tests/test-histedit-fold.t
> --- a/tests/test-histedit-fold.t
> +++ b/tests/test-histedit-fold.t
> @@ -509,4 +509,44 @@ into the hook command.
>     $ hg add amended.txt
>     $ hg ci -q --config extensions.largefiles= --amend -I amended.txt
>   
> +  $ echo foo >> foo
> +  $ hg add foo
> +  $ hg ci -m foo1
> +  $ echo foo >> foo
> +  $ hg ci -m foo2
> +  $ echo foo >> foo
> +  $ hg ci -m foo3
> +  $ hg logt
> +  4:21679ff7675c foo3
> +  3:b7389cc4d66e foo2
> +  2:0e01aeef5fa8 foo1
> +  1:578c7455730c a
> +  0:79b99e9c8e49 b
> +  $ cat > $TESTTMP/editor.sh <<EOF
> +  > echo merged foos > $$1
> +  > echo ran editor >> $TESTTMP/editorlog.txt
> +  > EOF
> +  $ HGEDITOR="sh $TESTTMP/editor.sh" hg histedit 1 --commands - 2>&1 <<EOF | fixbundle
> +  > pick 578c7455730c 1 a
> +  > pick 0e01aeef5fa8 2 foo1
> +  > fold b7389cc4d66e 3 foo2
> +  > fold 21679ff7675c 4 foo3
> +  > EOF
> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  reverting foo
> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  merging foo
> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ hg logt
> +  2:f7c59a6ef880 foo1
> +  1:578c7455730c a
> +  0:79b99e9c8e49 b
> +Editor should have run only once
> +  $ cat $TESTTMP/editorlog.txt
> +  ran editor
> +
>     $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Sept. 1, 2015, 5:17 p.m.
Yeah, I can do that. Sounds a little overengineered for the current
case, but I can see it being useful. I'll roll a v3 soon. Thanks!

On Tue, Sep 1, 2015 at 1:13 PM, Durham Goode <durham@fb.com> wrote:
>
>
> On 8/31/15 9:56 PM, Augie Fackler wrote:
>>
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1440701186 14400
>> #      Thu Aug 27 14:46:26 2015 -0400
>> # Node ID 6d4eb98524de846543455102dfe4aaa3c8c8af45
>> # Parent  049005de325ea400893f45bd6221215cc9b26db0
>> histedit: use one editor when multiple folds happen in a row (issue3524)
>> (BC)
>>
>> This was the first ever feature request for histedit, originally filed
>> back on April 4, 2009. Finally fixed.
>>
>> diff --git a/hgext/histedit.py b/hgext/histedit.py
>> --- a/hgext/histedit.py
>> +++ b/hgext/histedit.py
>> @@ -559,6 +559,9 @@ class fold(histeditaction):
>>       def skipprompt(self):
>>           return False
>>   +    def mergedescs(self):
>> +        return True
>> +
>>       def finishfold(self, ui, repo, ctx, oldctx, newnode,
>> internalchanges):
>>           parent = ctx.parents()[0].node()
>>           hg.update(repo, parent)
>> @@ -566,7 +569,7 @@ class fold(histeditaction):
>>           commitopts = {}
>>           commitopts['user'] = ctx.user()
>>           # commit message
>> -        if self.skipprompt():
>> +        if not self.mergedescs():
>>               newmessage = ctx.description()
>>           else:
>>               newmessage = '\n***\n'.join(
>> @@ -601,7 +604,22 @@ class fold(histeditaction):
>>               replacements.append((ich, (n,)))
>>           return repo[n], replacements
>>   +class _multifold(fold):
>> +    """fold subclass used for when multiple folds happen in a row
>> +
>> +    We only want to fire the editor for the folded message once when
>> +    (say) four CLs are folded down into a single change. This is
>> +    similar to rollup, but we should preserve both messages so that
>> +    when the last fold operation runs we can show the user all the
>> +    commit messages in their editor.
>> +    """
>> +    def skipprompt(self):
>> +        return True
>> +
>>   class rollup(fold):
>> +    def mergedescs(self):
>> +        return False
>> +
>>       def skipprompt(self):
>>           return True
>>   @@ -644,6 +662,7 @@ actiontable = {'p': pick,
>>                  'edit': edit,
>>                  'f': fold,
>>                  'fold': fold,
>> +               '_multifold': _multifold,
>>                  'r': rollup,
>>                  'roll': rollup,
>>                  'd': drop,
>> @@ -850,6 +869,14 @@ 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
>
> This is moving us a bit back towards the world where there are hard coded
> special cases all over the place.  Can we add a preprocess() function to
> each histeditaction class and put this logic inside the multifold class?
> Then just call preprocess on every action class.
>
>> +
>>       while state.rules:
>>           state.write()
>>           action, ha = state.rules.pop(0)
>> @@ -995,7 +1022,7 @@ def verifyrules(rules, repo, ctxs):
>>               raise util.Abort(_('duplicated command for changeset %s') %
>>                       ha[:12])
>>           seen.add(ha)
>> -        if action not in actiontable:
>> +        if action not in actiontable or action.startswith('_'):
>>               raise util.Abort(_('unknown action "%s"') % action)
>>           parsed.append([action, ha])
>>       missing = sorted(expected - seen)  # sort to stabilize output
>> diff --git a/tests/test-histedit-fold.t b/tests/test-histedit-fold.t
>> --- a/tests/test-histedit-fold.t
>> +++ b/tests/test-histedit-fold.t
>> @@ -509,4 +509,44 @@ into the hook command.
>>     $ hg add amended.txt
>>     $ hg ci -q --config extensions.largefiles= --amend -I amended.txt
>>   +  $ echo foo >> foo
>> +  $ hg add foo
>> +  $ hg ci -m foo1
>> +  $ echo foo >> foo
>> +  $ hg ci -m foo2
>> +  $ echo foo >> foo
>> +  $ hg ci -m foo3
>> +  $ hg logt
>> +  4:21679ff7675c foo3
>> +  3:b7389cc4d66e foo2
>> +  2:0e01aeef5fa8 foo1
>> +  1:578c7455730c a
>> +  0:79b99e9c8e49 b
>> +  $ cat > $TESTTMP/editor.sh <<EOF
>> +  > echo merged foos > $$1
>> +  > echo ran editor >> $TESTTMP/editorlog.txt
>> +  > EOF
>> +  $ HGEDITOR="sh $TESTTMP/editor.sh" hg histedit 1 --commands - 2>&1
>> <<EOF | fixbundle
>> +  > pick 578c7455730c 1 a
>> +  > pick 0e01aeef5fa8 2 foo1
>> +  > fold b7389cc4d66e 3 foo2
>> +  > fold 21679ff7675c 4 foo3
>> +  > EOF
>> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>> +  reverting foo
>> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
>> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
>> +  merging foo
>> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
>> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
>> +  $ hg logt
>> +  2:f7c59a6ef880 foo1
>> +  1:578c7455730c a
>> +  0:79b99e9c8e49 b
>> +Editor should have run only once
>> +  $ cat $TESTTMP/editorlog.txt
>> +  ran editor
>> +
>>     $ cd ..
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> https://selenic.com/mailman/listinfo/mercurial-devel
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Durham Goode - Sept. 1, 2015, 5:23 p.m.
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'

On 9/1/15 10:17 AM, Augie Fackler wrote:
> Yeah, I can do that. Sounds a little overengineered for the current
> case, but I can see it being useful. I'll roll a v3 soon. Thanks!
>
> On Tue, Sep 1, 2015 at 1:13 PM, Durham Goode <durham@fb.com> wrote:
>>
>> On 8/31/15 9:56 PM, Augie Fackler wrote:
>>> # HG changeset patch
>>> # User Augie Fackler <augie@google.com>
>>> # Date 1440701186 14400
>>> #      Thu Aug 27 14:46:26 2015 -0400
>>> # Node ID 6d4eb98524de846543455102dfe4aaa3c8c8af45
>>> # Parent  049005de325ea400893f45bd6221215cc9b26db0
>>> histedit: use one editor when multiple folds happen in a row (issue3524)
>>> (BC)
>>>
>>> This was the first ever feature request for histedit, originally filed
>>> back on April 4, 2009. Finally fixed.
>>>
>>> diff --git a/hgext/histedit.py b/hgext/histedit.py
>>> --- a/hgext/histedit.py
>>> +++ b/hgext/histedit.py
>>> @@ -559,6 +559,9 @@ class fold(histeditaction):
>>>        def skipprompt(self):
>>>            return False
>>>    +    def mergedescs(self):
>>> +        return True
>>> +
>>>        def finishfold(self, ui, repo, ctx, oldctx, newnode,
>>> internalchanges):
>>>            parent = ctx.parents()[0].node()
>>>            hg.update(repo, parent)
>>> @@ -566,7 +569,7 @@ class fold(histeditaction):
>>>            commitopts = {}
>>>            commitopts['user'] = ctx.user()
>>>            # commit message
>>> -        if self.skipprompt():
>>> +        if not self.mergedescs():
>>>                newmessage = ctx.description()
>>>            else:
>>>                newmessage = '\n***\n'.join(
>>> @@ -601,7 +604,22 @@ class fold(histeditaction):
>>>                replacements.append((ich, (n,)))
>>>            return repo[n], replacements
>>>    +class _multifold(fold):
>>> +    """fold subclass used for when multiple folds happen in a row
>>> +
>>> +    We only want to fire the editor for the folded message once when
>>> +    (say) four CLs are folded down into a single change. This is
>>> +    similar to rollup, but we should preserve both messages so that
>>> +    when the last fold operation runs we can show the user all the
>>> +    commit messages in their editor.
>>> +    """
>>> +    def skipprompt(self):
>>> +        return True
>>> +
>>>    class rollup(fold):
>>> +    def mergedescs(self):
>>> +        return False
>>> +
>>>        def skipprompt(self):
>>>            return True
>>>    @@ -644,6 +662,7 @@ actiontable = {'p': pick,
>>>                   'edit': edit,
>>>                   'f': fold,
>>>                   'fold': fold,
>>> +               '_multifold': _multifold,
>>>                   'r': rollup,
>>>                   'roll': rollup,
>>>                   'd': drop,
>>> @@ -850,6 +869,14 @@ 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
>> This is moving us a bit back towards the world where there are hard coded
>> special cases all over the place.  Can we add a preprocess() function to
>> each histeditaction class and put this logic inside the multifold class?
>> Then just call preprocess on every action class.
>>
>>> +
>>>        while state.rules:
>>>            state.write()
>>>            action, ha = state.rules.pop(0)
>>> @@ -995,7 +1022,7 @@ def verifyrules(rules, repo, ctxs):
>>>                raise util.Abort(_('duplicated command for changeset %s') %
>>>                        ha[:12])
>>>            seen.add(ha)
>>> -        if action not in actiontable:
>>> +        if action not in actiontable or action.startswith('_'):
>>>                raise util.Abort(_('unknown action "%s"') % action)
>>>            parsed.append([action, ha])
>>>        missing = sorted(expected - seen)  # sort to stabilize output
>>> diff --git a/tests/test-histedit-fold.t b/tests/test-histedit-fold.t
>>> --- a/tests/test-histedit-fold.t
>>> +++ b/tests/test-histedit-fold.t
>>> @@ -509,4 +509,44 @@ into the hook command.
>>>      $ hg add amended.txt
>>>      $ hg ci -q --config extensions.largefiles= --amend -I amended.txt
>>>    +  $ echo foo >> foo
>>> +  $ hg add foo
>>> +  $ hg ci -m foo1
>>> +  $ echo foo >> foo
>>> +  $ hg ci -m foo2
>>> +  $ echo foo >> foo
>>> +  $ hg ci -m foo3
>>> +  $ hg logt
>>> +  4:21679ff7675c foo3
>>> +  3:b7389cc4d66e foo2
>>> +  2:0e01aeef5fa8 foo1
>>> +  1:578c7455730c a
>>> +  0:79b99e9c8e49 b
>>> +  $ cat > $TESTTMP/editor.sh <<EOF
>>> +  > echo merged foos > $$1
>>> +  > echo ran editor >> $TESTTMP/editorlog.txt
>>> +  > EOF
>>> +  $ HGEDITOR="sh $TESTTMP/editor.sh" hg histedit 1 --commands - 2>&1
>>> <<EOF | fixbundle
>>> +  > pick 578c7455730c 1 a
>>> +  > pick 0e01aeef5fa8 2 foo1
>>> +  > fold b7389cc4d66e 3 foo2
>>> +  > fold 21679ff7675c 4 foo3
>>> +  > EOF
>>> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>> +  reverting foo
>>> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
>>> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>> +  merging foo
>>> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
>>> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>> +  $ hg logt
>>> +  2:f7c59a6ef880 foo1
>>> +  1:578c7455730c a
>>> +  0:79b99e9c8e49 b
>>> +Editor should have run only once
>>> +  $ cat $TESTTMP/editorlog.txt
>>> +  ran editor
>>> +
>>>      $ cd ..
>>> _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel@selenic.com
>>> https://selenic.com/mailman/listinfo/mercurial-devel
>>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> https://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Sept. 1, 2015, 7:59 p.m.
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.

>
>
> On 9/1/15 10:17 AM, Augie Fackler wrote:
>>
>> Yeah, I can do that. Sounds a little overengineered for the current
>> case, but I can see it being useful. I'll roll a v3 soon. Thanks!
>>
>> On Tue, Sep 1, 2015 at 1:13 PM, Durham Goode <durham@fb.com> wrote:
>>>
>>>
>>> On 8/31/15 9:56 PM, Augie Fackler wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Augie Fackler <augie@google.com>
>>>> # Date 1440701186 14400
>>>> #      Thu Aug 27 14:46:26 2015 -0400
>>>> # Node ID 6d4eb98524de846543455102dfe4aaa3c8c8af45
>>>> # Parent  049005de325ea400893f45bd6221215cc9b26db0
>>>> histedit: use one editor when multiple folds happen in a row (issue3524)
>>>> (BC)
>>>>
>>>> This was the first ever feature request for histedit, originally filed
>>>> back on April 4, 2009. Finally fixed.
>>>>
>>>> diff --git a/hgext/histedit.py b/hgext/histedit.py
>>>> --- a/hgext/histedit.py
>>>> +++ b/hgext/histedit.py
>>>> @@ -559,6 +559,9 @@ class fold(histeditaction):
>>>>        def skipprompt(self):
>>>>            return False
>>>>    +    def mergedescs(self):
>>>> +        return True
>>>> +
>>>>        def finishfold(self, ui, repo, ctx, oldctx, newnode,
>>>> internalchanges):
>>>>            parent = ctx.parents()[0].node()
>>>>            hg.update(repo, parent)
>>>> @@ -566,7 +569,7 @@ class fold(histeditaction):
>>>>            commitopts = {}
>>>>            commitopts['user'] = ctx.user()
>>>>            # commit message
>>>> -        if self.skipprompt():
>>>> +        if not self.mergedescs():
>>>>                newmessage = ctx.description()
>>>>            else:
>>>>                newmessage = '\n***\n'.join(
>>>> @@ -601,7 +604,22 @@ class fold(histeditaction):
>>>>                replacements.append((ich, (n,)))
>>>>            return repo[n], replacements
>>>>    +class _multifold(fold):
>>>> +    """fold subclass used for when multiple folds happen in a row
>>>> +
>>>> +    We only want to fire the editor for the folded message once when
>>>> +    (say) four CLs are folded down into a single change. This is
>>>> +    similar to rollup, but we should preserve both messages so that
>>>> +    when the last fold operation runs we can show the user all the
>>>> +    commit messages in their editor.
>>>> +    """
>>>> +    def skipprompt(self):
>>>> +        return True
>>>> +
>>>>    class rollup(fold):
>>>> +    def mergedescs(self):
>>>> +        return False
>>>> +
>>>>        def skipprompt(self):
>>>>            return True
>>>>    @@ -644,6 +662,7 @@ actiontable = {'p': pick,
>>>>                   'edit': edit,
>>>>                   'f': fold,
>>>>                   'fold': fold,
>>>> +               '_multifold': _multifold,
>>>>                   'r': rollup,
>>>>                   'roll': rollup,
>>>>                   'd': drop,
>>>> @@ -850,6 +869,14 @@ 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
>>>
>>> This is moving us a bit back towards the world where there are hard coded
>>> special cases all over the place.  Can we add a preprocess() function to
>>> each histeditaction class and put this logic inside the multifold class?
>>> Then just call preprocess on every action class.
>>>
>>>> +
>>>>        while state.rules:
>>>>            state.write()
>>>>            action, ha = state.rules.pop(0)
>>>> @@ -995,7 +1022,7 @@ def verifyrules(rules, repo, ctxs):
>>>>                raise util.Abort(_('duplicated command for changeset %s')
>>>> %
>>>>                        ha[:12])
>>>>            seen.add(ha)
>>>> -        if action not in actiontable:
>>>> +        if action not in actiontable or action.startswith('_'):
>>>>                raise util.Abort(_('unknown action "%s"') % action)
>>>>            parsed.append([action, ha])
>>>>        missing = sorted(expected - seen)  # sort to stabilize output
>>>> diff --git a/tests/test-histedit-fold.t b/tests/test-histedit-fold.t
>>>> --- a/tests/test-histedit-fold.t
>>>> +++ b/tests/test-histedit-fold.t
>>>> @@ -509,4 +509,44 @@ into the hook command.
>>>>      $ hg add amended.txt
>>>>      $ hg ci -q --config extensions.largefiles= --amend -I amended.txt
>>>>    +  $ echo foo >> foo
>>>> +  $ hg add foo
>>>> +  $ hg ci -m foo1
>>>> +  $ echo foo >> foo
>>>> +  $ hg ci -m foo2
>>>> +  $ echo foo >> foo
>>>> +  $ hg ci -m foo3
>>>> +  $ hg logt
>>>> +  4:21679ff7675c foo3
>>>> +  3:b7389cc4d66e foo2
>>>> +  2:0e01aeef5fa8 foo1
>>>> +  1:578c7455730c a
>>>> +  0:79b99e9c8e49 b
>>>> +  $ cat > $TESTTMP/editor.sh <<EOF
>>>> +  > echo merged foos > $$1
>>>> +  > echo ran editor >> $TESTTMP/editorlog.txt
>>>> +  > EOF
>>>> +  $ HGEDITOR="sh $TESTTMP/editor.sh" hg histedit 1 --commands - 2>&1
>>>> <<EOF | fixbundle
>>>> +  > pick 578c7455730c 1 a
>>>> +  > pick 0e01aeef5fa8 2 foo1
>>>> +  > fold b7389cc4d66e 3 foo2
>>>> +  > fold 21679ff7675c 4 foo3
>>>> +  > EOF
>>>> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>>> +  reverting foo
>>>> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
>>>> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>>> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>>> +  merging foo
>>>> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
>>>> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>>> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>>> +  $ hg logt
>>>> +  2:f7c59a6ef880 foo1
>>>> +  1:578c7455730c a
>>>> +  0:79b99e9c8e49 b
>>>> +Editor should have run only once
>>>> +  $ cat $TESTTMP/editorlog.txt
>>>> +  ran editor
>>>> +
>>>>      $ cd ..
>>>> _______________________________________________
>>>> Mercurial-devel mailing list
>>>> Mercurial-devel@selenic.com
>>>> https://selenic.com/mailman/listinfo/mercurial-devel
>>>
>>>
>>> _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel@selenic.com
>>> https://selenic.com/mailman/listinfo/mercurial-devel
>
>
Durham Goode - Sept. 1, 2015, 10:16 p.m.
On 9/1/15, 12: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.

Fair enough. I'm fine doing it later then.  As long as we keep a close eye
on the hardcoding creep.

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -559,6 +559,9 @@  class fold(histeditaction):
     def skipprompt(self):
         return False
 
+    def mergedescs(self):
+        return True
+
     def finishfold(self, ui, repo, ctx, oldctx, newnode, internalchanges):
         parent = ctx.parents()[0].node()
         hg.update(repo, parent)
@@ -566,7 +569,7 @@  class fold(histeditaction):
         commitopts = {}
         commitopts['user'] = ctx.user()
         # commit message
-        if self.skipprompt():
+        if not self.mergedescs():
             newmessage = ctx.description()
         else:
             newmessage = '\n***\n'.join(
@@ -601,7 +604,22 @@  class fold(histeditaction):
             replacements.append((ich, (n,)))
         return repo[n], replacements
 
+class _multifold(fold):
+    """fold subclass used for when multiple folds happen in a row
+
+    We only want to fire the editor for the folded message once when
+    (say) four CLs are folded down into a single change. This is
+    similar to rollup, but we should preserve both messages so that
+    when the last fold operation runs we can show the user all the
+    commit messages in their editor.
+    """
+    def skipprompt(self):
+        return True
+
 class rollup(fold):
+    def mergedescs(self):
+        return False
+
     def skipprompt(self):
         return True
 
@@ -644,6 +662,7 @@  actiontable = {'p': pick,
                'edit': edit,
                'f': fold,
                'fold': fold,
+               '_multifold': _multifold,
                'r': rollup,
                'roll': rollup,
                'd': drop,
@@ -850,6 +869,14 @@  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
+
     while state.rules:
         state.write()
         action, ha = state.rules.pop(0)
@@ -995,7 +1022,7 @@  def verifyrules(rules, repo, ctxs):
             raise util.Abort(_('duplicated command for changeset %s') %
                     ha[:12])
         seen.add(ha)
-        if action not in actiontable:
+        if action not in actiontable or action.startswith('_'):
             raise util.Abort(_('unknown action "%s"') % action)
         parsed.append([action, ha])
     missing = sorted(expected - seen)  # sort to stabilize output
diff --git a/tests/test-histedit-fold.t b/tests/test-histedit-fold.t
--- a/tests/test-histedit-fold.t
+++ b/tests/test-histedit-fold.t
@@ -509,4 +509,44 @@  into the hook command.
   $ hg add amended.txt
   $ hg ci -q --config extensions.largefiles= --amend -I amended.txt
 
+  $ echo foo >> foo
+  $ hg add foo
+  $ hg ci -m foo1
+  $ echo foo >> foo
+  $ hg ci -m foo2
+  $ echo foo >> foo
+  $ hg ci -m foo3
+  $ hg logt
+  4:21679ff7675c foo3
+  3:b7389cc4d66e foo2
+  2:0e01aeef5fa8 foo1
+  1:578c7455730c a
+  0:79b99e9c8e49 b
+  $ cat > $TESTTMP/editor.sh <<EOF
+  > echo merged foos > $$1
+  > echo ran editor >> $TESTTMP/editorlog.txt
+  > EOF
+  $ HGEDITOR="sh $TESTTMP/editor.sh" hg histedit 1 --commands - 2>&1 <<EOF | fixbundle
+  > pick 578c7455730c 1 a
+  > pick 0e01aeef5fa8 2 foo1
+  > fold b7389cc4d66e 3 foo2
+  > fold 21679ff7675c 4 foo3
+  > EOF
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  reverting foo
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  merging foo
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg logt
+  2:f7c59a6ef880 foo1
+  1:578c7455730c a
+  0:79b99e9c8e49 b
+Editor should have run only once
+  $ cat $TESTTMP/editorlog.txt
+  ran editor
+
   $ cd ..