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

[Sean Farley]

Pierre-Yves David <pierre-yves.david at 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 at 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

?