Patchwork [1,of,9] histedit: use "editor" argument of "commit()" instead of explicit "ui.edit()"

login
register
mail settings
Submitter Katsunori FUJIWARA
Date May 5, 2014, 12:33 p.m.
Message ID <b683de8e06581ebb2a59.1399293181@feefifofum>
Download mbox | patch
Permalink /patch/4632/
State Accepted
Commit 213fd1a99cd9b330652d6fd28e8476f7cfe85e09
Headers show

Comments

Katsunori FUJIWARA - May 5, 2014, 12:33 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1399292800 -32400
#      Mon May 05 21:26:40 2014 +0900
# Branch stable
# Node ID b683de8e06581ebb2a59120b118539a839bcfb06
# Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
histedit: use "editor" argument of "commit()" instead of explicit "ui.edit()"

Before this patch, "message" action of "hg histedit" uses "ui.edit()"
explicitly to get commit message edited manually.

This requires explicit "localrepository.savecommitmessage()"
invocation to save edited commit message into ".hg/last-message.txt",
because unexpected exception raising may abort command execution
before saving it in "localrepository.commit()".

This patch uses "editor" argument of "localrepository.commit()"
instead of explicit "ui.edit()" invocation for "message" action of "hg
histedit"

"localrepository.commit()" will invoke "editor()" function newly added
in this patch, and save edited commit message into ".hg/last-message.txt"
automatically.
Pierre-Yves David - May 7, 2014, 12:14 a.m.
On 05/05/2014 05:33 AM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1399292800 -32400
> #      Mon May 05 21:26:40 2014 +0900
> # Branch stable
> # Node ID b683de8e06581ebb2a59120b118539a839bcfb06
> # Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
> histedit: use "editor" argument of "commit()" instead of explicit "ui.edit()"

After a long hesitation I ended up queuing this series to the 
clowncopter repo.

I uncomfortable with multiple aspect of this series and I'm half 
thinking that passing editor to memctx.__init__ is a layer violation 
(but also half thinking it make perfect sense).

However I finally realized that nothing in this series can be worth that 
the widespread new._test = editor(…) idioms.

Thanks alot for the cleanup!

Consider keep digging further in this direction.
Katsunori FUJIWARA - May 7, 2014, 2:17 p.m.
At Tue, 06 May 2014 17:14:30 -0700,
Pierre-Yves David wrote:
> 
> On 05/05/2014 05:33 AM, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1399292800 -32400
> > #      Mon May 05 21:26:40 2014 +0900
> > # Branch stable
> > # Node ID b683de8e06581ebb2a59120b118539a839bcfb06
> > # Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
> > histedit: use "editor" argument of "commit()" instead of explicit "ui.edit()"
> 
> After a long hesitation I ended up queuing this series to the 
> clowncopter repo.
> 
> I uncomfortable with multiple aspect of this series and I'm half 
> thinking that passing editor to memctx.__init__ is a layer violation 
> (but also half thinking it make perfect sense).
> 
> However I finally realized that nothing in this series can be worth that 
> the widespread new._test = editor(…) idioms.
> 
> Thanks alot for the cleanup!
> 
> Consider keep digging further in this direction.

In fact, I already finished to work for patches below:

  - replace explicit "opts['edit']" checking and choosing
    "commit*editor" by specific utility function to simplify code path

      before:
          editor = cmdutil.commiteditor # or editor = None/False
          if opts.get('edit'):
              editor = cmdutil.commitforceeditor

      after:
          editor = cmdutil.getcommiteditor(**opts)

  - replace explicit "ui.edit()" using by newly added utility function

      before:
          def editor(repo, ctx, subs):
              return ui.edit(ctx.description() + "\n", ctx.user())

      after:
          editor = cmdutil.getcommiteditor(edit=True)

but didn't include them into this series to keep this series small
enough, because above consists of over 10 patches :-)

Then, are there any other refactoring points you suppose ?


