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

login
register
mail settings
Submitter phabricator
Date Jan. 13, 2019, 10:45 p.m.
Message ID <5f311bcabfce71e47729f24d5b252408@localhost.localdomain>
Download mbox | patch
Permalink /patch/37715/
State Not Applicable
Headers show

Comments

phabricator - Jan. 13, 2019, 10:45 p.m.
angel.ezquerra updated this revision to Diff 13204.
angel.ezquerra edited the summary of this revision.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D5495?vs=13017&id=13204

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

AFFECTED FILES
  mercurial/revset.py

CHANGE DETAILS




To: angel.ezquerra, #hg-reviewers
Cc: pulkit, yuja, mercurial-devel
Yuya Nishihara - Jan. 14, 2019, 3:09 a.m.
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.

> -@predicate('merge()', safe=True)
> +@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.
phabricator - Jan. 14, 2019, 3:11 a.m.
yuja added a comment.


  Generally looks good.
  
  Can you fix a couple of nits? And if possible, fold the tests from https://phab.mercurial-scm.org/D5577
  into this and the next patch. We prefer including relevant test in each
  commit.
  
  > -@predicate('merge()', safe=True)
  >  +@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"). """
  >   1. 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.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -1249,15 +1249,41 @@ 
         pass
     return baseset(datarepr=('<max %r, %r>', subset, os))
 
-@predicate('merge()', safe=True)
+@predicate('merge(withbranch)', safe=True)
 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)
     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)
+        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()))
+    else:
+        # matchfn is a function that returns true when a revision is a merge
+        matchfn = lambda r: cl.parentrevs(r)[1] != -1
+    return subset.filter(matchfn, condrepr='<merge>')
 
 @predicate('branchpoint()', safe=True)
 def branchpoint(repo, subset, x):