Patchwork [evolve-ext] inhibit: direct access with and without warning on a per command basis

login
register
mail settings
Submitter Laurent Charignon
Date May 11, 2015, 11:56 p.m.
Message ID <23a0b73d771fe6b675e5.1431388613@lcharignon-mbp.local>
Download mbox | patch
Permalink /patch/9016/
State Superseded
Headers show

Comments

Laurent Charignon - May 11, 2015, 11:56 p.m.
# HG changeset patch
# User Laurent Charignon <lcharignon@fb.com>
# Date 1428086461 25200
#      Fri Apr 03 11:41:01 2015 -0700
# Node ID 23a0b73d771fe6b675e52f000ffb47768c4ad474
# Parent  5e82d78f5872c9503d7b6691c594a13794a9b4a4
inhibit: direct access with and without warning on a per command basis

We introduce a new filtername visibile-directaccess-nowarn to enable direct
access with no warning on a per command basis.
The motivation behing this change is to display warning when attempting direct
access in destructive commands.
Pierre-Yves David - May 12, 2015, 7:34 p.m.
On 05/11/2015 04:56 PM, Laurent Charignon wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon@fb.com>
> # Date 1428086461 25200
> #      Fri Apr 03 11:41:01 2015 -0700
> # Node ID 23a0b73d771fe6b675e52f000ffb47768c4ad474
> # Parent  5e82d78f5872c9503d7b6691c594a13794a9b4a4
> inhibit: direct access with and without warning on a per command basis
>
> We introduce a new filtername visibile-directaccess-nowarn to enable direct
> access with no warning on a per command basis.
> The motivation behing this change is to display warning when attempting direct
> access in destructive commands.
>
> diff --git a/hgext/inhibit.py b/hgext/inhibit.py
> --- a/hgext/inhibit.py
> +++ b/hgext/inhibit.py
> @@ -37,6 +37,10 @@
>   cmdtable = {}
>   command = cmdutil.command(cmdtable)
>
> +# List of commands where no warning is shown for direct access
> +directaccessnowarning = ['update', 'export']
> +directaccessnowarningevolve = ['prune']

We will need to do that for an arbitrary number of extensions.

I suggest the following format:

directaccesslevel = [
     (False, None. 'update'), # no warning, core command, command name
     (True,  'rebase'. 'rebase'), # warning, rebase extensions, command name
]

remember we have three level:

- no direct access (suggestion: not in the list)
- direct access with warning (suggestion: first item to True)
- direct access without warning (sugguestion: first item to False)


The current suggestion is conservative, we can changes the behavior once 
we have all in place.

There is also obvious missing commands in my and your list. But it is 
okay, lets focus on the mechanism first them work on the list content.

