Patchwork [1,of,3] rebase: use one dirstateguard for entire rebase

login
register
mail settings
Submitter Durham Goode
Date March 19, 2017, 7 p.m.
Message ID <c2f68e8cd96f92c2fa24.1489950056@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/19464/
State Accepted
Headers show

Comments

Durham Goode - March 19, 2017, 7 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1489949655 25200
#      Sun Mar 19 11:54:15 2017 -0700
# Node ID c2f68e8cd96f92c2fa240fe04e541c14c1bad3d8
# Parent  75e4bae56068bec4f965773a2d5f4d272a8dc5b0
rebase: use one dirstateguard for entire rebase

Recently we switched rebases to run the entire rebase inside a single
transaction, which dramatically improved the speed of rebases in repos with
large working copies. Let's also move the dirstate into a single dirstateguard
to get the same benefits. This let's us avoid serializing the dirstate after
each commit.

In a large repo, rebasing 27 commits is sped up by about 20%.

I believe the test changes are because us touching the dirstate gave the
transaction something to actually rollback.
Yuya Nishihara - March 20, 2017, 6:35 a.m.
On Sun, 19 Mar 2017 12:00:56 -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1489949655 25200
> #      Sun Mar 19 11:54:15 2017 -0700
> # Node ID c2f68e8cd96f92c2fa240fe04e541c14c1bad3d8
> # Parent  75e4bae56068bec4f965773a2d5f4d272a8dc5b0
> rebase: use one dirstateguard for entire rebase
> 
> Recently we switched rebases to run the entire rebase inside a single
> transaction, which dramatically improved the speed of rebases in repos with
> large working copies. Let's also move the dirstate into a single dirstateguard
> to get the same benefits. This let's us avoid serializing the dirstate after
> each commit.

This seems good for consistency. Queued, thanks.

> +            try:
> +                newnode = concludenode(repo, revtoreuse, p1, self.external,
> +                                       commitmsg=commitmsg,
> +                                       extrafn=_makeextrafn(self.extrafns),
> +                                       editor=editor,
> +                                       keepbranches=self.keepbranchesf,
> +                                       date=self.date)
> +                dsguard.close()
> +                release(dsguard)
> +            except error.InterventionRequired:
> +                dsguard.close()
> +                release(dsguard)
> +                raise
> +            except Exception:
> +                release(dsguard)
> +                raise

could be "finally: release(dsguard)".

> +            dsguard = dirstateguard.dirstateguard(repo, 'rebase')
>              try:
>                  rbsrt._performrebase(tr)
> +                dsguard.close()
> +                release(dsguard)
>              except error.InterventionRequired:
> +                dsguard.close()
> +                release(dsguard)
>                  tr.close()
>                  raise
> +            except Exception:
> +                release(dsguard)
> +                raise

this, too.
Katsunori FUJIWARA - March 20, 2017, 11:14 a.m.
(Cc-ed to yuya, for confirmation)

At Sun, 19 Mar 2017 12:00:56 -0700,
Durham Goode wrote:
> 
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1489949655 25200
> #      Sun Mar 19 11:54:15 2017 -0700
> # Node ID c2f68e8cd96f92c2fa240fe04e541c14c1bad3d8
> # Parent  75e4bae56068bec4f965773a2d5f4d272a8dc5b0
> rebase: use one dirstateguard for entire rebase
> 
> Recently we switched rebases to run the entire rebase inside a single
> transaction, which dramatically improved the speed of rebases in repos with
> large working copies. Let's also move the dirstate into a single dirstateguard
> to get the same benefits. This let's us avoid serializing the dirstate after
> each commit.
> 
> In a large repo, rebasing 27 commits is sped up by about 20%.
> 
> I believe the test changes are because us touching the dirstate gave the
> transaction something to actually rollback.
> 
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -475,12 +475,24 @@ class rebaseruntime(object):
>                  editopt = True
>              editor = cmdutil.getcommiteditor(edit=editopt, editform=editform)
>              revtoreuse = max(self.state)
> -            newnode = concludenode(repo, revtoreuse, p1, self.external,
> -                                   commitmsg=commitmsg,
> -                                   extrafn=_makeextrafn(self.extrafns),
> -                                   editor=editor,
> -                                   keepbranches=self.keepbranchesf,
> -                                   date=self.date)
> +            dsguard = dirstateguard.dirstateguard(repo, 'rebase')
> +            try:
> +                newnode = concludenode(repo, revtoreuse, p1, self.external,
> +                                       commitmsg=commitmsg,
> +                                       extrafn=_makeextrafn(self.extrafns),
> +                                       editor=editor,
> +                                       keepbranches=self.keepbranchesf,
> +                                       date=self.date)
> +                dsguard.close()
> +                release(dsguard)
> +            except error.InterventionRequired:
> +                dsguard.close()
> +                release(dsguard)
> +                raise
> +            except Exception:
> +                release(dsguard)
> +                raise
> +
>              if newnode is None:
>                  newrev = self.target
>              else:

