D5496: revset: add "samebranch" keyword argument to the merge revset

[angel.ezquerra (Angel Ezquerra)]

angel.ezquerra added a comment.


  In https://phab.mercurial-scm.org/D5496#82394, @yuja wrote:
  
  > > - at predicate('merge(withbranch)', safe=True)
  > >  + at predicate('merge(withbranch, samebranch=True)', safe=True)
  >
  > `[, samebranch]` or [, samebranch=False]`.
  
  
  I guess that means:
  
  @predicate('merge([withbranch [, samebranch=None]])', safe=True)
  
  Right? (I realized that it is incorrect to say that samebranch's default value is False).
  
  >>   withbranch = ''
  >>   if 'withbranch' in args:
  >>       withbranch = getstring(args['withbranch'],
  >>                              _('withbranch argument must be a string'))
  >>       kind, branchname, branchmatcher = stringutil.stringmatcher(withbranch)
  >> 
  >> +    samebranch = None
  >>  +    if 'samebranch' in args:
  >>  +        # i18n: "samebranch" is a keyword
  >>  +        samebranch = getboolean(args['samebranch'],
  >>  +            _('samebranch argument must be a True or False'))
  >> 
  >>   cl = repo.changelog
  >>   # create the function that will be used to filter the subset
  >>   if withbranch:
  >>       # matchfn is a function that returns true when a revision
  >>       # is a merge and the second parent belongs to a branch that
  >>       # matches the withbranch pattern (which can be a literal or a regex)
  >>       if kind == 'literal':
  >> 
  >> - matchfn = lambda r: (cl.parentrevs(r)[1] != -1
  >> - and repo[r].p2().branch() == withbranch) +            basematchfn = lambda r: (cl.parentrevs(r)[1] != -1 +                                     and repo[r].p2().branch() == withbranch) else:
  >> - matchfn = lambda r: (cl.parentrevs(r)[1] != -1
  >> - and branchmatcher(repo[r].p2().branch()))
  >> - else:
  >> - # matchfn is a function that returns true when a revision is a merge
  >> - matchfn = lambda r: cl.parentrevs(r)[1] != -1 +            basematchfn = lambda r: (cl.parentrevs(r)[1] != -1 +                                     and branchmatcher(repo[r].p2().branch())) +    else: +        basematchfn = lambda r: cl.parentrevs(r)[1] != -1 +    if samebranch is None: +        matchfn = basematchfn +    else: +        # if samebranch was specified, build a new match function +        # that on top of basematch checks if the parents belong (or not) +        # to the same branch (depending on the value of samebranch) +        def matchfn(r): +            c = repo[r] +            if not basematchfn(r): +                return False +            issamebranchmerge = c.p1().branch() == c.p2().branch() +            return issamebranchmerge if samebranch else not issamebranchmerge
  > 
  > These conditions can be formed as followed:
  > 
  >   matchfns = [lambda r: cl.parentrevs(r)[1] != -1]
  >   if withbranch:
  >       matchfns.append(lambda r: branchmatcher(repo[r].p2().branch()))
  >   if samebranch:
  >       matchfns.append(samebranchmatchfn)
  >   
  >   if len(matchfns) == 1:
  >       # fast path for common case
  >       return subset.filter(matchfn[0], ...)
  >   else:
  >       return subset.filter(lambda r: all(p(r) for p in matchfn), ...)
  
  Do you think this makes the code simpler? In any case, if you think this approach is best I can do it, but perhaps it would be a little better to keep a single subset.filter call as follows:
  
    if len(matchfns) == 1:
        finalmatchfn = matchfns[0]
    else:
        finalmatchfn = lambda r: all(p(r) for p in matchfns)
    return subset.filter(finalmatchfn, condrepr='<merge>')
  
  What do you think?

REPOSITORY
  rHG Mercurial

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

To: angel.ezquerra, #hg-reviewers
Cc: yuja, mercurial-devel