D5495: revset: add "branch" positional arguments to the merge revset

Yuya Nishihara yuya at tcha.org
Sun Jan 13 22:09:29 EST 2019


Generally looks good.

Can you fix a couple of nits? And if possible, fold the tests from D5577
into this and the next patch. We prefer including relevant test in each
commit.

> - at predicate('merge()', safe=True)
> + at predicate('merge(withbranch)', safe=True)

`merge([withbranch])` as it is an optional parameter.

>  def merge(repo, subset, x):
> -    """Changeset is a merge changeset.
> +    """Changeset is a merge changeset
> +
> +    All merge revisions are returned by default. If a "withbranch"
> +    pattern is provided only merges with (i.e. whose second parent
> +    belongs to) those branches that match the pattern will be returned.
> +    The simplest pattern is the name of a single branch. It is also
> +    possible to specify a regular expression by starting the pattern
> +    with "re:". This can be used to match more than one branch
> +    (e.g. "re:branch1|branch2").
>      """
>      # i18n: "merge" is a keyword
> -    getargs(x, 0, 0, _("merge takes no arguments"))
> +    args = getargsdict(x, 'merge', 'withbranch')
> +    withbranch = ''
> +    if 'withbranch' in args:
> +        withbranch = getstring(args['withbranch'],
> +                               _('withbranch argument must be a string'))
> +        kind, branchname, branchmatcher = stringutil.stringmatcher(withbranch)

Can you merge this with the next `if withbranch:` block to reduce the number
of conditionally defined variables referenced later?

>      cl = repo.changelog
> -    return subset.filter(lambda r: cl.parentrevs(r)[1] != -1,
> -                         condrepr='<merge>')
> +    # 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)

Nit: these comments seem a bit verbose. It's documented in stringmatcher().

> +        if kind == 'literal':
> +            matchfn = 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()))

If we don't have anything special about the `literal` kind, we can always use
the `branchmatcher()`. `if kind == 'literal'` isn't needed.


More information about the Mercurial-devel mailing list