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

login
register
mail settings
Submitter phabricator
Date Jan. 6, 2019, 7:03 p.m.
Message ID <differential-rev-PHID-DREV-apsd3srhtueakqrqrgup-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/37496/
State New
Headers show

Comments

phabricator - Jan. 6, 2019, 7:03 p.m.
angel.ezquerra created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  By default all merges are shown but if "samebranch" is set to False then merges
  with the same branch (i.e. where both parents belong to the same branch) will
  be filtered out.
  
  Conversely, if "samebranch" is set to True then only merges with the same branch
  will be shown.
  
  This is useful to visualize at a high level the relationships between different
  branches and how they are merged with each other.
  
  With the addition of the merge(withbranch) idiom on a previous revision this
  could already be done in a quite complicated way, by doing something like:
  
  merge() and branch(somebranch) and not merge(somebranch)
  
  This is not very practical ano only works for a single branch. Thus this new
  option is added.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/revset.py

CHANGE DETAILS




To: angel.ezquerra, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 16, 2019, 12:02 a.m.
angel.ezquerra added a comment.


  In https://phab.mercurial-scm.org/D5496#82394, @yuja wrote:
  
  > > -@predicate('merge(withbranch)', safe=True)
  > >  +@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
Yuya Nishihara - Jan. 16, 2019, 12:58 p.m.
>   > `[, 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).

Okay, I didn't notice that. And it's tricky to map `samebranch=False` to
"different branch" constraint. I would read it as "I don't care whether
the branches are the same or not."

We can instead express it as `merge() - merge(samebranch=True)`.

>   >   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?

Yes. The original version was hard to find all possible call paths.
Separate function per constraint is easier to follow.

> 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>')

I don't care about these differences.
phabricator - Jan. 16, 2019, 1:10 p.m.
yuja added a comment.


  >   > `[, 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).
  
  Okay, I didn't notice that. And it's tricky to map `samebranch=False` to
  "different branch" constraint. I would read it as "I don't care whether
  the branches are the same or not."
  
  We can instead express it as `merge() - merge(samebranch=True)`.
  
  >   >   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?
  
  Yes. The original version was hard to find all possible call paths.
  Separate function per constraint is easier to follow.
  
  > 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>')
  
  I don't care about these differences.

REPOSITORY
  rHG Mercurial

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

To: angel.ezquerra, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - Jan. 16, 2019, 6:11 p.m.
angel.ezquerra added a comment.


  In https://phab.mercurial-scm.org/D5496#82671, @yuja wrote:
  
  > >   > `[, 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).
  >
  > Okay, I didn't notice that. And it's tricky to map `samebranch=False` to
  >  "different branch" constraint. I would read it as "I don't care whether
  >  the branches are the same or not."
  
  
  
  
  In https://phab.mercurial-scm.org/D5496#82671, @yuja wrote:
  
  > >   > `[, 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).
  >
  > Okay, I didn't notice that. And it's tricky to map `samebranch=False` to
  >  "different branch" constraint. I would read it as "I don't care whether
  >  the branches are the same or not."
  >
  > We can instead express it as `merge() - merge(samebranch=True)`.
  
  
  Do you mean that the flag should only indicate whether you want to hide the same branch merges? I guess that is OK too, since the main use case for this flag is to hide the merge from the same branch. However I think we should change the flag name then. Perhaps "hidesame"? Or "includesame" or "includeself", defaulting to True? Any ideas?

REPOSITORY
  rHG Mercurial

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

To: angel.ezquerra, #hg-reviewers
Cc: yuja, mercurial-devel
Yuya Nishihara - Jan. 17, 2019, 2:20 p.m.
>   > Okay, I didn't notice that. And it's tricky to map `samebranch=False` to
>   >  "different branch" constraint. I would read it as "I don't care whether
>   >  the branches are the same or not."
>   >
>   > We can instead express it as `merge() - merge(samebranch=True)`.
>   
>   Do you mean that the flag should only indicate whether you want to hide the same branch merges?

I just mean tri-state bool is confusing. `<whatever>=False` sounds like we
don't care about the `<whatever>` condition.

> I guess that is OK too, since the main use case for this flag is to hide the merge from the same branch. However I think we should change the flag name then. Perhaps "hidesame"? Or "includesame" or "includeself", defaulting to True? Any ideas?

