[PATCH] mq: defer command wrapping to extsetup

Pierre-Yves David pierre-yves.david at logilab.fr
Mon Jun 11 04:09:06 CDT 2012


On Sat, Jun 09, 2012 at 11:39:32AM -0400, Matt Harbison wrote:
> Pierre-Yves David wrote:
> >On 9 juin 2012, at 06:33, Matt Harbison wrote:
> >This seems a "big" changes. Can you elaborate in you changeset description what kind of operation are moved from uisetup to extsetup?
> >
> 
> Hi Pierre-Yves,
> 
> How about:
> 
> mq: defer command wrapping to extsetup
> 
> mq wraps all commands that are not in commands.norepo, which is now
> performed in this second phase of the extensions setup process.
> This goes against the current best practices on the wiki [1] as far
> as where command wrapping is performed, but follows it regarding
> where global options are injected.
> 
> mq needs to be the first layer called when command dispatching in
> order to consistently retarget to the patch repo, regardless of the
> load order of the extensions.  This means being the last to wrap the
> command table.  Previously, 'hg <extdiff> --mq' would diff the main
> repo unless mq was enabled after extdiff.
> 
> [1] http://mercurial.selenic.com/wiki/WritingExtensions

Looks good to me. You should add a (API) flags in the first line.

A small in code comment that explain why wrapping are done in extsetup is
probably a good idea too.

> >It seems to be command wrapping only (which belong to "uisetup"). But this is command wrapping to add a "global options" which belong to "extsetup"?
> >
> 
> Yes, and I wasn't sure how to reconcile the two.  I had an email
> composed to ask, but the patch was shorter :-)  But, does that mean
> import, summary and init should go back to uisetup?  I still think
> mq should be the last to wrap those too for consistency, though I
> can't imagine there will be a lot of issues there either way.

I've checked the patch more closely and I did not saw any former content of
uisetup that does not deserve to move to extsetup. I think the patch is fine as
it is.

> >The patch looks good otherwise. It will probably deserve a warning in the next changelog given the amount of extensions that may interact with mq.
> >
> 
> Is there something I need to do for the changelog, or is this a note
> to Matt?

The wiki says:

> use '(BC)' to flag backward compatibility changes, use '(API)' to flag major internal API changes.

So just stick an (API) in your first line.

-- 
Pierre-Yves David

http://www.logilab.fr/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20120611/7d5d7c72/attachment.pgp>


More information about the Mercurial-devel mailing list