[PATCH] lfs: register the flag processors per repository

[Gregory Szorc]

On Thu, Oct 4, 2018 at 9:48 PM Matt Harbison <mharbison72 at gmail.com> wrote:

> On Thu, 04 Oct 2018 14:35:49 -0400, Gregory Szorc
> <gregory.szorc at gmail.com> wrote:
>
> > On Thu, Oct 4, 2018 at 6:09 AM Yuya Nishihara <yuya at tcha.org> wrote:
> >
> >> On Thu, 04 Oct 2018 00:39:12 -0400, Matt Harbison wrote:
> >> > # HG changeset patch
> >> > # User Matt Harbison <matt_harbison at yahoo.com>
> >> > # Date 1538626646 14400
> >> > #      Thu Oct 04 00:17:26 2018 -0400
> >> > # Node ID e010a7be6dc96ea7d48be81a7c5e8a8ed8bf6c31
> >> > # Parent  7a347d362a455d84bccf34347171d89724b9c9df
> >> > lfs: register the flag processors per repository
> >>
> >> > +    if b'lfs' in requirements:
> >> > +        options[b'enableextstored'] = True
> >> > +
> >> >      if repository.NARROW_REQUIREMENT in requirements:
> >> >          options[b'enableellipsis'] = True
> >> >
> >> > diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> >> > --- a/mercurial/revlog.py
> >> > +++ b/mercurial/revlog.py
> >> > @@ -110,6 +110,26 @@ parsers = policy.importmod(r'parsers')
> >> >      REVIDX_ISCENSORED: None,
> >> >  }
> >> >
> >> > +# Flag processors for REVIDX_EXTSTORED.
> >> > +def extstoredreadprocessor(rl, text):
> >> > +    raise error.ProgrammingError(b'extstored flags processor enabled
> >> without'
> >> > +                                 b' wrapping processor function')
> >> > +
> >> > +def extstoredwriteprocessor(rl, text):
> >> > +    raise error.ProgrammingError(b'extstored flags processor enabled
> >> without'
> >> > +                                 b' wrapping processor function')
> >> > +
> >> > +def extstoredrawprocessor(rl, text):
> >> > +    raise error.ProgrammingError(b'extstored flags processor enabled
> >> without'
> >> > +                                 b' wrapping processor function')
> >> > +
> >> > +# Lambdas are needed here so that these methods can be wrapped by
> >> lfs.
> >> > +extstoredprocessor = (
> >> > +    lambda rl, text: extstoredreadprocessor(rl, text),
> >> > +    lambda rl, text: extstoredwriteprocessor(rl, text),
> >> > +    lambda rl, text: extstoredrawprocessor(rl, text),
> >> > +)
> >> > +
> >> >  # Flag processors for REVIDX_ELLIPSIS.
> >> >  def ellipsisreadprocessor(rl, text):
> >> >      return text, False
> >> > @@ -405,6 +425,8 @@ class revlog(object):
> >> >                  self._srdensitythreshold =
> >> opts['sparse-read-density-threshold']
> >> >              if 'sparse-read-min-gap-size' in opts:
> >> >                  self._srmingapsize = opts['sparse-read-min-gap-size']
> >> > +            if opts.get('enableextstored'):
> >> > +                self._flagprocessors[REVIDX_EXTSTORED] =
> >> extstoredprocessor
> >>
> >> I feel it's awkward that we have to wrap the stub extstoredprocessor and
> >> pass in enableextstored=True.
> >>
> >> I don't have a concrete idea, but maybe we can provide an API to
> >> register
> >> a stock flagprocessor which won't be copied to the revlog instance by
> >> default.
> >>
> >
> > I also found this a bit awkward.
> >
> > I wrote up a patch a few weeks ago that added an "extraflagprocessors"
> > argument or some such to revlog.__init__ that would allow extra flag
> > processors to be passed in. I never submitted it because I had originally
> > wrote it for censoring and I solved that problem by moving the censor
> > flag
> > processor into revlog.py and introduced the "censorable=False" argument
> > instead.
> >
> > But the flag processor for LFS is in the same boat and we may want to do
> > something similar. But since LFS's flag processors require code in the
> > LFS
> > extension, we can't simply move the flag processors code to revlog.py.
> >
> > I think our best bet is to pass a flag processor into revlog.__init__ -
> > either via an argument or as a key in the store/opener options. Store
> > options is simpler, as we'd only need to teach revlog.py to add extra
> > flag
> > processors defined in a store options key and the LFS extension would
> > only
> > need to monkeypatch localrepo.resolverevlogstorevfsoptions(). Adding an
> > argument to revlog.__init__ requires a bit more work to thread through
> > everything. If the LFS flag processor code were in core, I'd be fine with
> > introducing that argument. But since it isn't, I think going through the
> > store options is the way to go.
>
> Should the revlog.addflagprocessor() method be copied to localrepo so
> that
> it still does the validation, and can also hide the fact that it's being
> tacked on to the store options?
>

Found this email when triaging my inbox.

I know some other flag processor changes landed after this. Are you
satisfied with the current state of things or do you think we should do
more?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20181031/73a3a6e8/attachment.html>