>   def reposetup(ui, repo):
>
>       class obsinhibitedrepo(repo.__class__):
> @@ -62,7 +66,20 @@
>       repo.__class__ = obsinhibitedrepo
>       repo._explicitaccess = set()
>
> -
> +def setupdirectaccess():
> +    """ Add two new filtername that behave like visible to provide direct access
> +    and direct access with warning. Wraps the commands to setup direct access """
> +    visiblefilter = repoview.computehidden
> +    repoview.filtertable.update({'visible-directaccess-nowarn': visiblefilter})
> +    for cmd in directaccessnowarning:
> +        extensions.wrapcommand(commands.table, cmd, wrapwithoutwarning)
> +        evolve = extensions.find("evolve")
> +    for cmd in directaccessnowarningevolve:
> +        try:
> +            extensions.wrapcommand(evolve.cmdtable, cmd, wrapwithoutwarning)
> +        except error.UnknownCommand:
> +            # Command not loaded, in that case we don't need to override it
> +            pass
>   def _update(orig, ui, repo, *args, **kwargs):
>       """
>       When moving to a commit we want to inhibit any obsolete commit affecting
> @@ -192,6 +209,14 @@
>       transaction.addpostclose('inhibitposttransaction', inhibitposttransaction)
>       return transaction
>
> +def wrapwithoutwarning(orig, *args, **kwargs):
> +    ui, repo, = args[0], args[1]
> +    if repo and repo.filtername == 'visible':
> +        object.__setattr__(repo, 'filtername',
> +            'visible-directaccess-nowarn')
> +    k = orig(*args, **kwargs)
> +    return k

eeerrrk, We should not change the filtername in flight.

use repo = repo.filtered('visible-directaccess-nowarn')


> +
>   def extsetup(ui):
>       # lets wrap the computation of the obsolete set
>       # We apply inhibition there
> @@ -214,6 +239,7 @@
>       extensions.wrapfunction(obsolete, 'createmarkers', _createmarkers)
>       extensions.wrapfunction(repoview, '_getdynamicblockers', _accessvisible)
>       extensions.wrapfunction(revset, 'posttreebuilthook', _posttreebuilthook)
> +    setupdirectaccess()
>       # wrap update to make sure that no obsolete commit is visible after an
>       # update
>       extensions.wrapcommand(commands.table, 'update', _update)
> @@ -250,8 +276,10 @@
>       # We extract the symbols that look like hashes and add them to the
>       # explicitaccess set
>       orig(tree, repo)
> -    if repo is not None and repo.filtername == 'visible':
> +    if repo and repo.filtername and \
> +        repo.filtername.startswith('visible'):

1) if you want to check against None, explicitly check "is not None".
2) we also only want allow unfiltered access for filtername that starts 
with 'visible-directaccess' to ensure we can have stuff like push and 
pull still requires --hidden
3) do not use \ for line continuation it is hard to read and error 
prone. use either parentheses or temporary variable.

>           prelength = len(repo._explicitaccess)
> +        accessbefore = set(repo._explicitaccess)
>           repo.symbols = gethashsymbols(tree)
>           cl = repo.unfiltered().changelog
>           for node in repo.symbols:
> @@ -264,6 +292,12 @@
>                   if rev not in repo.changelog:
>                       repo._explicitaccess.add(rev)
>           if prelength != len(repo._explicitaccess):
> +            if repo.filtername != 'visible-directaccess-nowarn':
> +                unhiddencommits = repo._explicitaccess - accessbefore
> +                repo.ui.warn( _("Warning: accessing hidden changesets %s "
> +                                "for write operation\n") %
> +                                (",".join([str(l) for l in unhiddencommits])))
> +

We should display hash because people used hash to refer to these commit

>               repo.invalidatevolatilesets()
>
>   @command('debugobsinhibit', [], '')
> diff --git a/tests/test-inhibit.t b/tests/test-inhibit.t
> --- a/tests/test-inhibit.t
> +++ b/tests/test-inhibit.t
> @@ -397,6 +397,9 @@
>     (use --hidden to access hidden revisions)
>     [255]
>     $ hg rebase -r ad78ff7d621f -r 53a94305e133 -d  2db36d8066ff
> +  Warning: accessing hidden changesets 3 for write operation
> +  Warning: accessing hidden changesets 10 for write operation
> +  Warning: accessing hidden changesets 11 for write operation

(We should display hash because people used hash to refer to these commit)
Laurent Charignon - May 12, 2015, 7:46 p.m.
On 5/12/15, 12:34 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:

>
>
>On 05/11/2015 04:56 PM, Laurent Charignon wrote:
>> # HG changeset patch
>> # User Laurent Charignon <lcharignon@fb.com>
>> # Date 1428086461 25200
>> #      Fri Apr 03 11:41:01 2015 -0700
>> # Node ID 23a0b73d771fe6b675e52f000ffb47768c4ad474
>> # Parent  5e82d78f5872c9503d7b6691c594a13794a9b4a4
>> inhibit: direct access with and without warning on a per command basis
>>
>> We introduce a new filtername visibile-directaccess-nowarn to enable
>>direct
>> access with no warning on a per command basis.
>> The motivation behing this change is to display warning when attempting
>>direct
>> access in destructive commands.
>>
>> diff --git a/hgext/inhibit.py b/hgext/inhibit.py
>> --- a/hgext/inhibit.py
>> +++ b/hgext/inhibit.py
>> @@ -37,6 +37,10 @@
>>   cmdtable = {}
>>   command = cmdutil.command(cmdtable)
>>
>> +# List of commands where no warning is shown for direct access
>> +directaccessnowarning = ['update', 'export']
>> +directaccessnowarningevolve = ['prune']
>
>We will need to do that for an arbitrary number of extensions.
>
>I suggest the following format:
>
>directaccesslevel = [
>     (False, None. 'update'), # no warning, core command, command name
>     (True,  'rebase'. 'rebase'), # warning, rebase extensions, command
>name
>]

Ok, looks good
>
>remember we have three level:
>
>- no direct access (suggestion: not in the list)
>- direct access with warning (suggestion: first item to True)
>- direct access without warning (sugguestion: first item to False)

You suggested a week ago to change it to two levels:
- direct access without-warning applied to a list of commands "whitelisted"
- visible (stays as is but provides direct access with warning)

Do you want to go back to the three levels of the previous patch?

>
>
>The current suggestion is conservative, we can changes the behavior once
>we have all in place.
>
>There is also obvious missing commands in my and your list. But it is
>okay, lets focus on the mechanism first them work on the list content.
>
>>   def reposetup(ui, repo):
>>
>>       class obsinhibitedrepo(repo.__class__):
>> @@ -62,7 +66,20 @@
>>       repo.__class__ = obsinhibitedrepo
>>       repo._explicitaccess = set()
>>
>> -
>> +def setupdirectaccess():
>> +    """ Add two new filtername that behave like visible to provide
>>direct access
>> +    and direct access with warning. Wraps the commands to setup direct
>>access """
>> +    visiblefilter = repoview.computehidden
>> +    repoview.filtertable.update({'visible-directaccess-nowarn':
>>visiblefilter})
>> +    for cmd in directaccessnowarning:
>> +        extensions.wrapcommand(commands.table, cmd, wrapwithoutwarning)
>> +        evolve = extensions.find("evolve")
>> +    for cmd in directaccessnowarningevolve:
>> +        try:
>> +            extensions.wrapcommand(evolve.cmdtable, cmd,
>>wrapwithoutwarning)
>> +        except error.UnknownCommand:
>> +            # Command not loaded, in that case we don't need to
>>override it
>> +            pass
>>   def _update(orig, ui, repo, *args, **kwargs):
>>       """
>>       When moving to a commit we want to inhibit any obsolete commit
>>affecting
>> @@ -192,6 +209,14 @@
>>       transaction.addpostclose('inhibitposttransaction',
>>inhibitposttransaction)
>>       return transaction
>>
>> +def wrapwithoutwarning(orig, *args, **kwargs):
>> +    ui, repo, = args[0], args[1]
>> +    if repo and repo.filtername == 'visible':
>> +        object.__setattr__(repo, 'filtername',
>> +            'visible-directaccess-nowarn')
>> +    k = orig(*args, **kwargs)
>> +    return k
>
>eeerrrk, We should not change the filtername in flight.
>
>use repo = repo.filtered('visible-directaccess-nowarn')

