Patchwork rebase: add flag to commit to insert commit in stack

login
register
mail settings
Submitter Christian Delahousse
Date Nov. 25, 2015, 8:04 p.m.
Message ID <e08ef9c3e1409b9864e1.1448481850@dev4253.prn1.facebook.com>
Download mbox | patch
Permalink /patch/11663/
State Rejected
Headers show

Comments

Christian Delahousse - Nov. 25, 2015, 8:04 p.m.
# HG changeset patch
# User Laurent Charignon <lcharignon@fb.com>
# Date 1448481218 28800
#      Wed Nov 25 11:53:38 2015 -0800
# Node ID e08ef9c3e1409b9864e14f5b66ddd3821b4d6115
# Parent  397baacd694d0f8343888b64c200bfa926cdd893
rebase: add flag to commit to insert commit in stack

This patch adds a flag to insert a commit within an existing stack without
forcing the user to do a separate rebase step. From the call to commit --insert,
the tool flag is passed into our call to rebase in case the user specifies it.

A new test file is created because as there was no existing suitable place to
test the functionality.
Durham Goode - Nov. 25, 2015, 8:12 p.m.
On 11/25/15 3:04 PM, cdelahousse@fb.com wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon@fb.com>
> # Date 1448481218 28800
> #      Wed Nov 25 11:53:38 2015 -0800
> # Node ID e08ef9c3e1409b9864e14f5b66ddd3821b4d6115
> # Parent  397baacd694d0f8343888b64c200bfa926cdd893
> rebase: add flag to commit to insert commit in stack
>
> This patch adds a flag to insert a commit within an existing stack without
> forcing the user to do a separate rebase step. From the call to commit --insert,
> the tool flag is passed into our call to rebase in case the user specifies it.
>
> A new test file is created because as there was no existing suitable place to
> test the functionality.
>
The test file appears to be missing?

If there are conflicts is it obvious to the user that they need to run 
hg rebase --continue?

Also, what happens if I do 'hg commit --amend --insert'?

If we're going to allow this kind of flag, it opens the doors for a lot 
of other interesting flags (hg commit --amend --rebase ; for amending 
with a rebase. hg commit --amend --into X ; for amending pending changes 
into a commit further down the stack).  Do you see the '--insert' 
grammar making sense anywhere else (hg rebase -r X -d Y --insert ; to 
insert X between Y and it's desendants)?  Just seems like we'd want to 
strive for a consistent ux around these operations, and if we get it 
right it could be very powerful.
Augie Fackler - Nov. 25, 2015, 10:35 p.m.
> On Nov 25, 2015, at 3:12 PM, Durham Goode <durham@fb.com> wrote:
> 
> On 11/25/15 3:04 PM, cdelahousse@fb.com wrote:
>> # HG changeset patch
>> # User Laurent Charignon <lcharignon@fb.com>
>> # Date 1448481218 28800
>> #      Wed Nov 25 11:53:38 2015 -0800
>> # Node ID e08ef9c3e1409b9864e14f5b66ddd3821b4d6115
>> # Parent  397baacd694d0f8343888b64c200bfa926cdd893
>> rebase: add flag to commit to insert commit in stack
>> 
>> This patch adds a flag to insert a commit within an existing stack without
>> forcing the user to do a separate rebase step. From the call to commit --insert,
>> the tool flag is passed into our call to rebase in case the user specifies it.
>> 
>> A new test file is created because as there was no existing suitable place to
>> test the functionality.
>> 
> The test file appears to be missing?
> 
> If there are conflicts is it obvious to the user that they need to run hg rebase --continue?
> 
> Also, what happens if I do 'hg commit --amend --insert'?
> 
> If we're going to allow this kind of flag, it opens the doors for a lot of other interesting flags (hg commit --amend --rebase ; for amending with a rebase. hg commit --amend --into X ; for amending pending changes into a commit further down the stack).  Do you see the '--insert' grammar making sense anywhere else (hg rebase -r X -d Y --insert ; to insert X between Y and it's desendants)?  Just seems like we'd want to strive for a consistent ux around these operations, and if we get it right it could be very powerful.

This seems like duplicate functionality with some stuff Mateusz and I talked about at the sprint for histedit, in particular:

base $REBASE_DESTINATION
pick $THING_TO_INSERT
$ORIGINAL_RULES_HERE

It’s perhaps harder to discover, but maybe saner to use?

(I agree with Durham’s concerns around a consistent UI for this more generally, as this flag feels a little camel’s-nose-ish to me)

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -1229,6 +1229,30 @@ 
 
     return obsoletenotrebased
 
+def commitinsert(orig, ui, repo, *args, **opts):
+    '''Insert a commit and rebase the current descendants on top of it
+    '''
+    ret = None
+    if opts.get('insert'):
+        wlock = lock = None
+        try:
+            wlock = repo.wlock()
+            lock = repo.lock()
+            descendants = list(repo.revs('(.::) - .'))
+            ret = orig(ui, repo, *args, **opts)
+            rebaseopts = {
+                'dest' : '.',
+                'rev' : descendants,
+                'tool': opts.get('tool')
+            }
+
+            rebase(ui, repo, **rebaseopts)
+        finally:
+            release(lock, wlock)
+    else:
+        ret = orig(ui, repo, *args, **opts)
+    return ret
+
 def summaryhook(ui, repo):
     if not os.path.exists(repo.join('rebasestate')):
         return
@@ -1248,6 +1272,9 @@ 
 
 def uisetup(ui):
     #Replace pull with a decorator to provide --rebase option
+    entry = extensions.wrapcommand(commands.table, 'commit', commitinsert)
+    entry[1].append(('', 'insert', None,
+                     _("insert a commit and rebase descendants on top of it")))
     entry = extensions.wrapcommand(commands.table, 'pull', pullrebase)
     entry[1].append(('', 'rebase', None,
                      _("rebase working directory to branch head")))