Patchwork histedit: do not check experimental.histediting in extsetup

login
register
mail settings
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 - March 9, 2016, 3:49 a.m.
# 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.
Sean Farley - March 9, 2016, 7:11 p.m.
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.
Pierre-Yves David - March 10, 2016, 3:48 p.m.
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
>
Jun Wu - March 10, 2016, 3:54 p.m.
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.
Pierre-Yves David - March 10, 2016, 3:56 p.m.
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,
Jun Wu - March 10, 2016, 4:23 p.m.
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.
Pierre-Yves David - March 10, 2016, 4:43 p.m.
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
Jun Wu - March 10, 2016, 5:11 p.m.
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?
Pierre-Yves David - March 10, 2016, 5:17 p.m.
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.
timeless - March 10, 2016, 5:57 p.m.
-        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?
Jun Wu - March 11, 2016, 10:57 a.m.
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.
Yuya Nishihara - March 11, 2016, 11:37 a.m.
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)