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
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.
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
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,
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