It could be an argument taking a string like `'same'`, but I can't think
of nice names. What's the best term describing a merge between two named
branches?
phabricator - Jan. 17, 2019, 2:21 p.m.
yuja added a comment.


  >   > Okay, I didn't notice that. And it's tricky to map `samebranch=False` to
  >   >  "different branch" constraint. I would read it as "I don't care whether
  >   >  the branches are the same or not."
  >   >
  >   > We can instead express it as `merge() - merge(samebranch=True)`.
  >   
  >   Do you mean that the flag should only indicate whether you want to hide the same branch merges?
  
  I just mean tri-state bool is confusing. `<whatever>=False` sounds like we
  don't care about the `<whatever>` condition.
  
  > I guess that is OK too, since the main use case for this flag is to hide the merge from the same branch. However I think we should change the flag name then. Perhaps "hidesame"? Or "includesame" or "includeself", defaulting to True? Any ideas?
  
  It could be an argument taking a string like `'same'`, but I can't think
  of nice names. What's the best term describing a merge between two named
  branches?

REPOSITORY
  rHG Mercurial

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

To: angel.ezquerra, #hg-reviewers
Cc: yuja, mercurial-devel
Didly - Jan. 20, 2019, 11:14 a.m.
What about “selfmerge”?

El El jue, 17 ene 2019 a las 14:27, yuja (Yuya Nishihara) <
phabricator@mercurial-scm.org> escribió:

> yuja added a comment.
>
>
>   >   > Okay, I didn't notice that. And it's tricky to map
> `samebranch=False` to
>   >   >  "different branch" constraint. I would read it as "I don't care
> whether
>   >   >  the branches are the same or not."
>   >   >
>   >   > We can instead express it as `merge() - merge(samebranch=True)`.
>   >
>   >   Do you mean that the flag should only indicate whether you want to
> hide the same branch merges?
>
>   I just mean tri-state bool is confusing. `<whatever>=False` sounds like
> we
>   don't care about the `<whatever>` condition.
>
>   > I guess that is OK too, since the main use case for this flag is to
> hide the merge from the same branch. However I think we should change the
> flag name then. Perhaps "hidesame"? Or "includesame" or "includeself",
> defaulting to True? Any ideas?
>
>   It could be an argument taking a string like `'same'`, but I can't think
>   of nice names. What's the best term describing a merge between two named
>   branches?
>
> REPOSITORY
>   rHG Mercurial
>
> REVISION DETAIL
>   https://phab.mercurial-scm.org/D5496
>
> To: angel.ezquerra, #hg-reviewers
> Cc: yuja, mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - Jan. 22, 2019, 1:34 p.m.
> What about “selfmerge”?

IIUC, we need the opposite word to turn `samebranch=False` into
`<something>=True` or `<someattr>=<something>`.
Didly - Jan. 22, 2019, 1:59 p.m.
My suggestion is to have it default to True. That is, by default merge()
would include self merges, but it would be possible to hide them by using:

merge(selfmerge=False)

What do you think?

Angel

El El mar, 22 ene 2019 a las 14:34, Yuya Nishihara <yuya@tcha.org> escribió:

> > What about “selfmerge”?
>
> IIUC, we need the opposite word to turn `samebranch=False` into
> `<something>=True` or `<someattr>=<something>`.
>
phabricator - Jan. 22, 2019, 6:28 p.m.
mharbison72 added a comment.


  In https://phab.mercurial-scm.org/D5496#82908, @angel.ezquerra wrote:
  
  > In https://phab.mercurial-scm.org/D5496#82671, @yuja wrote:
  >
  > >
  >
  >
  > Do you mean that the flag should only indicate whether you want to hide the same branch merges? I guess that is OK too, since the main use case for this flag is to hide the merge from the same branch. However I think we should change the flag name then. Perhaps "hidesame"? Or "includesame" or "includeself", defaulting to True? Any ideas?
  
  
  Maybe `anonymous`, defaulting to True?  That's in the glossary under `Branch, anonymous`, so not technically a merge, but I think it still conveys the point.
  
  I think I have a similar reaction as Yuya, but in the opposite direction- `merge(anonymous=True)` makes me think that's all that's of interest.  So maybe `withanonymous`?
  
  I do like the compactness of:
  
    merge() => all merges
    merge(anonymous=True) => only merges with matching (p1, p2) branch names
    merge(anonymous=False) => only merges with different (p1, p2) branch names
  
  Otherwise finding only anonymous merges is something like `merge() - merge(anonymous=False)`, and it took some thinking to get there with the double negative.
  
  But I have no idea how well that applies to other things if we set that precedent here, and don't feel that strongly about it.

