This is an archive of the discontinued Mercurial Phabricator instance.

revset: add "samebranch" keyword argument to the merge revset
Needs RevisionPublic

Authored by angel.ezquerra on Jan 6 2019, 2:03 PM.

Details

Reviewers
baymax
Group Reviewers
hg-reviewers
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

angel.ezquerra created this revision.Jan 6 2019, 2:03 PM
yuja added a subscriber: yuja.Jan 13 2019, 10:11 PM

-@predicate('merge(withbranch)', safe=True)
+@predicate('merge(withbranch, samebranch=True)', safe=True)

[, samebranch] or [, samebranch=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), ...)
In 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?

yuja added a comment.Jan 16 2019, 8:10 AM
> `[, 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.

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

yuja added a comment.Jan 17 2019, 9:21 AM
> 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?

In 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.

yuja added a comment.Jan 23 2019, 7:51 AM
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.

Maybe we should not try to use boolean. we could have

merge(parentbranches="same")

with the possible value being: same, different, any (an possibly some constains about branch(p1(x)) == branch(x)

marmoute added a comment.EditedApr 22 2020, 12:00 PM

Gentle ping on this series, the feature sounds interesting.

baymax requested changes to this revision.Jul 31 2020, 1:56 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jul 31 2020, 1:56 PM