Patchwork [RFC] histedit: add exec command (issue4036)

login
register
mail settings
Submitter Durham Goode
Date Nov. 12, 2013, 6:01 p.m.
Message ID <08b86341b720d65be3f1.1384279281@dev350.prn1.facebook.com>
Download mbox | patch
Permalink /patch/2934/
State Changes Requested, archived
Headers show

Comments

Durham Goode - Nov. 12, 2013, 6:01 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1383437476 25200
#      Sat Nov 02 17:11:16 2013 -0700
# Node ID 08b86341b720d65be3f1ce80295ad50f5ca188eb
# Parent  aa80446aacc3b1574211649cd8f190250b6b04b3
histedit: add exec command (issue4036)

This is RFC. Don't queue it (there's no documentation changes included yet).

Adds an exec/x command to the available histedit commands. This allows users
working with a stack of commits to execute an arbitrary shell command on each
commit. This is the same as the git rebase -i exec command.

For example, assume the user has a shell command 'arc amend' that adds
a "Reviewed By: soandso" line to the commit message before they push the change.
They can now do:

pick d33tb11f Blah blah
exec arc amend
pick a53cb361 More blah
exec arc amend

To run 'arc amend' on all commits at once.

Unfortunately this patch required a change to how pick works. Previously pick
would do nothing if the picked commit was already on the correct parent. The
problem with this is that it may still have child commits, so an exec can't run
amend. The patch changes pick to always 'pick' the commit, resulting in a new
commit that exec can then operate on before it picks the next commit. This is a
perf hit, adds a little more output spam, and will require updating a lot of the
hard coded hashes in the tests, so I wanted to throw it out there first before I
go update all the histedit tests.

Just some food for thought before the sprint.
Durham Goode - Nov. 12, 2013, 6:04 p.m.
On 11/12/13 10:01 AM, "Durham Goode" <durham@fb.com> wrote:

>
>diff --git a/hgext/histedit.py b/hgext/histedit.py
>--- a/hgext/histedit.py
>+++ b/hgext/histedit.py
>@@ -405,6 +402,26 @@
>     # We didn't make an edit, so just indicate no replaced nodes
>     return newctx, []
> 
>+def execute(ui, repo, ctx, ha, opts):
>+    hg.update(repo, ctx.node())
>+    os.system(ha)
>+
>+    # reset caches
>+    repo.changelog
>+    del repo.changelog
>+    repo.manifest
>+    del repo.manifest
>+    repo.dirstate
>+    del repo.dirstate
>+    repo._phasecache
>+    del repo._phasecache
>+
>+    newctx = repo['.']
>+    if not ctx.node() in repo:
>+        return newctx, [(ctx.node(), (newctx.node(),))]
>+
>+    return newctx, []
>+

It's also worth calling out that I don't think our property caches
(repo.changelog/manifest/dirstate/etc) are being invalidated correctly
when the underlying file changes. I haven't had time to dig in, but
manually invalidating the cache here was required in order for it to pick
up new changes.
Idan Kamara - Nov. 14, 2013, 1:18 p.m.
On Tue, Nov 12, 2013 at 8:04 PM, Durham Goode <durham@fb.com> wrote:

> On 11/12/13 10:01 AM, "Durham Goode" <durham@fb.com> wrote:
>
> >
> >diff --git a/hgext/histedit.py b/hgext/histedit.py
> >--- a/hgext/histedit.py
> >+++ b/hgext/histedit.py
> >@@ -405,6 +402,26 @@
> >     # We didn't make an edit, so just indicate no replaced nodes
> >     return newctx, []
> >
> >+def execute(ui, repo, ctx, ha, opts):
> >+    hg.update(repo, ctx.node())
> >+    os.system(ha)
> >+
> >+    # reset caches
> >+    repo.changelog
> >+    del repo.changelog
> >+    repo.manifest
> >+    del repo.manifest
> >+    repo.dirstate
> >+    del repo.dirstate
> >+    repo._phasecache
> >+    del repo._phasecache
> >+
> >+    newctx = repo['.']
> >+    if not ctx.node() in repo:
> >+        return newctx, [(ctx.node(), (newctx.node(),))]
> >+
> >+    return newctx, []
> >+
>
> It's also worth calling out that I don't think our property caches
> (repo.changelog/manifest/dirstate/etc) are being invalidated correctly
> when the underlying file changes. I haven't had time to dig in, but
> manually invalidating the cache here was required in order for it to pick
> up new changes.