Ok
>
>
>> +
>>   def extsetup(ui):
>>       # lets wrap the computation of the obsolete set
>>       # We apply inhibition there
>> @@ -214,6 +239,7 @@
>>       extensions.wrapfunction(obsolete, 'createmarkers', _createmarkers)
>>       extensions.wrapfunction(repoview, '_getdynamicblockers',
>>_accessvisible)
>>       extensions.wrapfunction(revset, 'posttreebuilthook',
>>_posttreebuilthook)
>> +    setupdirectaccess()
>>       # wrap update to make sure that no obsolete commit is visible
>>after an
>>       # update
>>       extensions.wrapcommand(commands.table, 'update', _update)
>> @@ -250,8 +276,10 @@
>>       # We extract the symbols that look like hashes and add them to the
>>       # explicitaccess set
>>       orig(tree, repo)
>> -    if repo is not None and repo.filtername == 'visible':
>> +    if repo and repo.filtername and \
>> +        repo.filtername.startswith('visible'):
>
>1) if you want to check against None, explicitly check "is not None".
>2) we also only want allow unfiltered access for filtername that starts
>with 'visible-directaccess' to ensure we can have stuff like push and
>pull still requires --hidden
>3) do not use \ for line continuation it is hard to read and error
>prone. use either parentheses or temporary variable.
>
>>           prelength = len(repo._explicitaccess)
>> +        accessbefore = set(repo._explicitaccess)
>>           repo.symbols = gethashsymbols(tree)
>>           cl = repo.unfiltered().changelog
>>           for node in repo.symbols:
>> @@ -264,6 +292,12 @@
>>                   if rev not in repo.changelog:
>>                       repo._explicitaccess.add(rev)
>>           if prelength != len(repo._explicitaccess):
>> +            if repo.filtername != 'visible-directaccess-nowarn':
>> +                unhiddencommits = repo._explicitaccess - accessbefore
>> +                repo.ui.warn( _("Warning: accessing hidden changesets
>>%s "
>> +                                "for write operation\n") %
>> +                                (",".join([str(l) for l in
>>unhiddencommits])))
>> +
>
>We should display hash because people used hash to refer to these commit

See V2 sent right after
>
>>               repo.invalidatevolatilesets()
>>
>>   @command('debugobsinhibit', [], '')
>> diff --git a/tests/test-inhibit.t b/tests/test-inhibit.t
>> --- a/tests/test-inhibit.t
>> +++ b/tests/test-inhibit.t
>> @@ -397,6 +397,9 @@
>>     (use --hidden to access hidden revisions)
>>     [255]
>>     $ hg rebase -r ad78ff7d621f -r 53a94305e133 -d  2db36d8066ff
>> +  Warning: accessing hidden changesets 3 for write operation
>> +  Warning: accessing hidden changesets 10 for write operation
>> +  Warning: accessing hidden changesets 11 for write operation
>
>(We should display hash because people used hash to refer to these commit)

