[PATCH 1 of 3] dispatch: store args and command run on ui

Jun Wu quark at fb.com
Mon Mar 20 12:16:27 EDT 2017


Excerpts from Ryan McElroy's message of 2017-03-20 11:43:22 +0000:
> I think Jun needs to weigh in on this since the ui object is something 
> he's wanted to make immutable for a while and this doesn't seem to help 
> move us in that direction.

Immutable is for the config object. I'm +1 to add ui.argv, because
"sys.argv" is unreliable in chg's case and my attempt to change "sys.argv"
got rejected.

> 
> On 3/20/17 7:49 AM, Phil Cohen wrote:
> > # HG changeset patch
> > # User Phil Cohen <phillco at fb.com>
> > # Date 1489995201 25200
> > #      Mon Mar 20 00:33:21 2017 -0700
> > # Node ID 2c1d5a02ec533055f182b0cc1280107fbfb76206
> > # Parent  291951ad070b3fa39dd1d83503aa1011a20d9a21
> > dispatch: store args and command run on ui
> >
> > This could also be used to simplify cases where we wrap `dispatch.runcommand` in
> > extensions to get this information (e.g. in resolve.py and keyword.py)
> 
> This summary is a bit of a non-sequitur: you describe additional things 
> you can do, but you haven't yet described the main thing you intend to 
> do. Please take a small step back and describe how you intend to use 
> this at first, then include this "could also be used" statement.
> 
> >
> > diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> > --- a/mercurial/dispatch.py
> > +++ b/mercurial/dispatch.py
> > @@ -830,6 +830,8 @@
> >               ui.warn(_("warning: --repository ignored\n"))
> >   
> >           msg = ' '.join(' ' in a and repr(a) or a for a in fullargs)
> 
> It seems that we should do some kind of shell escaping here to be 
> "correct", but I don't know how important that is for this use-case (and 
> you didn't change it yourself, so feel free to ignore me).

This is for ui.log / blackbox. It could be a separate patch.

> 
> > +        ui.args = fullargs
> 
> I'm afraid that a name like 'ui.args' will tempt others to start using 
> it to grab arguments out of, which would be very bad (TM). Instead, I'd 
> suggest naming this "argstring" or "fullargs" to disambiguate it.

It's meant to be a replacement for sys.argv. "fullargs" looks good to me.

> 
> > +        ui.commandname = cmd
> 
> Personally, I'd prefer more "private" variables set through setter 
> functions rather than this set-public-vars-directly style, but I'm not 
> sure if the community is onboard with that style yet (if not, I'll keep 
> working on it :-).

While it'd be nice if it's more private and read-only to normal code, I'm a
bit concerned that we make code unnecessarily long. Maybe we can add some
check-code rules to forbid other modules from modifying some ui fields
(including underscore ones).

> 
> >           ui.log("command", '%s\n', msg)
> >           strcmdopt = pycompat.strkwargs(cmdoptions)
> >           d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
> > diff --git a/mercurial/ui.py b/mercurial/ui.py
> > --- a/mercurial/ui.py
> > +++ b/mercurial/ui.py
> > @@ -160,6 +160,8 @@
> >           self._colormode = None
> >           self._terminfoparams = {}
> >           self._styles = {}
> > +        self.commandname = None
> > +        self.args = None
> >   
> >           if src:
> >               self.fout = src.fout
> >
> 


More information about the Mercurial-devel mailing list