| Submitter | Jun Wu |
|---|---|
| Date | March 9, 2016, 3:49 a.m. |
| Message ID | <4886a03b8b2b779cf9cf.1457495366@x1c> |
| Download | mbox | patch |
| Permalink | /patch/13718/ |
| State | Changes Requested |
| Delegated to: | Pierre-Yves David |
| Headers | show |
Comments
Jun Wu <quark@fb.com> writes: > # HG changeset patch > # User Jun Wu <quark@fb.com> > # Date 1457490597 0 > # Wed Mar 09 02:29:57 2016 +0000 > # Node ID 4886a03b8b2b779cf9cf69a5dc29a2b4e8b24c52 > # Parent ffd3ac07b1d79dda7f57bd826208fdaf92a76717 > histedit: do not check experimental.histediting in extsetup > > chgserver only hashes [extensions] as confighash. It expects extensions to not > have global side effects depending on configs in {ui,ext}setup. Otherwise, > if related configs are changed, but the [extensions] section remains same, > a same chgserver will be reused and the extensions will still have the old > behavior. > > This patch fixes the issue by moving experimental.histediting check out from > extsetup. Seems fine to me.
On 03/09/2016 03:49 AM, Jun Wu wrote: > # HG changeset patch > # User Jun Wu <quark@fb.com> > # Date 1457490597 0 > # Wed Mar 09 02:29:57 2016 +0000 > # Node ID 4886a03b8b2b779cf9cf69a5dc29a2b4e8b24c52 > # Parent ffd3ac07b1d79dda7f57bd826208fdaf92a76717 > histedit: do not check experimental.histediting in extsetup > > chgserver only hashes [extensions] as confighash. It expects extensions to not > have global side effects depending on configs in {ui,ext}setup. Otherwise, > if related configs are changed, but the [extensions] section remains same, > a same chgserver will be reused and the extensions will still have the old > behavior. > > This patch fixes the issue by moving experimental.histediting check out from > extsetup. So what is the new way to handle the "ng" action? Do we still have a global state but a delayed one? > > diff --git a/hgext/histedit.py b/hgext/histedit.py > --- a/hgext/histedit.py > +++ b/hgext/histedit.py > @@ -219,6 +219,13 @@ > secondaryactions = set() > tertiaryactions = set() > internalactions = set() > +hiddenactions = set() > + > +def _updatehiddenactions(ui): > + hiddenactions.clear() > + if not ui.configbool("experimental", "histeditng"): > + hiddenactions.add('base') > + hiddenactions.add('b') > > def geteditcomment(first, last): > """ construct the editor comment > @@ -250,7 +257,8 @@ > sorted(secondaryactions) + > sorted(tertiaryactions) > ): > - addverb(v) > + if v not in hiddenactions: > + addverb(v) > actions.append('') > > return ''.join(['# %s\n' % l if l else '#\n' > @@ -782,6 +790,8 @@ > replacements.append((ich, (n,))) > return repo[n], replacements > > +@action(['base', 'b'], > + _('checkout changeset and apply further changesets from there')) > class base(histeditaction): > def constraints(self): > return set([_constraints.forceother]) > @@ -1048,6 +1058,7 @@ > state.keep = opts.get('keep', False) > > _validateargs(ui, repo, state, freeargs, opts, goal, rules, revs) > + _updatehiddenactions(ui) > > # rebuild state > if goal == goalcontinue: > @@ -1321,7 +1332,7 @@ > raise error.ParseError(_('malformed line "%s"') % r) > verb, rest = r.split(' ', 1) > > - if verb not in actiontable: > + if verb not in actiontable or verb in hiddenactions: > raise error.ParseError(_('unknown action "%s"') % verb) > > action = actiontable[verb].fromrule(state, rest) > @@ -1581,7 +1592,3 @@ > _("use 'hg histedit --continue' or 'hg histedit --abort'")]) > cmdutil.afterresolvedstates.append( > ['histedit-state', _('hg histedit --continue')]) > - if ui.configbool("experimental", "histeditng"): > - globals()['base'] = action(['base', 'b'], > - _('checkout changeset and apply further changesets from there') > - )(base) > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel >
On 03/10/2016 03:48 PM, Pierre-Yves David wrote: > So what is the new way to handle the "ng" action? Do we still have a global > state but a delayed one? I think the change makes the style more consistent. There are a few *actions variables scoped in the histedit module, which looks okay to me. I just added another one. The state (hiddenactions) is re-calculated per histedit command so it will be always up-to-date.
On 03/10/2016 03:54 PM, Jun Wu wrote: > On 03/10/2016 03:48 PM, Pierre-Yves David wrote: >> So what is the new way to handle the "ng" action? Do we still have a >> global >> state but a delayed one? > > I think the change makes the style more consistent. There are a few > *actions > variables scoped in the histedit module, which looks okay to me. I just > added > another one. > > The state (hiddenactions) is re-calculated per histedit command so it > will be > always up-to-date. But it is still in a global variable. If this is recomputed on the fly, can't we keep it in the scope of the function? Cheers,
On 03/10/2016 03:56 PM, Pierre-Yves David wrote: > But it is still in a global variable. If this is recomputed on the fly, > can't we keep it in the scope of the function? It requires "ui" to compute. We don't pass "ui" down to the functions which actually need it. I don't think passing down the "ui" object (the source of truth) is a good idea. If you add parameters to pass the *actions, you probably want to pass primaryactions, secondaryactions, tertiaryactions, internalactions, hiddenactions as well and it will be a huge change because a lot of functions will get involved. It will also prevent other extensions from adding new histedit actions. In general, I don't think the result will be better than the current code.
On 03/10/2016 04:23 PM, Jun Wu wrote: > On 03/10/2016 03:56 PM, Pierre-Yves David wrote: >> But it is still in a global variable. If this is recomputed on the fly, >> can't we keep it in the scope of the function? > > It requires "ui" to compute. We don't pass "ui" down to the functions which > actually need it. I don't think passing down the "ui" object (the source of > truth) is a good idea. could we stick this in the histedit state? 1) the state is already passed around 2) If you start as ng, you need to keep being ng anyway. > If you add parameters to pass the *actions, you probably want to pass > primaryactions, secondaryactions, tertiaryactions, internalactions, > hiddenactions as well and it will be a huge change because a lot of > functions We could gathers them into an "actions" variable > will get involved. It will also prevent other extensions from adding new > histedit actions. In general, I don't think the result will be better > than the current code. Given that the current code is using global variable to communication data between functions in the stacks I've an hard time to believe we can't do better :-/ Cheers
On 03/10/2016 04:43 PM, Pierre-Yves David wrote: > could we stick this in the histedit state? > > 1) the state is already passed around > 2) If you start as ng, you need to keep being ng anyway. The histeditstate is actually relying on the filesystem, which I think is even worse than the top-level module variables. > We could gathers them into an "actions" variable But where do you put the "actions" variable? If it is still a top-level module variable, I don't think the change is worthwhile. If it is to add a new parameter to every functions. As I said, it is a huge because a lot of functions are involved, and consider about how could other extensions add histedit actions. > Given that the current code is using global variable to communication data > between functions in the stacks I've an hard time to believe we can't do better I assume you want to put "actions" (or whatever it called) to every functions. I will argue the current design is actually "better": 1) Flexible (important) 3rd extension can add histedit actions, which can be a deal breaker. For example, I can add an action called "spellcheck" to filter the message to a spellchecker and apply the result. 2) Short. The code is shorter and therefore needs less time to read (and write). Think about histedit module as a closure and it's just maintaining its state in the closure. I think it's pretty normal. Could you explain why this is bad?
On 03/10/2016 05:11 PM, Jun Wu wrote: > On 03/10/2016 04:43 PM, Pierre-Yves David wrote: >> could we stick this in the histedit state? >> >> 1) the state is already passed around >> 2) If you start as ng, you need to keep being ng anyway. > > The histeditstate is actually relying on the filesystem, which I think is > even worse than the top-level module variables. But the filesystem bit are relative to a single repository that we lock. >> We could gathers them into an "actions" variable > > But where do you put the "actions" variable? > If it is still a top-level module variable, I don't think the change is > worthwhile. > If it is to add a new parameter to every functions. As I said, it is a > huge because a lot of functions are involved, and consider about how > could other extensions add histedit actions. My bottom line is: Don't use global variable for anything else than constant. I'm open to a lot of solution as long as they pass the "no global variable" check. > >> Given that the current code is using global variable to communication >> data >> between functions in the stacks I've an hard time to believe we can't >> do better > > I assume you want to put "actions" (or whatever it called) to every > functions. Either explicitly passign actions, or attaching them to the state object. Or adding the necessary bit to the state object or anything else not involving global variable. > I will argue the current design is actually "better": > 1) Flexible (important) > 3rd extension can add histedit actions, which can be a deal breaker. > For example, I can add an action called "spellcheck" to filter the > message > to a spellchecker and apply the result. We don't need to sacrifice extensible when moving away from global variable, there is a bunch of place that don't in the code (eg: capabilities) > 2) Short. > The code is shorter and therefore needs less time to read (and write). Global variables does not help code readability. Communicating data through global variables is plain bad.
- if verb not in actiontable: + if verb not in actiontable or verb in hiddenactions: This isn't the easiest logic to parse. Can you reverse the order?
On 03/10/2016 05:57 PM, timeless wrote: > - if verb not in actiontable: > + if verb not in actiontable or verb in hiddenactions: > > This isn't the easiest logic to parse. > Can you reverse the order? Good idea. Pierre-Yves: can I send a V2 just with this change? I don't want to do a large refactoring with little or negative benefit.
On Thu, 10 Mar 2016 16:23:58 +0000, Jun Wu wrote: > On 03/10/2016 03:56 PM, Pierre-Yves David wrote: > > But it is still in a global variable. If this is recomputed on the fly, > > can't we keep it in the scope of the function? > > It requires "ui" to compute. We don't pass "ui" down to the functions which > actually need it. I don't think passing down the "ui" object (the source of > truth) is a good idea. I don't think it's bad to pass "ui" to geteditcomment() and parserules() as these functions seem to be near the ui layer. Mutating global variables are source of trouble because it hides data dependencies and it mostly works even if you don't update the globals correctly.
Patch
diff --git a/hgext/histedit.py b/hgext/histedit.py --- a/hgext/histedit.py +++ b/hgext/histedit.py @@ -219,6 +219,13 @@ secondaryactions = set() tertiaryactions = set() internalactions = set() +hiddenactions = set() + +def _updatehiddenactions(ui): + hiddenactions.clear() + if not ui.configbool("experimental", "histeditng"): + hiddenactions.add('base') + hiddenactions.add('b') def geteditcomment(first, last): """ construct the editor comment @@ -250,7 +257,8 @@ sorted(secondaryactions) + sorted(tertiaryactions) ): - addverb(v) + if v not in hiddenactions: + addverb(v) actions.append('') return ''.join(['# %s\n' % l if l else '#\n' @@ -782,6 +790,8 @@ replacements.append((ich, (n,))) return repo[n], replacements +@action(['base', 'b'], + _('checkout changeset and apply further changesets from there')) class base(histeditaction): def constraints(self): return set([_constraints.forceother]) @@ -1048,6 +1058,7 @@ state.keep = opts.get('keep', False) _validateargs(ui, repo, state, freeargs, opts, goal, rules, revs) + _updatehiddenactions(ui) # rebuild state if goal == goalcontinue: @@ -1321,7 +1332,7 @@ raise error.ParseError(_('malformed line "%s"') % r) verb, rest = r.split(' ', 1) - if verb not in actiontable: + if verb not in actiontable or verb in hiddenactions: raise error.ParseError(_('unknown action "%s"') % verb) action = actiontable[verb].fromrule(state, rest) @@ -1581,7 +1592,3 @@ _("use 'hg histedit --continue' or 'hg histedit --abort'")]) cmdutil.afterresolvedstates.append( ['histedit-state', _('hg histedit --continue')]) - if ui.configbool("experimental", "histeditng"): - globals()['base'] = action(['base', 'b'], - _('checkout changeset and apply further changesets from there') - )(base)
# HG changeset patch # User Jun Wu <quark@fb.com> # Date 1457490597 0 # Wed Mar 09 02:29:57 2016 +0000 # Node ID 4886a03b8b2b779cf9cf69a5dc29a2b4e8b24c52 # Parent ffd3ac07b1d79dda7f57bd826208fdaf92a76717 histedit: do not check experimental.histediting in extsetup chgserver only hashes [extensions] as confighash. It expects extensions to not have global side effects depending on configs in {ui,ext}setup. Otherwise, if related configs are changed, but the [extensions] section remains same, a same chgserver will be reused and the extensions will still have the old behavior. This patch fixes the issue by moving experimental.histediting check out from extsetup.