Try calling repo.invalidate() and repo.invalidatedirstate().
Durham Goode - Nov. 16, 2013, 8:19 p.m.
On 11/12/13 10:01 AM, "Durham Goode" <durham@fb.com> wrote:

>histedit: add exec command (issue4036)
>
>This is RFC. Don't queue it (there's no documentation changes included
>yet).
>
>Adds an exec/x command to the available histedit commands. This allows
>users
>working with a stack of commits to execute an arbitrary shell command on
>each
>commit. This is the same as the git rebase -i exec command.
>
>For example, assume the user has a shell command 'arc amend' that adds
>a "Reviewed By: soandso" line to the commit message before they push the
>change.
>They can now do:
>
>pick d33tb11f Blah blah
>exec arc amend
>pick a53cb361 More blah
>exec arc amend

Talked in person at the sprint about this.  We think putting exec in a
separate command makes sense, and adding 'histprocess' or some equivalent
that operates like:

hg histprocess --rev <revset> --command <cmd>

Where each rev is rebased to not have children, has the command executed
on it, then it's children are rebased on to it.  This is better than
histedit in that it allows the command to run on nonlinear revsets.

I'll work on this later.

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -297,9 +297,6 @@ 
 
 def pick(ui, repo, ctx, ha, opts):
     oldctx = repo[ha]
-    if oldctx.parents()[0] == ctx:
-        ui.debug('node %s unchanged\n' % ha)
-        return oldctx, []
     hg.update(repo, ctx.node())
     stats = applychanges(ui, repo, oldctx, opts)
     if stats and stats[3] > 0:
@@ -405,6 +402,26 @@ 
     # We didn't make an edit, so just indicate no replaced nodes
     return newctx, []
 
+def execute(ui, repo, ctx, ha, opts):
+    hg.update(repo, ctx.node())
+    os.system(ha)
+
+    # reset caches
+    repo.changelog
+    del repo.changelog
+    repo.manifest
+    del repo.manifest
+    repo.dirstate
+    del repo.dirstate
+    repo._phasecache
+    del repo._phasecache
+
+    newctx = repo['.']
+    if not ctx.node() in repo:
+        return newctx, [(ctx.node(), (newctx.node(),))]
+
+    return newctx, []
+
 def findoutgoing(ui, repo, remote=None, force=False, opts={}):
     """utility function to find the first outgoing changeset
 
@@ -439,6 +456,8 @@ 
                'drop': drop,
                'm': message,
                'mess': message,
+               'x': execute,
+               'exec': execute,
                }
 
 @command('histedit',
@@ -752,17 +771,20 @@ 
         if ' ' not in r:
             raise util.Abort(_('malformed line "%s"') % r)
         action, rest = r.split(' ', 1)
-        ha = rest.strip().split(' ', 1)[0]
-        try:
-            ha = str(repo[ha])  # ensure its a short hash
-        except error.RepoError:
-            raise util.Abort(_('unknown changeset %s listed') % ha)
-        if ha not in expected:
-            raise util.Abort(
-                _('may not use changesets other than the ones listed'))
-        if ha in seen:
-            raise util.Abort(_('duplicated command for changeset %s') % ha)
-        seen.add(ha)
+        if action == "exec" or action == "x":
+          ha = rest
+        else:
+          ha = rest.strip().split(' ', 1)[0]
+          try:
+              ha = str(repo[ha])  # ensure its a short hash
+          except error.RepoError:
+              raise util.Abort(_('unknown changeset %s listed') % ha)
+          if ha not in expected:
+              raise util.Abort(
+                  _('may not use changesets other than the ones listed'))
+          if ha in seen:
+              raise util.Abort(_('duplicated command for changeset %s') % ha)
+          seen.add(ha)
         if action not in actiontable:
             raise util.Abort(_('unknown action "%s"') % action)
         parsed.append([action, ha])