This hunk just moves dirstateguard creation from concludenode() to
caller side.

Therefore, creation of dirstateguard at each concludenode() while
rebasing is not "one dirstateguard for entire rebase" but still
"dirstateguard per rebased-revision".

In addition to it, dirstateguard in this hunk should be redundant,
because we already know that another dirstategurad is certainly
created in outer scope (but in same transaction) as below hunk.

IMHO, only two hunks below are enough for "one dirstateguard for
entire rebase".


> @@ -712,11 +724,19 @@ def rebase(ui, repo, **opts):
>                  return retcode
>  
>          with repo.transaction('rebase') as tr:
> +            dsguard = dirstateguard.dirstateguard(repo, 'rebase')
>              try:
>                  rbsrt._performrebase(tr)
> +                dsguard.close()
> +                release(dsguard)
>              except error.InterventionRequired:
> +                dsguard.close()
> +                release(dsguard)
>                  tr.close()
>                  raise
> +            except Exception:
> +                release(dsguard)
> +                raise
>          rbsrt._finishrebase()
>      finally:
>          release(lock, wlock)
> @@ -840,33 +860,28 @@ def concludenode(repo, rev, p1, p2, comm
>      '''Commit the wd changes with parents p1 and p2. Reuse commit info from rev
>      but also store useful information in extra.
>      Return node of committed revision.'''
> -    dsguard = dirstateguard.dirstateguard(repo, 'rebase')
> -    try:
> -        repo.setparents(repo[p1].node(), repo[p2].node())
> -        ctx = repo[rev]
> -        if commitmsg is None:
> -            commitmsg = ctx.description()
> -        keepbranch = keepbranches and repo[p1].branch() != ctx.branch()
> -        extra = {'rebase_source': ctx.hex()}
> -        if extrafn:
> -            extrafn(ctx, extra)
> +    repo.setparents(repo[p1].node(), repo[p2].node())
> +    ctx = repo[rev]
> +    if commitmsg is None:
> +        commitmsg = ctx.description()
> +    keepbranch = keepbranches and repo[p1].branch() != ctx.branch()
> +    extra = {'rebase_source': ctx.hex()}
> +    if extrafn:
> +        extrafn(ctx, extra)
>  
> -        targetphase = max(ctx.phase(), phases.draft)
> -        overrides = {('phases', 'new-commit'): targetphase}
> -        with repo.ui.configoverride(overrides, 'rebase'):
> -            if keepbranch:
> -                repo.ui.setconfig('ui', 'allowemptycommit', True)
> -            # Commit might fail if unresolved files exist
> -            if date is None:
> -                date = ctx.date()
> -            newnode = repo.commit(text=commitmsg, user=ctx.user(),
> -                                  date=date, extra=extra, editor=editor)
> +    targetphase = max(ctx.phase(), phases.draft)
> +    overrides = {('phases', 'new-commit'): targetphase}
> +    with repo.ui.configoverride(overrides, 'rebase'):
> +        if keepbranch:
> +            repo.ui.setconfig('ui', 'allowemptycommit', True)
> +        # Commit might fail if unresolved files exist
> +        if date is None:
> +            date = ctx.date()
> +        newnode = repo.commit(text=commitmsg, user=ctx.user(),
> +                              date=date, extra=extra, editor=editor)
>  
> -        repo.dirstate.setbranch(repo[newnode].branch())
> -        dsguard.close()
> -        return newnode
> -    finally:
> -        release(dsguard)
> +    repo.dirstate.setbranch(repo[newnode].branch())
> +    return newnode
>  
>  def rebasenode(repo, rev, p1, base, state, collapse, target):
>      'Rebase a single revision rev on top of p1 using base as merge ancestor'
> diff --git a/tests/test-rebase-collapse.t b/tests/test-rebase-collapse.t
> --- a/tests/test-rebase-collapse.t
> +++ b/tests/test-rebase-collapse.t
> @@ -572,6 +572,8 @@ Interactions between collapse and keepbr
>    o  0: 'A'
>    
>    $ hg rebase --keepbranches --collapse -s 1 -d 3
> +  transaction abort!
> +  rollback completed
>    abort: cannot collapse multiple named branches
>    [255]
>  
> diff --git a/tests/test-rebase-scenario-global.t b/tests/test-rebase-scenario-global.t
> --- a/tests/test-rebase-scenario-global.t
> +++ b/tests/test-rebase-scenario-global.t
> @@ -270,6 +270,8 @@ G onto B - merge revision with both pare
>  
>    $ hg rebase -s 6 -d 1
>    rebasing 6:eea13746799a "G"
> +  transaction abort!
> +  rollback completed
>    abort: cannot use revision 6 as base, result would have 3 parents
>    [255]
>    $ hg rebase --abort
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - March 20, 2017, 1:17 p.m.
On Mon, 20 Mar 2017 20:14:20 +0900, FUJIWARA Katsunori wrote:
> (Cc-ed to yuya, for confirmation)
> 
> At Sun, 19 Mar 2017 12:00:56 -0700,
> Durham Goode wrote:
> > 
> > # HG changeset patch
> > # User Durham Goode <durham@fb.com>
> > # Date 1489949655 25200
> > #      Sun Mar 19 11:54:15 2017 -0700
> > # Node ID c2f68e8cd96f92c2fa240fe04e541c14c1bad3d8
> > # Parent  75e4bae56068bec4f965773a2d5f4d272a8dc5b0
> > rebase: use one dirstateguard for entire rebase
> > 
> > Recently we switched rebases to run the entire rebase inside a single
> > transaction, which dramatically improved the speed of rebases in repos with
> > large working copies. Let's also move the dirstate into a single dirstateguard
> > to get the same benefits. This let's us avoid serializing the dirstate after
> > each commit.
> > 
> > In a large repo, rebasing 27 commits is sped up by about 20%.
> > 
> > I believe the test changes are because us touching the dirstate gave the
> > transaction something to actually rollback.
> > 
> > diff --git a/hgext/rebase.py b/hgext/rebase.py
> > --- a/hgext/rebase.py
> > +++ b/hgext/rebase.py
> > @@ -475,12 +475,24 @@ class rebaseruntime(object):
> >                  editopt = True
> >              editor = cmdutil.getcommiteditor(edit=editopt, editform=editform)
> >              revtoreuse = max(self.state)
> > -            newnode = concludenode(repo, revtoreuse, p1, self.external,
> > -                                   commitmsg=commitmsg,
> > -                                   extrafn=_makeextrafn(self.extrafns),
> > -                                   editor=editor,
> > -                                   keepbranches=self.keepbranchesf,
> > -                                   date=self.date)
> > +            dsguard = dirstateguard.dirstateguard(repo, 'rebase')
> > +            try:
> > +                newnode = concludenode(repo, revtoreuse, p1, self.external,
> > +                                       commitmsg=commitmsg,
> > +                                       extrafn=_makeextrafn(self.extrafns),
> > +                                       editor=editor,
> > +                                       keepbranches=self.keepbranchesf,
> > +                                       date=self.date)
> > +                dsguard.close()
> > +                release(dsguard)
> > +            except error.InterventionRequired:
> > +                dsguard.close()
> > +                release(dsguard)
> > +                raise
> > +            except Exception:
> > +                release(dsguard)
> > +                raise
> > +
> >              if newnode is None:
> >                  newrev = self.target
> >              else:
> 
> This hunk just moves dirstateguard creation from concludenode() to
> caller side.
> 
> Therefore, creation of dirstateguard at each concludenode() while
> rebasing is not "one dirstateguard for entire rebase" but still
> "dirstateguard per rebased-revision".
> 
> In addition to it, dirstateguard in this hunk should be redundant,
> because we already know that another dirstategurad is certainly
> created in outer scope (but in same transaction) as below hunk.

This is actually _two_ dirstateguards for entire rebase. One for
_performrebase and the other for _finishrebase. I haven't investigated if
we can move _finishrebase into the transaction.
Katsunori FUJIWARA - March 20, 2017, 2:14 p.m.
At Mon, 20 Mar 2017 22:17:54 +0900,
Yuya Nishihara wrote:
> 
> On Mon, 20 Mar 2017 20:14:20 +0900, FUJIWARA Katsunori wrote:
> > (Cc-ed to yuya, for confirmation)
> > 
> > At Sun, 19 Mar 2017 12:00:56 -0700,
> > Durham Goode wrote:
> > > 
> > > # HG changeset patch
> > > # User Durham Goode <durham@fb.com>
> > > # Date 1489949655 25200
> > > #      Sun Mar 19 11:54:15 2017 -0700
> > > # Node ID c2f68e8cd96f92c2fa240fe04e541c14c1bad3d8
> > > # Parent  75e4bae56068bec4f965773a2d5f4d272a8dc5b0
> > > rebase: use one dirstateguard for entire rebase
> > > 
> > > Recently we switched rebases to run the entire rebase inside a single
> > > transaction, which dramatically improved the speed of rebases in repos with
> > > large working copies. Let's also move the dirstate into a single dirstateguard
> > > to get the same benefits. This let's us avoid serializing the dirstate after
> > > each commit.
> > > 
> > > In a large repo, rebasing 27 commits is sped up by about 20%.
> > > 
> > > I believe the test changes are because us touching the dirstate gave the
> > > transaction something to actually rollback.
> > > 
> > > diff --git a/hgext/rebase.py b/hgext/rebase.py
> > > --- a/hgext/rebase.py
> > > +++ b/hgext/rebase.py
> > > @@ -475,12 +475,24 @@ class rebaseruntime(object):
> > >                  editopt = True
> > >              editor = cmdutil.getcommiteditor(edit=editopt, editform=editform)
> > >              revtoreuse = max(self.state)
> > > -            newnode = concludenode(repo, revtoreuse, p1, self.external,
> > > -                                   commitmsg=commitmsg,
> > > -                                   extrafn=_makeextrafn(self.extrafns),
> > > -                                   editor=editor,
> > > -                                   keepbranches=self.keepbranchesf,
> > > -                                   date=self.date)
> > > +            dsguard = dirstateguard.dirstateguard(repo, 'rebase')
> > > +            try:
> > > +                newnode = concludenode(repo, revtoreuse, p1, self.external,
> > > +                                       commitmsg=commitmsg,
> > > +                                       extrafn=_makeextrafn(self.extrafns),
> > > +                                       editor=editor,
> > > +                                       keepbranches=self.keepbranchesf,
> > > +                                       date=self.date)
> > > +                dsguard.close()
> > > +                release(dsguard)
> > > +            except error.InterventionRequired:
> > > +                dsguard.close()
> > > +                release(dsguard)
> > > +                raise
> > > +            except Exception:
> > > +                release(dsguard)
> > > +                raise
> > > +
> > >              if newnode is None:
> > >                  newrev = self.target
> > >              else:
> > 
> > This hunk just moves dirstateguard creation from concludenode() to
> > caller side.
> > 
> > Therefore, creation of dirstateguard at each concludenode() while
> > rebasing is not "one dirstateguard for entire rebase" but still
> > "dirstateguard per rebased-revision".
> > 
> > In addition to it, dirstateguard in this hunk should be redundant,
> > because we already know that another dirstategurad is certainly
> > created in outer scope (but in same transaction) as below hunk.
> 
> This is actually _two_ dirstateguards for entire rebase. One for
> _performrebase and the other for _finishrebase. I haven't investigated if
> we can move _finishrebase into the transaction.

Oops, sorry I misunderstood the context. The 1st hunk is applied not
for _performrebase() but for _finishrebase(). It seems OK. Sorry,
again.
Durham Goode - March 21, 2017, 12:21 a.m.
On 3/20/17 6:17 AM, Yuya Nishihara wrote:
> On Mon, 20 Mar 2017 20:14:20 +0900, FUJIWARA Katsunori wrote:
>> (Cc-ed to yuya, for confirmation)
>>
>> At Sun, 19 Mar 2017 12:00:56 -0700,
>> Durham Goode wrote:
>>>
>>> # HG changeset patch
>>> # User Durham Goode <durham@fb.com>
>>> # Date 1489949655 25200
>>> #      Sun Mar 19 11:54:15 2017 -0700
>>> # Node ID c2f68e8cd96f92c2fa240fe04e541c14c1bad3d8
>>> # Parent  75e4bae56068bec4f965773a2d5f4d272a8dc5b0
>>> rebase: use one dirstateguard for entire rebase
>>>
>>> Recently we switched rebases to run the entire rebase inside a single
>>> transaction, which dramatically improved the speed of rebases in repos with
>>> large working copies. Let's also move the dirstate into a single dirstateguard
>>> to get the same benefits. This let's us avoid serializing the dirstate after
>>> each commit.
>>>
>>> In a large repo, rebasing 27 commits is sped up by about 20%.
>>>
>>> I believe the test changes are because us touching the dirstate gave the
>>> transaction something to actually rollback.
>>>
>>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>>> --- a/hgext/rebase.py
>>> +++ b/hgext/rebase.py
>>> @@ -475,12 +475,24 @@ class rebaseruntime(object):
>>>                  editopt = True
>>>              editor = cmdutil.getcommiteditor(edit=editopt, editform=editform)
>>>              revtoreuse = max(self.state)
>>> -            newnode = concludenode(repo, revtoreuse, p1, self.external,
>>> -                                   commitmsg=commitmsg,
>>> -                                   extrafn=_makeextrafn(self.extrafns),
>>> -                                   editor=editor,
>>> -                                   keepbranches=self.keepbranchesf,
>>> -                                   date=self.date)
>>> +            dsguard = dirstateguard.dirstateguard(repo, 'rebase')
>>> +            try:
>>> +                newnode = concludenode(repo, revtoreuse, p1, self.external,
>>> +                                       commitmsg=commitmsg,
>>> +                                       extrafn=_makeextrafn(self.extrafns),
>>> +                                       editor=editor,
>>> +                                       keepbranches=self.keepbranchesf,
>>> +                                       date=self.date)
>>> +                dsguard.close()
>>> +                release(dsguard)
>>> +            except error.InterventionRequired:
>>> +                dsguard.close()
>>> +                release(dsguard)
>>> +                raise
>>> +            except Exception:
>>> +                release(dsguard)
>>> +                raise
>>> +
>>>              if newnode is None:
>>>                  newrev = self.target
>>>              else:
>>
>> This hunk just moves dirstateguard creation from concludenode() to
>> caller side.
>>
>> Therefore, creation of dirstateguard at each concludenode() while
>> rebasing is not "one dirstateguard for entire rebase" but still
>> "dirstateguard per rebased-revision".
>>
>> In addition to it, dirstateguard in this hunk should be redundant,
>> because we already know that another dirstategurad is certainly
>> created in outer scope (but in same transaction) as below hunk.
>
> This is actually _two_ dirstateguards for entire rebase. One for
> _performrebase and the other for _finishrebase. I haven't investigated if
> we can move _finishrebase into the transaction.

We can probably move _finishrebase into the transaction in a future 
patch. I'm careful to change rebase incrementally, since each 
transaction change feels risky.

I may just move dirstate into a transaction at some point, since dsguard 
doesn't make sense now that we have better transaction support.

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -475,12 +475,24 @@  class rebaseruntime(object):
                 editopt = True
             editor = cmdutil.getcommiteditor(edit=editopt, editform=editform)
             revtoreuse = max(self.state)
-            newnode = concludenode(repo, revtoreuse, p1, self.external,
-                                   commitmsg=commitmsg,
-                                   extrafn=_makeextrafn(self.extrafns),
-                                   editor=editor,
-                                   keepbranches=self.keepbranchesf,
-                                   date=self.date)
+            dsguard = dirstateguard.dirstateguard(repo, 'rebase')
+            try:
+                newnode = concludenode(repo, revtoreuse, p1, self.external,
+                                       commitmsg=commitmsg,
+                                       extrafn=_makeextrafn(self.extrafns),
+                                       editor=editor,
+                                       keepbranches=self.keepbranchesf,
+                                       date=self.date)
+                dsguard.close()
+                release(dsguard)
+            except error.InterventionRequired:
+                dsguard.close()
+                release(dsguard)
+                raise
+            except Exception:
+                release(dsguard)
+                raise
+
             if newnode is None:
                 newrev = self.target
             else:
@@ -712,11 +724,19 @@  def rebase(ui, repo, **opts):
                 return retcode
 
         with repo.transaction('rebase') as tr:
+            dsguard = dirstateguard.dirstateguard(repo, 'rebase')
             try:
                 rbsrt._performrebase(tr)
+                dsguard.close()
+                release(dsguard)
             except error.InterventionRequired:
+                dsguard.close()
+                release(dsguard)
                 tr.close()
                 raise
+            except Exception:
+                release(dsguard)
+                raise
         rbsrt._finishrebase()
     finally:
         release(lock, wlock)
@@ -840,33 +860,28 @@  def concludenode(repo, rev, p1, p2, comm
     '''Commit the wd changes with parents p1 and p2. Reuse commit info from rev
     but also store useful information in extra.
     Return node of committed revision.'''
-    dsguard = dirstateguard.dirstateguard(repo, 'rebase')
-    try:
-        repo.setparents(repo[p1].node(), repo[p2].node())
-        ctx = repo[rev]
-        if commitmsg is None:
-            commitmsg = ctx.description()
-        keepbranch = keepbranches and repo[p1].branch() != ctx.branch()
-        extra = {'rebase_source': ctx.hex()}
-        if extrafn:
-            extrafn(ctx, extra)
+    repo.setparents(repo[p1].node(), repo[p2].node())
+    ctx = repo[rev]
+    if commitmsg is None:
+        commitmsg = ctx.description()
+    keepbranch = keepbranches and repo[p1].branch() != ctx.branch()
+    extra = {'rebase_source': ctx.hex()}
+    if extrafn:
+        extrafn(ctx, extra)
 
-        targetphase = max(ctx.phase(), phases.draft)
-        overrides = {('phases', 'new-commit'): targetphase}
-        with repo.ui.configoverride(overrides, 'rebase'):
-            if keepbranch:
-                repo.ui.setconfig('ui', 'allowemptycommit', True)
-            # Commit might fail if unresolved files exist
-            if date is None:
-                date = ctx.date()
-            newnode = repo.commit(text=commitmsg, user=ctx.user(),
-                                  date=date, extra=extra, editor=editor)
+    targetphase = max(ctx.phase(), phases.draft)
+    overrides = {('phases', 'new-commit'): targetphase}
+    with repo.ui.configoverride(overrides, 'rebase'):
+        if keepbranch:
+            repo.ui.setconfig('ui', 'allowemptycommit', True)
+        # Commit might fail if unresolved files exist
+        if date is None:
+            date = ctx.date()
+        newnode = repo.commit(text=commitmsg, user=ctx.user(),
+                              date=date, extra=extra, editor=editor)
 
-        repo.dirstate.setbranch(repo[newnode].branch())
-        dsguard.close()
-        return newnode
-    finally:
-        release(dsguard)
+    repo.dirstate.setbranch(repo[newnode].branch())
+    return newnode
 
 def rebasenode(repo, rev, p1, base, state, collapse, target):
     'Rebase a single revision rev on top of p1 using base as merge ancestor'
diff --git a/tests/test-rebase-collapse.t b/tests/test-rebase-collapse.t
--- a/tests/test-rebase-collapse.t
+++ b/tests/test-rebase-collapse.t
@@ -572,6 +572,8 @@  Interactions between collapse and keepbr
   o  0: 'A'
   
   $ hg rebase --keepbranches --collapse -s 1 -d 3
+  transaction abort!
+  rollback completed
   abort: cannot collapse multiple named branches
   [255]
 
diff --git a/tests/test-rebase-scenario-global.t b/tests/test-rebase-scenario-global.t
--- a/tests/test-rebase-scenario-global.t
+++ b/tests/test-rebase-scenario-global.t
@@ -270,6 +270,8 @@  G onto B - merge revision with both pare
 
   $ hg rebase -s 6 -d 1
   rebasing 6:eea13746799a "G"
+  transaction abort!
+  rollback completed
   abort: cannot use revision 6 as base, result would have 3 parents
   [255]
   $ hg rebase --abort