REPOSITORY
  rHG Mercurial

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

To: angel.ezquerra, #hg-reviewers
Cc: mharbison72, yuja, mercurial-devel
Yuya Nishihara - Jan. 23, 2019, 12:48 p.m.
>   Maybe `anonymous`, defaulting to True?

To all: the default can't be True. It would break the current `merge()`
revset behavior. Only viable choice is to set the default to "don't care".

And I think tri-state bool is confusing in this context. So, IMHO, it's
better to add an argument taking a keyword string specifying constraint
such as `merge(between="a-keyword-to-select-merges-of-named-branches")`.

> That's in the glossary under `Branch, anonymous`, so not technically
> a merge, but I think it still conveys the point.

I disagree. Merges of the same branch are pretty common if your team preferred
merge-based strategy. I wouldn't explicitly call new head pulled from public
repo as an anonymous head.

It can also be wrong if you're using bookmarks.
phabricator - Jan. 23, 2019, 12:51 p.m.
yuja added a comment.


  >   Maybe `anonymous`, defaulting to True?
  
  To all: the default can't be True. It would break the current `merge()`
  revset behavior. Only viable choice is to set the default to "don't care".
  
  And I think tri-state bool is confusing in this context. So, IMHO, it's
  better to add an argument taking a keyword string specifying constraint
  such as `merge(between="a-keyword-to-select-merges-of-named-branches")`.
  
  > That's in the glossary under `Branch, anonymous`, so not technically
  >  a merge, but I think it still conveys the point.
  
  I disagree. Merges of the same branch are pretty common if your team preferred
  merge-based strategy. I wouldn't explicitly call new head pulled from public
  repo as an anonymous head.
  
  It can also be wrong if you're using bookmarks.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -1249,32 +1249,54 @@ 
         pass
     return baseset(datarepr=('<max %r, %r>', subset, os))
 
-@predicate('merge(*withbranch)', safe=True)
+@predicate('merge(*withbranch, samebranch=True)', safe=True)
 def merge(repo, subset, x):
     """Changeset is a merge changeset
 
     All merge revisions are returned by default. If one or more "withbranch"
     names are provided only merges with those branches (i.e. whose
     second parent belongs to one of those branches) will be returned.
+
+    It is also possible to only return merges where both parents belong to
+    the same branch by specifying samebranch=True. If samebranch=False is
+    set then only merges where both parents do not belong to the same branch
+    will be returned.
     """
     # i18n: "merge" is a keyword
-    args = getargsdict(x, 'merge', '*withbranch')
+    args = getargsdict(x, 'merge', '*withbranch samebranch')
     withbranches = []
     if 'withbranch' in args:
         for el in args['withbranch']:
             # i18n: "withbranch" is a keyword
             withbranches.append(getstring(el,
                 _('withbranch argument must be a string')))
+    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
     if withbranches:
         # basematch is a function that returns true when a revision
         # is a merge and the second parent belongs to one of the
         # selected "merge with branches"
-        matches = lambda r: (cl.parentrevs(r)[1] != -1
-                             and repo[r].p2().branch() in withbranches)
+        basematch = lambda r: (cl.parentrevs(r)[1] != -1
+                               and repo[r].p2().branch() in withbranches)
     else:
         # basematch is a function that returns true when a revision is a merge
-        matches = lambda r: cl.parentrevs(r)[1] != -1
+        basematch = lambda r: cl.parentrevs(r)[1] != -1
+    if samebranch is None:
+        matches = basematch
+    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 matches(r):
+            c = repo[r]
+            if not basematch(r):
+                return False
+            issamebranchmerge = c.p1().branch() == c.p2().branch()
+            return issamebranchmerge if samebranch else not issamebranchmerge
     return subset.filter(matches, condrepr='<merge>')
 
 @predicate('branchpoint()', safe=True)