[PATCH] mq: defer command wrapping to extsetup

[Idan Kamara]

On Mon, Jun 11, 2012 at 12:09 PM, Pierre-Yves David <
pierre-yves.david at logilab.fr> wrote:
>
> 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.

Why does it matter in terms of APi that this was moved from uisetup()
to extsetup()?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20120611/221552ab/attachment-0002.html>