Patchwork [1,of,2,v2] histedit: add execute method

# HG changeset patch
# User David Soria Parra <davidsp@fb.com>
# Date 1410901051 25200
#      Tue Sep 16 13:57:31 2014 -0700
# Node ID d4e975e2707a0ea606c22ed472d56fab4fda7ae2
# Parent  ca854cd4a26a8770fbc22b4d7ee1ac6823b682a5
histedit: add execute method

Add a method to execute command after a changeset is picked, folded, etc.

On 09/16/2014 05:58 PM, David Soria Parra wrote:
> # HG changeset patch
> # User David Soria Parra <davidsp@fb.com>
> # Date 1410901051 25200
> #      Tue Sep 16 13:57:31 2014 -0700
> # Node ID d4e975e2707a0ea606c22ed472d56fab4fda7ae2
> # Parent  ca854cd4a26a8770fbc22b4d7ee1ac6823b682a5
> histedit: add execute method
>
> Add a method to execute command after a changeset is picked, folded, etc.

We need more details about what happens if the command rewrite history. 
and what happen when the repo is left dirty (I assume the logic from 
edit kicks in an get you to commit them)

>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -152,6 +152,7 @@
>   import errno
>   import os
>   import sys
> +import subprocess
>
>   from mercurial import cmdutil
>   from mercurial import discovery
> @@ -406,6 +407,30 @@
>       return ctx, [(repo[ha].node(), ())]
>
>
> +def execute(ui, repo, ctx, cmd, opts):
> +    hg.update(repo, ctx.node())
> +
> +    # release locks so the programm can call hg and then relock.
> +    release(repo._histeditlock, repo._histeditwlock)
> +
> +    process = subprocess.Popen(cmd, close_fds=True, shell=True,
> +            cwd=repo.root)
> +    process.communicate()
> +
> +    # relock the repository
> +    repo._histeditlock = repo.lock()
> +    repo._histeditwlock = repo.wlock()

You are locking in the wrong order, wlock should go first.

Also, can we use something else than monkey patching localrepo?

Finally, We need a stronger unlocking here. If someone else took the 
lock before call histedit, the release call will just decrement the 
depth and not unlock the underlying repo.

> +    if process.returncode != 0:
> +        raise error.InterventionRequired(
> +            _("Command '%s' failed with exit status %d.") % (cmd,
> +                process.returncode))
> +    if util.any(repo.status()[:4]):
> +        raise error.InterventionRequired(
> +            _('Working copy dirty, please check the files listed above.\n'
> +              'When you are finished, run hg histedit --continue to resume.'))
> +    return ctx, []
> +
>   def message(ui, repo, ctx, ha, opts):
>       oldctx = repo[ha]
>       hg.update(repo, ctx.node())
> @@ -500,11 +525,11 @@
>       """
>       lock = wlock = None
>       try:
> -        wlock = repo.wlock()
> -        lock = repo.lock()
> +        repo._histeditwlock = repo.wlock()
> +        repo._histeditlock = repo.lock()
>           _histedit(ui, repo, *freeargs, **opts)
>       finally:
> -        release(lock, wlock)
> +        release(repo._histeditlock, repo._histeditwlock)
>
>   def _histedit(ui, repo, *freeargs, **opts):
>       # TODO only abort if we try and histedit mq patches, not just
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>

On Fri, Sep 19, 2014 at 05:57:32PM -0700, Pierre-Yves David wrote:
>
>
> On 09/16/2014 05:58 PM, David Soria Parra wrote:
> ># HG changeset patch
> ># User David Soria Parra <davidsp@fb.com>
> ># Date 1410901051 25200
> >#      Tue Sep 16 13:57:31 2014 -0700
> ># Node ID d4e975e2707a0ea606c22ed472d56fab4fda7ae2
> ># Parent  ca854cd4a26a8770fbc22b4d7ee1ac6823b682a5
> >histedit: add execute method
> >
> >Add a method to execute command after a changeset is picked, folded, etc.
>
> We need more details about what happens if the command rewrite history. and
> what happen when the repo is left dirty (I assume the logic from edit kicks
> in an get you to commit them)

+1

>
> >
> >diff --git a/hgext/histedit.py b/hgext/histedit.py
> >--- a/hgext/histedit.py
> >+++ b/hgext/histedit.py
> >@@ -152,6 +152,7 @@
> >  import errno
> >  import os
> >  import sys
> >+import subprocess
> >
> >  from mercurial import cmdutil
> >  from mercurial import discovery
> >@@ -406,6 +407,30 @@
> >      return ctx, [(repo[ha].node(), ())]
> >
> >
> >+def execute(ui, repo, ctx, cmd, opts):
> >+    hg.update(repo, ctx.node())
> >+
> >+    # release locks so the programm can call hg and then relock.
> >+    release(repo._histeditlock, repo._histeditwlock)
> >+
> >+    process = subprocess.Popen(cmd, close_fds=True, shell=True,
> >+            cwd=repo.root)
> >+    process.communicate()
> >+
> >+    # relock the repository
> >+    repo._histeditlock = repo.lock()
> >+    repo._histeditwlock = repo.wlock()
>
> You are locking in the wrong order, wlock should go first.

Eep. I don't think I knew that. I wonder if we can add that to the
lock-checker that's in contrib.

>
> Also, can we use something else than monkey patching localrepo?
>
> Finally, We need a stronger unlocking here. If someone else took the lock
> before call histedit, the release call will just decrement the depth and not
> unlock the underlying repo.

I defer to marmoute in this area - I've not thought too hard about our locking.

>
> >+    if process.returncode != 0:
> >+        raise error.InterventionRequired(
> >+            _("Command '%s' failed with exit status %d.") % (cmd,
> >+                process.returncode))
> >+    if util.any(repo.status()[:4]):
> >+        raise error.InterventionRequired(
> >+            _('Working copy dirty, please check the files listed above.\n'
> >+              'When you are finished, run hg histedit --continue to resume.'))
> >+    return ctx, []
> >+
> >  def message(ui, repo, ctx, ha, opts):
> >      oldctx = repo[ha]
> >      hg.update(repo, ctx.node())
> >@@ -500,11 +525,11 @@
> >      """
> >      lock = wlock = None
> >      try:
> >-        wlock = repo.wlock()
> >-        lock = repo.lock()
> >+        repo._histeditwlock = repo.wlock()
> >+        repo._histeditlock = repo.lock()
> >          _histedit(ui, repo, *freeargs, **opts)
> >      finally:
> >-        release(lock, wlock)
> >+        release(repo._histeditlock, repo._histeditwlock)
> >
> >  def _histedit(ui, repo, *freeargs, **opts):
> >      # TODO only abort if we try and histedit mq patches, not just
> >_______________________________________________
> >Mercurial-devel mailing list
> >Mercurial-devel@selenic.com
> >http://selenic.com/mailman/listinfo/mercurial-devel
> >
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel