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

> `[, 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?

> 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#82908, @angel.ezquerra wrote:

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.

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)

Gentle ping on this series, the feature sounds interesting.