[PATCH 2 of 2] lfs: migrate most file filtering from threshold to custom filter

[Boris Feld]

On Tue, 2018-01-16 at 08:05 -0500, Matt Harbison wrote:
> On Jan 16, 2018, at 5:26 AM, Boris Feld <boris.feld at octobus.net>
> wrote:
> 
> > On Sun, 2018-01-14 at 17:26 +0900, Yuya Nishihara wrote:
> > > On Sun, 14 Jan 2018 00:22:05 -0500, Matt Harbison wrote:
> > > > On Sat, 13 Jan 2018 21:11:45 -0500, Yuya Nishihara <yuya at tcha.o
> > > > rg>
> > > > wrote:
> > > > 
> > > > > On Sat, 13 Jan 2018 02:23:16 -0500, Matt Harbison wrote:
> > > > > > # HG changeset patch
> > > > > > # User Matt Harbison <matt_harbison at yahoo.com>
> > > > > > # Date 1514706889 18000
> > > > > > #      Sun Dec 31 02:54:49 2017 -0500
> > > > > > # Node ID 66976f55793cced57929dedc8993204be340c717
> > > > > > # Parent  868cc63bfe9d7d7f5b40bc8cd70175cf1a608a95
> > > > > > lfs: migrate most file filtering from threshold to custom
> > > > > > filter
> > > > > 
> > > > > Queued, thanks.
> > > > > 
> > > > > > -    threshold = repo.ui.configbytes('lfs', 'threshold')
> > > > > > +    trackspec = repo.ui.config('lfs', 'track')
> > > > > > 
> > > > > > -    repo.svfs.options['lfsthreshold'] = threshold
> > > > > > +    # deprecated config: lfs.threshold
> > > > > > +    threshold = repo.ui.configbytes('lfs', 'threshold')
> > > > > > +    if threshold:
> > > > > > +        trackspec = "(%s) | size('>%s')" % (trackspec,
> > > > > > threshold)
> > > > > 
> > > > > I've added fileset.parse(trackspec) to detect bad spec like
> > > > > '..)
> > > > > ..  
> > > > > (..', and
> > > > > s/%s/%d/ to make sure threshold is integer so no need of
> > > > > string
> > > > > escape.
> > > > 
> > > > Thanks.
> > > > 
> > > > I assume that your amend obsoleted something local only to your
> > > > system?   
> > > > The reason I ask is because I ran `hg extdiff --patch --hidden
> > > > -r  
> > > > 'precursors(c780e06)' -r c780e06`, and it aborted with an empty
> > > > revision.   
> > > > It's the precursors() predicate coming up empty.  I ended up
> > > > using  
> > > > 'last(allprecursors(c780e06))', and that found what I
> > > > submitted.  But that  
> > > > seems inconsistent with the obsolete line of that logged
> > > > revision
> > > > pointing  
> > > > directly to c780e06.
> > > 
> > > What I generally do is:
> > > 
> > >  hg import --obsolete mails  # yours->mine
> > >  hg amend                    # mine->committed
> > > 
> > > > At minimum, it would be nice to have a shorter way to spell
> > > > that.
> > > 
> > > I think allprecursors() should be renamed to precursors(), and
> > > delete/rename
> > > the original precursors().
> > > 
> > > > But it
> > > > seems weird that the log predicate would care about revisions
> > > > not
> > > > in the  
> > > > repo.  Boris- it looks like '{precursors}' will skip the
> > > > obsolete
> > > > revision  
> > > > not on my system, and agrees with 'last(..)'.
> > > > 
> > 
> > I checked the precursors revset and the behavior is not the one
> > expected, thank you for pointing that.
> > 
> > 
> > I also realized that the revset is in Evolve and not in Core, which
> > makes it a good opportunity to upstream it.
> > 
> > I'm not sure if we should keep only the equivalent of
> > allprecursors(),
> > what about having a predecessor() and allpredecessors() revsets.
> > 
> > predecessors() would returns the closests locally known
> > predecessors,
> > like the {predecessors} template keyword while allpredecessors()
> > would
> > returns all the locally known predecessors.
> > 
> > What do you think?
> 
> I think a dedicated revset to find the first generation, for lack of
> a better word, of predecessors is useful.  I use it all the time to
> ensure rebase/evolve conflict resolution didn’t drop anything, though
> it only works if it wasn’t split or folded.

I think the term we used is closest predecessor, at least we have a
function named closestpredecessors in obsutil.py.

> There was a proposal for short handing something like this, so maybe
> a dedicated predicate won’t be needed in the future.  But it would be
> nice if the template and revset agreed, and I don’t see an “all”
> template as being as useful as the current one.
> 
> 
> http://mercurial.markmail.org/thread/sjnnwa43s4eksu62
> 

I was not aware of this proposal. I think a dedicated predicate would
be usefull too to have more verbose and more readable templates.

When you are speaking about an "all" tempalte, do you means that you
don't think an "allpredecessors" template would be usefull in core?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20180116/b8505f38/attachment.html>