D4283: webcommands: fix `@webcommand` decorator

[foozy (Katsunori FUJIWARA)]

foozy added a comment.


  Sorry, I overlooked delivery error of reply to phabricator.
  
  This is re-sending of the reply at Sat, 18 Aug 2018 15:18:10 +0900,
  which was cc-ed to mercurial-devel at mercurial-scm.org.
  
  At Wed, 15 Aug 2018 19:00:51 +0000,
  sheehan (Connor Sheehan) wrote:
  [snip]
  
  > diff --git a/mercurial/hgweb/hgweb_mod.py b/mercurial/hgweb/hgweb_mod.py
  > 
  >   - a/mercurial/hgweb/hgweb_mod.py +++ b/mercurial/hgweb/hgweb_mod.py @@ -354,7 +354,7 @@ cmd = cmd[style + 1:]
  >     1. avoid accepting e.g. style parameter as command
  > - if util.safehasattr(webcommands, cmd): +            if cmd in webcommands.commands: req.qsparams['cmd'] = cmd
  > 
  >   if cmd == 'static': @@ -416,15 +416,15 @@
  > 
  >   res.headers['ETag'] = tag
  > - if cmd not in webcommands.__all__: +            if cmd not in webcommands.commands: msg = 'no such method: %s' % cmd raise ErrorResponse(HTTP_BAD_REQUEST, msg) else:
  >   1. Set some globals appropriate for web handlers. Commands can
  >   2. override easily enough. res.status = '200 Script output follows' res.headers['Content-Type'] = ctype
  > - return getattr(webcommands, cmd)(rctx) +                return webcommands.commands[cmd](rctx)
  > 
  >   except (error.LookupError, error.RepoLookupError) as err: msg = pycompat.bytestr(err)
  
  IMHO, for backward compatibility, this kind of (API) change should
  have deprecation period before stopping support, and should use
  ui.deprecwan() while such period, like below (this implementation
  assumes introduction of new decorator registrar.webcommand).
  
    def _getwebcommand(ui, name):
        cmd = webcommands.commands.get(name, None)
        if not cmd:
            if (name in webcommands.__all__ and
                util.safehasattr(webcommands, name)):
                cmd = getattr(webcommands, name)
                # store into webcommands.commands for subsequent access
                webcommands.commands[name] = cmd
    
                ui.deprecwarn('legacy registration for webcommand %s,'
                              ' use registrar.webcommand decorator' % name,
                              '5.0')
        return cmd
  
  > diff --git a/hgext/largefiles/uisetup.py b/hgext/largefiles/uisetup.py
  > 
  >   - a/hgext/largefiles/uisetup.py +++ b/hgext/largefiles/uisetup.py @@ -151,7 +151,7 @@ extensions.wrapfunction(archival, 'archive', overrides.overridearchive) extensions.wrapfunction(subrepo.hgsubrepo, 'archive', overrides.hgsubrepoarchive)
  > - extensions.wrapfunction(webcommands, 'archive', overrides.hgwebarchive) +    webcommands.wrapwebcommand('archive', overrides.hgwebarchive) extensions.wrapfunction(cmdutil, 'bailifchanged', overrides.overridebailifchanged)
  
  After changes above, direct invocation of original webcommand function
  'webcommands.archive()' from another code path avoids wrapping effect,
  because 'wrapwebcommand' wraps only the function stored in dispatch
  table 'webcommands.command'.
  
  Not for webcommand wrapping example, but for normal command wrapping
  example, largefiles extension wraps both 'rebase' command and 'rebase'
  function. In largefiles/uisetup.py::
  
    if name == 'rebase':
        extensions.wrapcommand(getattr(module, 'cmdtable'), 'rebase',
            overrides.overriderebase)
        extensions.wrapfunction(module, 'rebase',
                                overrides.overriderebase)
  
  Of course, it will be rare that un-wrapped original function causes
  problem in webcommand case. But "wrap function in dispatch table"
  should be separated from "stop wrapping original webcommand function"
  at first, IMHO.
  
  I'm also working around webcommand registration, and will post patches
  soon. But, of course, feel free to re-send your revised one, for
  feedback between us :-)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D4283

To: sheehan, #hg-reviewers
Cc: foozy, indygreg, mercurial-devel