merges() vs merges(x)

[arialdomartini]

Most of the functions in the revset language take revsets as parameters.

I just wonder why merges() does not.

I acknowledge that merges(x) can be obtained with merges() & x. Yet I think that the following would probably fit more consistently with the rest of functions:

• merges(x): Merge commits in x
• merges(): alias of merges(all())

What do you think?

[arialdomartini]

For completeness, the list of functions taking revsets:

• parents(x, [depth])
• children(x, [depth])
• ancestors(x, [depth])
• descendants(x, [depth])
• first_parent(x, [depth])
• first_ancestors(x, [depth])
• reachable(srcs, domain)
• connected(x)
• heads(x)
• roots(x)
• latest(x, [count])
• fork_point(x)
• bisect(x)
• present(x)
• at_operation(op, x)
• coalesce(revsets...)

and the list of functions not taking revsets (kind of assuming all() as their argument):

• all()
• none()
• root()
• merges()
• git_refs()
• git_head()
• visible_heads()
• mine()
• signed()
• empty()
• working_copies()
• conflicts()

About the latter, the ones maybe deserving an optional revsets parameter could be:

• merges()
• mine()
• signed()

[jennings]

The functions that don't take arguments match inherent properties of each commit:

• merges() matches any commit with more than 1 parent.
• empty() matches any commit containing no changes.
• root() matches the virtual commit zzzzzzz and nothing else
• all() Is it a commit? It matches.

On the other hand, the functions that take a revset argument need that argument to calculate the result:

• heads(X) selects the heads in the revset X.
• parents(X) selects the parents of each commit in X.
• connected(X) selects the connected graph containing X.

heads(all()) & X is not the same as heads(X). If you have the following graph:

@  tqkywttx user-036a 1 second ago 66047618
│  (empty) (redacted)
○  kzkzsltu user-036a 2 seconds ago git_head() 170eb63b
│  (empty) (redacted)
○  lrrlsoyt user-036a 3 seconds ago edbcf24f
│  (empty) (redacted)

Then heads(lrr::kzk) contains only kzk, but heads(all()) & lrr::kzk is empty:

heads(all()) & lrr::kzk = heads(lrr | kzk | tqk) & lrr::kzk
                        = tqk & lrr::kzk
                        = none()

[martinvonz]

Perhaps it would have made sense to call the functions that just filter a commit as boolean methods, e.g. is_merge(), is_empty(), is_signed(), has_author(<pattern>) etc. I'm not sure if that would have been a good idea, and it may not be worth it now even if it was.

[arialdomartini]

to call the functions that just filter a commit as boolean methods

I really like this. It would make the intent crystal clear.

[jennings]

I recognize the pragmatism of that naming convention, but I dislike the aesthetic of it :)

I feel like it doesn't fit the set logic as well. merges() is the set of commits that are merges. is_merge() looks to me like it should be used like so: filtered(X, is_merge)

[arialdomartini]

Makes sense, but then you could not use the boolean operators & and | and you would need some combinators for composing is_merge and is_author, like Rust's predicates::prelude and and or. As much as I love FP, I would not recommend that for a larger audience, honestly.