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

[Pierre-Yves David]

On 09/08/2015 10:28 AM, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie at 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.

-- 
Pierre-Yves David