----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Pierre-Yves David - May 7, 2014, 6:10 p.m.
On 05/07/2014 07:17 AM, FUJIWARA Katsunori wrote:
>
> At Tue, 06 May 2014 17:14:30 -0700,
> Pierre-Yves David wrote:
>>
>> On 05/05/2014 05:33 AM, FUJIWARA Katsunori wrote:
>>> # HG changeset patch
>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>>> # Date 1399292800 -32400
>>> #      Mon May 05 21:26:40 2014 +0900
>>> # Branch stable
>>> # Node ID b683de8e06581ebb2a59120b118539a839bcfb06
>>> # Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
>>> histedit: use "editor" argument of "commit()" instead of explicit "ui.edit()"
>>
>> After a long hesitation I ended up queuing this series to the
>> clowncopter repo.
>>
>> I uncomfortable with multiple aspect of this series and I'm half
>> thinking that passing editor to memctx.__init__ is a layer violation
>> (but also half thinking it make perfect sense).
>>
>> However I finally realized that nothing in this series can be worth that
>> the widespread new._test = editor(…) idioms.
>>
>> Thanks alot for the cleanup!
>>
>> Consider keep digging further in this direction.
>
> In fact, I already finished to work for patches below:
>
>    - replace explicit "opts['edit']" checking and choosing
>      "commit*editor" by specific utility function to simplify code path
>
>        before:
>            editor = cmdutil.commiteditor # or editor = None/False
>            if opts.get('edit'):
>                editor = cmdutil.commitforceeditor
>
>        after:
>            editor = cmdutil.getcommiteditor(**opts)
>
>    - replace explicit "ui.edit()" using by newly added utility function
>
>        before:
>            def editor(repo, ctx, subs):
>                return ui.edit(ctx.description() + "\n", ctx.user())
>
>        after:
>            editor = cmdutil.getcommiteditor(edit=True)
>
> but didn't include them into this series to keep this series small
> enough, because above consists of over 10 patches :-)
>
> Then, are there any other refactoring points you suppose ?

Yes

1. first I over looked the lack of doc in your memctx change. I would be 
dashed cool if you could update the function documentation explaining:

* What are the expected prototype this ancestors function
* its expected behavior
* its interaction with the `text` argument of the same function
* Why it is necessary for this function to be there

2. getting savecommitmessage out of local repo would be a plus

3. I;m stil not super fan of this function passed to __init__ but I 
can't think of anything better for now.

Cheers,
Sean Farley - May 7, 2014, 6:42 p.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 05/07/2014 07:17 AM, FUJIWARA Katsunori wrote:
>>
>> At Tue, 06 May 2014 17:14:30 -0700,
>> Pierre-Yves David wrote:
>>>
>>> On 05/05/2014 05:33 AM, FUJIWARA Katsunori wrote:
>>>> # HG changeset patch
>>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>>>> # Date 1399292800 -32400
>>>> #      Mon May 05 21:26:40 2014 +0900
>>>> # Branch stable
>>>> # Node ID b683de8e06581ebb2a59120b118539a839bcfb06
>>>> # Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
>>>> histedit: use "editor" argument of "commit()" instead of explicit "ui.edit()"
>>>
>>> After a long hesitation I ended up queuing this series to the
>>> clowncopter repo.
>>>
>>> I uncomfortable with multiple aspect of this series and I'm half
>>> thinking that passing editor to memctx.__init__ is a layer violation
>>> (but also half thinking it make perfect sense).
>>>
>>> However I finally realized that nothing in this series can be worth that
>>> the widespread new._test = editor(…) idioms.
>>>
>>> Thanks alot for the cleanup!
>>>
>>> Consider keep digging further in this direction.
>>
>> In fact, I already finished to work for patches below:
>>
>>    - replace explicit "opts['edit']" checking and choosing
>>      "commit*editor" by specific utility function to simplify code path
>>
>>        before:
>>            editor = cmdutil.commiteditor # or editor = None/False
>>            if opts.get('edit'):
>>                editor = cmdutil.commitforceeditor
>>
>>        after:
>>            editor = cmdutil.getcommiteditor(**opts)
>>
>>    - replace explicit "ui.edit()" using by newly added utility function
>>
>>        before:
>>            def editor(repo, ctx, subs):
>>                return ui.edit(ctx.description() + "\n", ctx.user())
>>
>>        after:
>>            editor = cmdutil.getcommiteditor(edit=True)
>>
>> but didn't include them into this series to keep this series small
>> enough, because above consists of over 10 patches :-)
>>
>> Then, are there any other refactoring points you suppose ?
>
> Yes
>
> 1. first I over looked the lack of doc in your memctx change. I would be 
> dashed cool if you could update the function documentation explaining:
>
> * What are the expected prototype this ancestors function
> * its expected behavior
> * its interaction with the `text` argument of the same function
> * Why it is necessary for this function to be there

