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

# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1440701186 14400
#      Thu Aug 27 14:46:26 2015 -0400
# Node ID 23d0ea9adbdb41dd989e08b02e09500a2445809b
# Parent  7187f6e923d55cf6b7e6910d24645f303db671ee
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.

In the future we'll probably want to make it possible for other
preprocessing steps to be added to the list, but for now we're
skipping that because it's unclear what the API should look like
without a proposed consumer.

On 09/08/2015 10:28 AM, 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 23d0ea9adbdb41dd989e08b02e09500a2445809b
> # Parent  7187f6e923d55cf6b7e6910d24645f303db671ee
> 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.
>
> In the future we'll probably want to make it possible for other
> preprocessing steps to be added to the list, but for now we're
> skipping that because it's unclear what the API should look like
> without a proposed consumer.
>
> 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):

This is probably worth documenting for the next poor soul getting lost 
in this codebase.

As far as I understand, we now have

- skippromt: do we pop the editor
- mergedescs: do we append the commit message with the other one.

> +        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

"Four CLs"?  is this Google internal jargon creeping in?

> +    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
>

Your probably want to introduce a "header comment" here to mark the 
begining of your specific test.


> +  $ 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

Can we also check what the editor got, and the final commit message 
content. I'm curious about how it looks like.

Core logic seems otherwise good to me.