idem
>
>-- 
>Pierre-Yves David

Patch

diff --git a/hgext/inhibit.py b/hgext/inhibit.py
--- a/hgext/inhibit.py
+++ b/hgext/inhibit.py
@@ -37,6 +37,10 @@ 
 cmdtable = {}
 command = cmdutil.command(cmdtable)
 
+# List of commands where no warning is shown for direct access
+directaccessnowarning = ['update', 'export']
+directaccessnowarningevolve = ['prune']
+
 def reposetup(ui, repo):
 
     class obsinhibitedrepo(repo.__class__):
@@ -62,7 +66,20 @@ 
     repo.__class__ = obsinhibitedrepo
     repo._explicitaccess = set()
 
-
+def setupdirectaccess():
+    """ Add two new filtername that behave like visible to provide direct access
+    and direct access with warning. Wraps the commands to setup direct access """
+    visiblefilter = repoview.computehidden
+    repoview.filtertable.update({'visible-directaccess-nowarn': visiblefilter})
+    for cmd in directaccessnowarning:
+        extensions.wrapcommand(commands.table, cmd, wrapwithoutwarning)
+        evolve = extensions.find("evolve")
+    for cmd in directaccessnowarningevolve:
+        try:
+            extensions.wrapcommand(evolve.cmdtable, cmd, wrapwithoutwarning)
+        except error.UnknownCommand:
+            # Command not loaded, in that case we don't need to override it
+            pass
 def _update(orig, ui, repo, *args, **kwargs):
     """
     When moving to a commit we want to inhibit any obsolete commit affecting
@@ -192,6 +209,14 @@ 
     transaction.addpostclose('inhibitposttransaction', inhibitposttransaction)
     return transaction
 
+def wrapwithoutwarning(orig, *args, **kwargs):
+    ui, repo, = args[0], args[1]
+    if repo and repo.filtername == 'visible':
+        object.__setattr__(repo, 'filtername',
+            'visible-directaccess-nowarn')
+    k = orig(*args, **kwargs)
+    return k
+
 def extsetup(ui):
     # lets wrap the computation of the obsolete set
     # We apply inhibition there
@@ -214,6 +239,7 @@ 
     extensions.wrapfunction(obsolete, 'createmarkers', _createmarkers)
     extensions.wrapfunction(repoview, '_getdynamicblockers', _accessvisible)
     extensions.wrapfunction(revset, 'posttreebuilthook', _posttreebuilthook)
+    setupdirectaccess()
     # wrap update to make sure that no obsolete commit is visible after an
     # update
     extensions.wrapcommand(commands.table, 'update', _update)
@@ -250,8 +276,10 @@ 
     # We extract the symbols that look like hashes and add them to the
     # explicitaccess set
     orig(tree, repo)
-    if repo is not None and repo.filtername == 'visible':
+    if repo and repo.filtername and \
+        repo.filtername.startswith('visible'):
         prelength = len(repo._explicitaccess)
+        accessbefore = set(repo._explicitaccess)
         repo.symbols = gethashsymbols(tree)
         cl = repo.unfiltered().changelog
         for node in repo.symbols:
@@ -264,6 +292,12 @@ 
                 if rev not in repo.changelog:
                     repo._explicitaccess.add(rev)
         if prelength != len(repo._explicitaccess):
+            if repo.filtername != 'visible-directaccess-nowarn':
+                unhiddencommits = repo._explicitaccess - accessbefore
+                repo.ui.warn( _("Warning: accessing hidden changesets %s " 
+                                "for write operation\n") % 
+                                (",".join([str(l) for l in unhiddencommits])))
+
             repo.invalidatevolatilesets()
 
 @command('debugobsinhibit', [], '')
diff --git a/tests/test-inhibit.t b/tests/test-inhibit.t
--- a/tests/test-inhibit.t
+++ b/tests/test-inhibit.t
@@ -397,6 +397,9 @@ 
   (use --hidden to access hidden revisions)
   [255]
   $ hg rebase -r ad78ff7d621f -r 53a94305e133 -d  2db36d8066ff
+  Warning: accessing hidden changesets 3 for write operation
+  Warning: accessing hidden changesets 10 for write operation
+  Warning: accessing hidden changesets 11 for write operation
   rebasing 10:ad78ff7d621f "add cK"
   rebasing 11:53a94305e133 "add cL"
   $ hg log -G