Patchwork [3,of,7,V2] cmdutil: convert _revertprefetch() to a generic stored file hook (API)

# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1517771668 18000
#      Sun Feb 04 14:14:28 2018 -0500
# Node ID ab23d9644edaf62b2c3927b735d2170fc76ca711
# Parent  94d427f881cfca5cae792c5eac4bf00e942106ec
cmdutil: convert _revertprefetch() to a generic stored file hook (API)

This will be used by LFS to fetch required files in a group for multiple
commands, prior to being accessed.  That avoids the one-at-a-time fetch when the
filelog wrapper goes to access it, and it is missing locally (which costs two
round trips to the server.)  The core command list that needs this is probably
at least:

  - annotate
  - archive (which is also used by extdiff)
  - cat
  - diff
  - export
  - grep
  - verify (sadly)
  - anything that has the '{data}' template

There are no core users of the revert prefetch hook, and never have been since
it was introduced in 45e02cfad4bd for remotefilelog.  Thanks to Yuya for
figuring out a way to reliably trigger the deprecated warning.  Unfortunately,
it wanted to blame the caller of revert.  Passing along an adjusted stack level
seemed the least bad choice (although it still blames a core function).

One thing to note is that the store lock isn't being held when this is called.
I'm not at all familiar with remotefilelog or its locking requirements, so this
may not be a big deal.  Currently, LFS doesn't hold a lock when downloading
files.  Even though largefiles doesn't either, I'm starting to think it should,
and maybe the .hg/store/lock isn't good enough to cover the globally shared
cache.

.. api::

   The cmdutil._revertprefetch() hook point for prefetching stored files has
   been replaced by the command agnostic cmdutil._prefetchfiles().  The new
   function takes a list of files, instead of a list of lists of files.

On Tue, 06 Feb 2018 00:29:06 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1517771668 18000
> #      Sun Feb 04 14:14:28 2018 -0500
> # Node ID ab23d9644edaf62b2c3927b735d2170fc76ca711
> # Parent  94d427f881cfca5cae792c5eac4bf00e942106ec
> cmdutil: convert _revertprefetch() to a generic stored file hook (API)

> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -2862,7 +2862,14 @@
>  
>          if not opts.get('dry_run'):
>              needdata = ('revert', 'add', 'undelete')
> -            _revertprefetch(repo, ctx, *[actions[name][0] for name in needdata])
> +            if _revertprefetch is not _revertprefetchstub:
> +                ui.deprecwarn("'cmdutil._revertprefetch' is deprecated, use "
> +                              "'cmdutil._prefetchfiles'", '4.6', stacklevel=1)
> +                _revertprefetch(repo, ctx,
> +                                *[actions[name][0] for name in needdata])
> +            oplist = [actions[name][0] for name in needdata]
> +            _prefetchfiles(repo, ctx,
> +                           [f for sublist in oplist for f in sublist])
>              _performrevert(repo, parents, ctx, actions, interactive, tobackup)
>  
>          if targetsubs:
> @@ -2875,8 +2882,15 @@
>                      raise error.Abort("subrepository '%s' does not exist in %s!"
>                                        % (sub, short(ctx.node())))
>  
> -def _revertprefetch(repo, ctx, *files):
> -    """Let extension changing the storage layer prefetch content"""
> +def _revertprefetchstub():

Restored the original arguments in case it's called by wrapper.

> +    """Stub method for detecting extension wrapping of _revertprefetch(), to
> +    issue a deprecation warning."""
> +
> +_revertprefetch = _revertprefetchstub
> +
> +def _prefetchfiles(repo, ctx, files):
> +    """Let extensions changing the storage layer prefetch content for any non
> +    merge based command."""

Random ideas:

 - prefetchfiles() could be hosted by mergemod so lfs won't need to wrap
   applyupdates().
 - it could be a list of callback functions, instead of carefully wrapping
   the function itself
 - should be a public function as archival.py depends on it for example?

> On Feb 6, 2018, at 7:20 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Tue, 06 Feb 2018 00:29:06 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1517771668 18000
>> #      Sun Feb 04 14:14:28 2018 -0500
>> # Node ID ab23d9644edaf62b2c3927b735d2170fc76ca711
>> # Parent  94d427f881cfca5cae792c5eac4bf00e942106ec
>> cmdutil: convert _revertprefetch() to a generic stored file hook (API)
>> 
>> +    """Stub method for detecting extension wrapping of _revertprefetch(), to
>> +    issue a deprecation warning."""
>> +
>> +_revertprefetch = _revertprefetchstub
>> +
>> +def _prefetchfiles(repo, ctx, files):
>> +    """Let extensions changing the storage layer prefetch content for any non
>> +    merge based command."""
> 
> Random ideas:
> 
> - prefetchfiles() could be hosted by mergemod so lfs won't need to wrap
>   applyupdates().

Would it be better to keep it in cmdutil, and call it  just before calling applyupdates()?  Calling mergemod.prefetchfiles() from archive, for example, seems weird, doesn’t it?  Or am I misunderstanding?

> - it could be a list of callback functions, instead of carefully wrapping
>   the function itself

Just a raw list that everything can access, or should it have add/remove methods too?  I’ve seen both patterns with hook registration, IIRC.

> - should be a public function as archival.py depends on it for example?

Good idea

On Tue, 6 Feb 2018 08:02:30 -0500, Matt Harbison wrote:
> > On Feb 6, 2018, at 7:20 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > - prefetchfiles() could be hosted by mergemod so lfs won't need to wrap
> >   applyupdates().
> 
> Would it be better to keep it in cmdutil, and call it  just before calling applyupdates()?

That wouldn't be possible because the actions are calculated in merge.py.

> Calling mergemod.prefetchfiles() from archive, for example, seems weird, doesn’t it?  Or am I misunderstanding?

Another option would be repo.prefetchfiles(), but I'm not sure if it's a
good idea.

> > - it could be a list of callback functions, instead of carefully wrapping
> >   the function itself
> 
> Just a raw list that everything can access, or should it have add/remove methods too?  I’ve seen both patterns with hook registration, IIRC.

I have no preference.