Can we bikeshed this until after my memctx changes? Or, at the very
least, let me do this so I don't have to deal with conflicts?

> 2. getting savecommitmessage out of local repo would be a plus
>
> 3. I;m stil not super fan of this function passed to __init__ but I 
> can't think of anything better for now.

The last commit message / savedcommitmessage seems to be part of a
committablectx, so how about this:

- we go through my patches to move repo.commit to committablectx.commit
- move savecommitmessage to committablectx
- refactor ui.edit logic to use this savecommitmessage
- see if we can clean up these calls to 'editor' by using memctx

?

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -402,12 +402,12 @@ 
     if stats and stats[3] > 0:
         raise error.InterventionRequired(
             _('Fix up the change and run hg histedit --continue'))
-    message = oldctx.description() + '\n'
-    message = ui.edit(message, ui.username())
-    repo.savecommitmessage(message)
+    message = oldctx.description()
+    def editor(repo, ctx, subs):
+        return ui.edit(ctx.description() + "\n", ctx.user())
     commit = commitfuncfor(repo, oldctx)
     new = commit(text=message, user=oldctx.user(), date=oldctx.date(),
-                 extra=oldctx.extra())
+                 extra=oldctx.extra(), editor=editor)
     newctx = repo[new]
     if oldctx.node() != newctx.node():
         return newctx, [(oldctx.node(), (new,))]
diff --git a/tests/test-histedit-edit.t b/tests/test-histedit-edit.t
--- a/tests/test-histedit-edit.t
+++ b/tests/test-histedit-edit.t
@@ -200,8 +200,9 @@ 
   >             raise util.Abort('emulating unexpected abort')
   >     repo.__class__ = commitfailure
   > EOF
-  $ cat > .hg/hgrc <<EOF
+  $ cat >> .hg/hgrc <<EOF
   > [extensions]
+  > # this failure occurs before editor invocation
   > commitfailure = $TESTTMP/commitfailure.py
   > EOF
 
@@ -211,22 +212,53 @@ 
   > echo "===="
   > echo "check saving last-message.txt" >> \$1
   > EOF
+
+(test that editor is not invoked before transaction starting)
+
   $ rm -f .hg/last-message.txt
   $ HGEDITOR="sh $TESTTMP/editor.sh" hg histedit tip --commands - 2>&1 << EOF | fixbundle
   > mess 1fd3b2fe7754 f
   > EOF
   0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  abort: emulating unexpected abort
+  $ cat .hg/last-message.txt
+  cat: .hg/last-message.txt: No such file or directory
+  [1]
+
+  $ cat >> .hg/hgrc <<EOF
+  > [extensions]
+  > commitfailure = !
+  > EOF
+  $ hg histedit --abort -q
+
+(test that editor is invoked and commit message is saved into
+"last-message.txt")
+
+  $ cat >> .hg/hgrc <<EOF
+  > [hooks]
+  > # this failure occurs after editor invocation
+  > pretxncommit.unexpectedabort = false
+  > EOF
+
+  $ rm -f .hg/last-message.txt
+  $ HGEDITOR="sh $TESTTMP/editor.sh" hg histedit tip --commands - 2>&1 << EOF | fixbundle
+  > mess 1fd3b2fe7754 f
+  > EOF
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
   ==== before editing
   f
   ====
-  abort: emulating unexpected abort
+  transaction abort!
+  rollback completed
+  note: commit message saved in .hg/last-message.txt
+  abort: pretxncommit.unexpectedabort hook exited with status 1
   $ cat .hg/last-message.txt
   f
   check saving last-message.txt
 
-  $ cat > .hg/hgrc <<EOF
-  > [extensions]
-  > commitfailure = !
+  $ cat >> .hg/hgrc <<EOF
+  > [hooks]
+  > pretxncommit.unexpectedabort =
   > EOF
   $ hg histedit --abort -q