This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3715

namespaces: allow namespaces whose symbols resolve to many nodes (API)
AbandonedPublic

Authored by martinvonz on Jun 11 2018, 7:10 PM.
Tags
None
Subscribers
yuja, durin42, smf and 2 others

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Summary

The goal of this commit is to allow namespaces that support multiple
nodes per name to also have the name resolve to those multiple nodes
in the revset. For example, the "topics" namespace seems to be a
natural candidate for this (I don't know if the extension's maintainer
agrees). I view a topic as having multiple nodes and I would therefore
expect hg log -r my-topic to list all those nodes. Note that the
topics extension already supports multiple nodes per name, but we
don't expose that to the user in a consistent way (each namespace has
to define its own revset).

This commit adds an option to namespaces to indicate that `hg log -r
<name in the namespace>` and similar should resolve to the nodes that
the namespace says and not just the highest revnum among them.

Marked (API) because I repurposed singlenode() to nodes().

Note: I think branches should also resolve to multiple nodes (so
e.g. hg log -r stable lists all nodes on stable), but it's obviously
too late to change that now (and perhaps BC is the reason it even
behaves the way it does). I also realize that any namespace that were
to use the new mechanism would be inconsistent with how branches
work. I think the convenience and intuitiveness outweighs the cost of
that inconsistency.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Jun 11 2018, 7:10 PM
durin42 added subscribers: lothiraldan, smf, durin42.Jun 12 2018, 1:41 PM
durin42 accepted this revision as: durin42.

I'm in favor, but feel like I've got enough conflict of interest I shouldn't land the patches.

@smf @lothiraldan this might be of interest to both of you?

smf added a comment.Jun 12 2018, 8:48 PM

durin42 (Augie Fackler) <phabricator@mercurial-scm.org> writes:

durin42 added subscribers: lothiraldan, smf, durin42.
durin42 accepted this revision as: durin42.
durin42 added a comment.

I'm in favor, but feel like I've got enough conflict of interest I shouldn't land the patches.
@smf @lothiraldan this might be of interest to both of you?

Side note: I keep missing messages that I'm tagged in because I'm not
explicitly mentioned in a CC field. Is it possible to add a CC to each
person tagged in a message?

Side note2: Phabricator emails are really non-trivial to parse and
(worse!) search. The raw emails are not simple, raw text so I'm having
trouble tagging these for higher priority.

Thanks for alerting me of this series! I've had a discussion with Martin
about this on IRC but I'm a bit out of time today to respond (but
definitely do want to respond). (I'm going to try to spend some time in
the mornings to do my email triaging so I can get back on top of this
list.)

lothiraldan added a comment.Jun 13 2018, 11:59 AM
In D3715#58383, @durin42 wrote:

I'm in favor, but feel like I've got enough conflict of interest I shouldn't land the patches.
@smf @lothiraldan this might be of interest to both of you?

The proposition of making namespaces behave differently than branches, and potentially making two namespaces behave differently (one with multinode=True and the other with multinode=False) make my spider sense tickle. But I will take time to see the exact implications of that series.

martinvonz added a comment.Jun 13 2018, 12:37 PM
In D3715#58383, @durin42 wrote:

I'm in favor, but feel like I've got enough conflict of interest I shouldn't land the patches.
@smf @lothiraldan this might be of interest to both of you?

In D3715#58507, @lothiraldan wrote:
In D3715#58383, @durin42 wrote:

I'm in favor, but feel like I've got enough conflict of interest I shouldn't land the patches.
@smf @lothiraldan this might be of interest to both of you?

The proposition of making namespaces behave differently than branches, and potentially making two namespaces behave differently (one with multinode=True and the other with multinode=False) make my spider sense tickle. But I will take time to see the exact implications of that series.

As I said in the commit message, I think branches should have multinode=True, but we can't do that for BC reasons. Bookmarks and tags clearly identify a single commit per name (at least in my mind).

How about we introduce the multinode option and default it to False for now, but plan to change the default to True later and then let only branches have the old behavior (i.e. multinode=False) and discourage extensions from setting multinode=False? That would result in consistency between namespaces (except for the legacy case of branches) while giving existing extensions some time to adapt (perhaps by making namemap return at most one node if they view their names as identifying a single commit).

Btw, the topics extension was just an example. We have an internal extension that's the reason for this patch. I don't use topics, so I don't care too much if topics decide to have one commit per name or not (but if I did use topics, I think I would definitely view it as having multiple nodes per name).

smf added a comment.Jun 14 2018, 8:34 PM

Sean Farley <sean@farley.io> writes:

durin42 (Augie Fackler) <phabricator@mercurial-scm.org> writes:

durin42 added subscribers: lothiraldan, smf, durin42.
durin42 accepted this revision as: durin42.
durin42 added a comment.

I'm in favor, but feel like I've got enough conflict of interest I shouldn't land the patches.
@smf @lothiraldan this might be of interest to both of you?

Side note: I keep missing messages that I'm tagged in because I'm not
explicitly mentioned in a CC field. Is it possible to add a CC to each
person tagged in a message?
Side note2: Phabricator emails are really non-trivial to parse and
(worse!) search. The raw emails are not simple, raw text so I'm having
trouble tagging these for higher priority.

I think I got this ironed out now.

Thanks for alerting me of this series! I've had a discussion with Martin
about this on IRC but I'm a bit out of time today to respond (but
definitely do want to respond). (I'm going to try to spend some time in
the mornings to do my email triaging so I can get back on top of this
list.)

This patch strikes me as a Seems-Like-A-Good-Idea-But-Could-Blowup type
of thing. So, what this patch does is conditionally change the behavior
of 'log -r' based on the type of object passed in. In terms of revsets,
this means:

hg log -r 'default' -> hg log -r 'branch(default)'

I'm using branches to illustrate the argument in terms of core behavior
but will go back to topics further down.

But what about more complex revsets? 'default & draft()' will now mean
'branch(default) & draft()' which is something completely different. You
could argue that, "Oh, well, we'll only do that if the argument sent to
-r is equal to a single branch name." But what about users that expect
'-r NAME' to work like 'branch(NAME)' in a revset? I would expect they
learn a bad habit of 'default == branch(default)'. And what about a more
complex that will reduce down to 'default'? Will that be
'branch(default)' now?

It's not just about 'hg log,' though. What about 'hg push -r default'?
Currently, this will push the highest rev of default but with default
being replaced in a user's mental model with 'branch(default)' this
should push each head.

For these reasons, I am fairly against having a conditional hack like
this on top of -r.

I'd like to point out that this is why -b exists. 'hg log -b default' is
the command you're looking for. "But what about topics?" you ask. Well,
personally, I think that extension should add -t to log if it wants that
functionality (-t is available to both 'push' and 'log' for those
curious).

P.S. (personal opinion mini-rant) This is why I don't really like the
concept of topics. In reality, they are conceptually sub-branches. What
I really want 99% of the time is a branch (we call them named branches)
that is closed when merged into default / stable. There are many points
I made about this almost a year ago but the biggest to me are: core hg
commands work out of the box, bitbucket knows how to deal with them, and
old clients know what to do with 90% of the cases.

I do think topics have a place in an advanced workflow but not as
feature branches. /endrant

martinvonz added a comment.Jun 15 2018, 1:13 AM
In D3715#58720, @smf wrote:

Sean Farley <sean@farley.io> writes:

durin42 (Augie Fackler) <phabricator@mercurial-scm.org> writes:

durin42 added subscribers: lothiraldan, smf, durin42.
durin42 accepted this revision as: durin42.
durin42 added a comment.

I'm in favor, but feel like I've got enough conflict of interest I shouldn't land the patches.
@smf @lothiraldan this might be of interest to both of you?

Side note: I keep missing messages that I'm tagged in because I'm not
explicitly mentioned in a CC field. Is it possible to add a CC to each
person tagged in a message?
Side note2: Phabricator emails are really non-trivial to parse and
(worse!) search. The raw emails are not simple, raw text so I'm having
trouble tagging these for higher priority.

I think I got this ironed out now.

Thanks for alerting me of this series! I've had a discussion with Martin
about this on IRC but I'm a bit out of time today to respond (but
definitely do want to respond). (I'm going to try to spend some time in
the mornings to do my email triaging so I can get back on top of this
list.)

This patch strikes me as a Seems-Like-A-Good-Idea-But-Could-Blowup type
of thing. So, what this patch does is conditionally change the behavior
of 'log -r' based on the type of object passed in.

No, it just allows namespaces to do that. As I said (or tried to say) in the commit message, hg log -r stable is protected by BC, so we can't change it. There should be no functional change from this patch.

Unfortunately, I think much of your comments below were based on this incorrect assumption about what this patch does. I've read through it, but it doesn't seem very relevant.

"But what about topics?" you ask. Well,
personally, I think that extension should add -t to log if it wants that
functionality (-t is available to both 'push' and 'log' for those
curious).

This is closer to the actual point of this patch. As I tried to say in the commit message, this patch allows an extension to indicate that the name->nodes mapping it provides should present in the plain revset symbol. For example, if the topics extension opts in to this behavior, then hg log -r my-topic would start including all nodes in the topic. More important to me (I don't use topics) is that our Google-internal extension could opt in (it would kind of lets you do something like hg log -r D3715 and get current and past versions of D3715). Again, I don't intend to change how hg log -r stable behaves.

martinvonz added a comment.EditedJun 15 2018, 1:24 AM
In D3715#58726, @martinvonz wrote:
In D3715#58720, @smf wrote:

Sean Farley <sean@farley.io> writes:

durin42 (Augie Fackler) <phabricator@mercurial-scm.org> writes:

durin42 added subscribers: lothiraldan, smf, durin42.
durin42 accepted this revision as: durin42.
durin42 added a comment.

I'm in favor, but feel like I've got enough conflict of interest I shouldn't land the patches.
@smf @lothiraldan this might be of interest to both of you?

Side note: I keep missing messages that I'm tagged in because I'm not
explicitly mentioned in a CC field. Is it possible to add a CC to each
person tagged in a message?
Side note2: Phabricator emails are really non-trivial to parse and
(worse!) search. The raw emails are not simple, raw text so I'm having
trouble tagging these for higher priority.

I think I got this ironed out now.

Thanks for alerting me of this series! I've had a discussion with Martin
about this on IRC but I'm a bit out of time today to respond (but
definitely do want to respond). (I'm going to try to spend some time in
the mornings to do my email triaging so I can get back on top of this
list.)

This patch strikes me as a Seems-Like-A-Good-Idea-But-Could-Blowup type
of thing. So, what this patch does is conditionally change the behavior
of 'log -r' based on the type of object passed in.

No, it just allows namespaces to do that. As I said (or tried to say) in the commit message, hg log -r stable is protected by BC, so we can't change it. There should be no functional change from this patch.
Unfortunately, I think much of your comments below were based on this incorrect assumption about what this patch does. I've read through it, but it doesn't seem very relevant.

"But what about topics?" you ask. Well,
personally, I think that extension should add -t to log if it wants that
functionality (-t is available to both 'push' and 'log' for those
curious).

This is closer to the actual point of this patch. As I tried to say in the commit message, this patch allows an extension to indicate that the name->nodes mapping it provides should present in the plain revset symbol. For example, if the topics extension opts in to this behavior, then hg log -r my-topic would start including all nodes in the topic. More important to me (I don't use topics) is that our Google-internal extension could opt in (it would kind of lets you do something like hg log -r D3715 and get current and past versions of D3715). Again, I don't intend to change how hg log -r stable behaves.

Also, I said in the commit message that I view a topic as a set of commits. Namespaces allows modeling that (as I'm sure you know). The topics extension takes advantage of that and does model it like that (i.e. by letting namemap() map a topic name to many nodes). It's just that the revset code resolves the symbol to a single value. To help illustrate what I mean, I find it a little silly that the topic extension tests pass with this patch applied:

diff --git a/hgext3rd/topic/__init__.py b/hgext3rd/topic/__init__.py
--- a/hgext3rd/topic/__init__.py
+++ b/hgext3rd/topic/__init__.py
@@ -284,7 +284,7 @@ def _namemap(repo, name):
     if name not in repo.topics:
         return []
     node = repo.changelog.node
-    return [node(rev) for rev in repo.revs('topic(%s)', name)]
+    return [node(rev) for rev in repo.revs('max(topic(%s))', name)]

 def _nodemap(repo, node):
     ctx = repo[node]
smf added a comment.Jun 15 2018, 2:16 AM

It seems I'm having email sending trouble ... going to attempt to send again

smf added a comment.Jun 15 2018, 2:16 AM

--=-=-=
Content-Type: text/plain

martinvonz (Martin von Zweigbergk) <phabricator@mercurial-scm.org> writes:

martinvonz added a comment.

In https://phab.mercurial-scm.org/D3715#58720, @smf wrote:
> Sean Farley <sean@farley.io> writes:
>
> > durin42 (Augie Fackler) <phabricator@mercurial-scm.org> writes:
> >
> >> durin42 added subscribers: lothiraldan, smf, durin42.
> >>  durin42 accepted this revision as: durin42.
> >>  durin42 added a comment.
> >>
> >>   I'm in favor, but feel like I've got enough conflict of interest I shouldn't land the patches.
> >>
> >>   @smf @lothiraldan this might be of interest to both of you?
> >
> > Side note: I keep missing messages that I'm tagged in because I'm not
> >  explicitly mentioned in a CC field. Is it possible to add a CC to each
> >  person tagged in a message?
> >
> > Side note2: Phabricator emails are really non-trivial to parse and
> >  (worse!) search. The raw emails are not simple, raw text so I'm having
> >  trouble tagging these for higher priority.
>
> I think I got this ironed out now.
>
> > Thanks for alerting me of this series! I've had a discussion with Martin
> >  about this on IRC but I'm a bit out of time today to respond (but
> >  definitely do want to respond). (I'm going to try to spend some time in
> >  the mornings to do my email triaging so I can get back on top of this
> >  list.)
>
> This patch strikes me as a Seems-Like-A-Good-Idea-But-Could-Blowup type
>  of thing. So, what this patch does is conditionally change the behavior
>  of 'log -r' based on the type of object passed in.
No, it just allows namespaces to do that. As I said (or tried to say) in the commit message, `hg log -r stable` is protected by BC, so we can't change it. There should be no functional change from this patch.
Unfortunately, I think much of your comments below were based on this incorrect assumption about what this patch does. I've read through it, but it doesn't seem very relevant.

No, I understand what you're trying to do. I was attempting to paint a
picture of what it looks like to me in the sense of "this works for one
type of object but not this other one". Why? Why does it work for topics
and not branches? As a user, and believe me I have years of experience
with angry ones, they do not understand the nuanced differences between
branches and topics. They just lump them all together.

Topics is going out of its way to pretend they are in the same namespace
as branches (that's the motivation of my mini rant in my previous
message). The difference between branches and topics is lost on everyone
outside this thread. When I tried to introduce them onto Bitbucket I
only got headaches: "can I merge between a topic and branch? can I merge
between a topic on branch A and another topic on branch B? if so, what
does that mean? Why is branch B still open? I merged one topic into
another and it closed the three topics it was based on, why?"

It solidified my stance that there should only be one namespace for most
users: branches.

And yes, your patch only applies to topics. My point is the cognitive
load. Why is it '-r topic-foo' for one and not '-r branch-bar' for the
other? To the average hg user, both topics and branches are treated
(semantically) the same. Why one and not the other?

Let's say you and I say a repo. I add a branch (because that's the only
thing I use) and you add a topic. Cool. Another dev comes along. 'hg log
-r martins-work' lists all the commits of his feature work, but, 'hg log
-r seans-work' only lists the tip-most commit of my feature work. Why?
Why should that dev be tripped up by this?

Furthermore, what about the revset language? Should 'topic-foo' become
'topic(topic-foo)' in our AST? I was hoping the abstraction I made
between branches and topics was clear but I guess I am too terse
sometimes, so I apologize.

> "But what about topics?" you ask. Well,
>  personally, I think that extension should add -t to log if it wants that
>  functionality (-t is available to both 'push' and 'log' for those
>  curious).
This is closer to the actual point of this patch. As I tried to say in the commit message, this patch allows an extension to indicate that the name->nodes mapping it provides should present in the plain revset symbol. For example, if the topics extension opts in to this behavior, then `hg log -r my-topic` would start including all nodes in the topic. More important to me (I don't use topics) is that our Google-internal extension could opt in (it would kind of lets you do something like `hg log -r D3715` and get current and past versions of https://phab.mercurial-scm.org/D3715). Again, I don't intend to change how `hg log -r stable` behaves.

Right, but as I tried to explain in my previous email I believe your
extension should add another custom flag of '--phab D3715' to signify
that "I want all the commits of D3715" (e.g. --stack I think in
phabread).

And what about push? Should 'hg push -r D3715' push those other diffs as
well? I know it sounds silly but that's what being an AST means (at
least to me).

Hopefully this message was a bit more clear but if not, then I can try
to discuss in person if that's better.

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEXQX2nmDejz5RkkDD/9jcOZ+fO9YFAlsjUK4ACgkQ/9jcOZ+f
O9ZoZA//f5RD6iA9HnwYhXdt126V1cB5xmgkGaZoDNY54B+hkrmeBQ+/6w1PfQij
pSZToUwhANrrh3L4vnwQOL3al6HII+sftTSmd+sh8/ch1r59+L0f7RYSEPXqapuI
nEE8otm8V18KKvHu48smOl69EoB3UDASX3C2b7E27VF0hJQjPOZV8ai9KOhRZpti
5Tftwhkh7PKdyOdk7JzNkNKZKcfQO1294YIpOFtuhrXa0GH5GPfbVXDT5gIcsIX2
vT9uZ9UWSpQANT7UfIoCh0nMH90uLnG4Jb68Q6nL8F6ZPnmmDn7dO6LV5SdtZUe3
svNBqPU5lSvZKokBpRdhqjYoxCtZ1KBFSGHO/N5km44HckshpS2FhyiXpNJrSxK/
K6lAc0VKTFmj5WwL1VYQiv5pr2GMPppRi5TSkVlsXtDio79qKd31ER9Nkc/ZibEF
A+wn6hLjtgPZAl9BCvGNSAciOPlfC4C3BJ2k7BGcqMGxYir0Kyp1+sl7R68dZ5sm
DEPLLlCmKuuHCOQ+NgEmsavLpEvTO/uypCW3cOBNy8FfLxgLvJZkeXG0daHJJkA1
Jb4dNiNXXPo6c/EL+xpCIUgKJdPmGab/qs7z9WlyZhUUmzpcZgzPFuCImnebgeUW
ABUAk9yBB/thUrunM9QmMKu2QZ8ggUoPInlt5blMbRPUxeMPbJs=

O0Nu

-----END PGP SIGNATURE-----

martinvonz added a comment.Jun 15 2018, 2:55 AM
In D3715#58729, @smf wrote:

--=-=-=
Content-Type: text/plain
martinvonz (Martin von Zweigbergk) <phabricator@mercurial-scm.org> writes:

martinvonz added a comment.

In https://phab.mercurial-scm.org/D3715#58720, @smf wrote:
> Sean Farley <sean@farley.io> writes:
>
> This patch strikes me as a Seems-Like-A-Good-Idea-But-Could-Blowup type
>  of thing. So, what this patch does is conditionally change the behavior
>  of 'log -r' based on the type of object passed in.
No, it just allows namespaces to do that. As I said (or tried to say) in the commit message, `hg log -r stable` is protected by BC, so we can't change it. There should be no functional change from this patch.
Unfortunately, I think much of your comments below were based on this incorrect assumption about what this patch does. I've read through it, but it doesn't seem very relevant.

No, I understand what you're trying to do. I was attempting to paint a
picture of what it looks like to me in the sense of "this works for one
type of object but not this other one". Why? Why does it work for topics
and not branches? As a user, and believe me I have years of experience
with angry ones, they do not understand the nuanced differences between
branches and topics. They just lump them all together.

As I said in the commit message, I'm aware of this argument, I just disagree. I think most users have never even tried to do hg log -r default and that's the only shipped-with-core namespace that behaves funny (IMO).

Topics is going out of its way to pretend they are in the same namespace
as branches (that's the motivation of my mini rant in my previous
message). The difference between branches and topics is lost on everyone
outside this thread. When I tried to introduce them onto Bitbucket I
only got headaches: "can I merge between a topic and branch? can I merge
between a topic on branch A and another topic on branch B? if so, what
does that mean? Why is branch B still open? I merged one topic into
another and it closed the three topics it was based on, why?"
It solidified my stance that there should only be one namespace for most
users: branches.

Sounds more like a criticism of topics.

And yes, your patch only applies to topics.

*Maybe* to topics. That will be up to that extensions developers.

My point is the cognitive
load. Why is it '-r topic-foo' for one and not '-r branch-bar' for the
other? To the average hg user, both topics and branches are treated
(semantically) the same. Why one and not the other?

For historical reasons (hg log -r branch-bar is protected by BC). Again, I think few users have even tried that command. If they did, perhaps they did it hoping that it would list all the commits on the branch?

Let's say you and I say a repo. I add a branch (because that's the only
thing I use) and you add a topic. Cool. Another dev comes along. 'hg log
-r martins-work' lists all the commits of his feature work, but, 'hg log
-r seans-work' only lists the tip-most commit of my feature work. Why?
Why should that dev be tripped up by this?

See my commit message.

Furthermore, what about the revset language? Should 'topic-foo' become
'topic(topic-foo)' in our AST?

No. The way I did it in this patch was to make the symbol "topic-foo" itself resolve to multiple revisions.

I was hoping the abstraction I made
between branches and topics was clear but I guess I am too terse
sometimes, so I apologize.

I think I see the similarities between them (and I did before your message too :)).

> "But what about topics?" you ask. Well,
>  personally, I think that extension should add -t to log if it wants that
>  functionality (-t is available to both 'push' and 'log' for those
>  curious).
This is closer to the actual point of this patch. As I tried to say in the commit message, this patch allows an extension to indicate that the name->nodes mapping it provides should present in the plain revset symbol. For example, if the topics extension opts in to this behavior, then `hg log -r my-topic` would start including all nodes in the topic. More important to me (I don't use topics) is that our Google-internal extension could opt in (it would kind of lets you do something like `hg log -r D3715` and get current and past versions of https://phab.mercurial-scm.org/D3715). Again, I don't intend to change how `hg log -r stable` behaves.

Right, but as I tried to explain in my previous email I believe your
extension should add another custom flag of '--phab D3715' to signify
that "I want all the commits of D3715" (e.g. --stack I think in
phabread).

Revsets were invented to prevent that kind of things, no? What if I want hg log -r 'only(D3715,@)::'? We obviously don't want a flag for that.

And what about push? Should 'hg push -r D3715' push those other diffs as
well?

Yes. Of course, things like hg co D3715 will still need to resolve the revset (which "D3715" is, and which "default" also is) to a single node. No change there.

I know it sounds silly but that's what being an AST means (at
least to me).

I'm not sure what that means, but I hope my replies clarified anyway.

Hopefully this message was a bit more clear but if not, then I can try
to discuss in person if that's better.

Sure, let me know if my replied don't clarify enough.

Again, there should be no user-visible change in this patch. We could safely queue this patch and then let the topics people and the Google people experiment with the feature (or not) and ask them later if it was helpful or harmful.

smf added a comment.Jun 15 2018, 3:19 AM

martinvonz (Martin von Zweigbergk) <phabricator@mercurial-scm.org> writes:

martinvonz added a comment.

In https://phab.mercurial-scm.org/D3715#58729, @smf wrote:
> --=-=-=
>  Content-Type: text/plain
>
> martinvonz (Martin von Zweigbergk) <phabricator@mercurial-scm.org> writes:
>
> > martinvonz added a comment.
> > 
> >   In https://phab.mercurial-scm.org/D3715#58720, @smf wrote:
> >   
> >   > Sean Farley <sean@farley.io> writes:
> >   >
> >   > This patch strikes me as a Seems-Like-A-Good-Idea-But-Could-Blowup type
> >   >  of thing. So, what this patch does is conditionally change the behavior
> >   >  of 'log -r' based on the type of object passed in.
> >   
> >   
> >   No, it just allows namespaces to do that. As I said (or tried to say) in the commit message, `hg log -r stable` is protected by BC, so we can't change it. There should be no functional change from this patch.
> >   
> >   Unfortunately, I think much of your comments below were based on this incorrect assumption about what this patch does. I've read through it, but it doesn't seem very relevant.
>
> No, I understand what you're trying to do. I was attempting to paint a
>  picture of what it looks like to me in the sense of "this works for one
>  type of object but not this other one". Why? Why does it work for topics
>  and not branches? As a user, and believe me I have years of experience
>  with angry ones, they do not understand the nuanced differences between
>  branches and topics. They just lump them all together.
As I said in the commit message, I'm aware of this argument, I just disagree. I think most users have never even tried to do `hg log -r default` and that's the only shipped-with-core namespace that behaves funny (IMO).
> Topics is going out of its way to pretend they are in the same namespace
>  as branches (that's the motivation of my mini rant in my previous
>  message). The difference between branches and topics is lost on everyone
>  outside this thread. When I tried to introduce them onto Bitbucket I
>  only got headaches: "can I merge between a topic and branch? can I merge
>  between a topic on branch A and another topic on branch B? if so, what
>  does that mean? Why is branch B still open? I merged one topic into
>  another and it closed the three topics it was based on, why?"
> 
> It solidified my stance that there should only be one namespace for most
>  users: branches.
Sounds more like a criticism of topics.
> And yes, your patch only applies to topics.
*Maybe* to topics. That will be up to that extensions developers.
> My point is the cognitive
>  load. Why is it '-r topic-foo' for one and not '-r branch-bar' for the
>  other? To the average hg user, both topics and branches are treated
>  (semantically) the same. Why one and not the other?
For historical reasons (`hg log -r branch-bar` is protected by BC). Again, I think few users have even tried that command. If they did, perhaps they did it hoping that it would list all the commits on the branch?
> Let's say you and I say a repo. I add a branch (because that's the only
>  thing I use) and you add a topic. Cool. Another dev comes along. 'hg log
>  -r martins-work' lists all the commits of his feature work, but, 'hg log
>  -r seans-work' only lists the tip-most commit of my feature work. Why?
>  Why should that dev be tripped up by this?
See my commit message.
> Furthermore, what about the revset language? Should 'topic-foo' become
>  'topic(topic-foo)' in our AST?
No. The way I did it in this patch was to make the symbol "topic-foo" itself resolve to multiple revisions.
> I was hoping the abstraction I made
>  between branches and topics was clear but I guess I am too terse
>  sometimes, so I apologize.
I think I see the similarities between them (and I did before your message too :)).
> 
> 
>>   > "But what about topics?" you ask. Well,
>>   >  personally, I think that extension should add -t to log if it wants that
>>   >  functionality (-t is available to both 'push' and 'log' for those
>>   >  curious).
>>   
>>   This is closer to the actual point of this patch. As I tried to say in the commit message, this patch allows an extension to indicate that the name->nodes mapping it provides should present in the plain revset symbol. For example, if the topics extension opts in to this behavior, then `hg log -r my-topic` would start including all nodes in the topic. More important to me (I don't use topics) is that our Google-internal extension could opt in (it would kind of lets you do something like `hg log -r D3715` and get current and past versions of https://phab.mercurial-scm.org/D3715). Again, I don't intend to change how `hg log -r stable` behaves.
> 
> Right, but as I tried to explain in my previous email I believe your
>  extension should add another custom flag of '--phab https://phab.mercurial-scm.org/D3715' to signify
>  that "I want all the commits of https://phab.mercurial-scm.org/D3715" (e.g. --stack I think in
>  phabread).
Revsets were invented to prevent that kind of things, no? What if I want `hg log -r 'only(D3715,@)::'`? We obviously don't want a flag for that.
> And what about push? Should 'hg push -r https://phab.mercurial-scm.org/D3715' push those other diffs as
>  well?
Yes. Of course, things like `hg co D3715` will still need to resolve the revset (which "https://phab.mercurial-scm.org/D3715" is, and which "default" also is) to a single node. No change there.
> I know it sounds silly but that's what being an AST means (at
>  least to me).
I'm not sure what that means, but I hope my replies clarified anyway.
> Hopefully this message was a bit more clear but if not, then I can try
>  to discuss in person if that's better.
Sure, let me know if my replied don't clarify enough.
Again, there should be no user-visible change in this patch. We could safely queue this patch and then let the topics people and the Google people experiment with the feature (or not) and ask them later if it was helpful or harmful.

It's not that this is a user-visible change, it's that we're opening the
floodgates to confusing users. You haven't really addressed my questions
about -r branch and -r topic/google/whatever in the sense that you
haven't addressed the cognitive overload of a user. My point is that
this is dangerous and more harmful to users than the benefits.

yuja added a subscriber: yuja.Jun 15 2018, 8:06 AM
> This patch strikes me as a Seems-Like-A-Good-Idea-But-Could-Blowup type
>  of thing. So, what this patch does is conditionally change the behavior
>  of 'log -r' based on the type of object passed in.
No, it just allows namespaces to do that. As I said (or tried to say) in the commit message, `hg log -r stable` is protected by BC, so we can't change it. There should be no functional change from this patch.

How about adding syntax to resolve multinode namespace symbols?
stable~, for example, will be resolved to all revisions in the stable
branch.

Implementation wise, we can get rid of the revsymbol() hack, and extra
repo[n] lookup won't be needed. A possible drawback (other than the '~'
suffix itself) is we'll soon run out of shell-safe symbols.

martinvonz added a comment.Jun 26 2018, 12:48 AM
In D3715#58736, @yuja wrote:
> This patch strikes me as a Seems-Like-A-Good-Idea-But-Could-Blowup type
>  of thing. So, what this patch does is conditionally change the behavior
>  of 'log -r' based on the type of object passed in.
No, it just allows namespaces to do that. As I said (or tried to say) in the commit message, `hg log -r stable` is protected by BC, so we can't change it. There should be no functional change from this patch.

How about adding syntax to resolve multinode namespace symbols?
stable~, for example, will be resolved to all revisions in the stable
branch.

I was also thinking we could do that, but it feels like a workaround to me. I think of bookmarks and tags as pointing to specific commits. I think of branches as being many commits. hg help revisions says this:

Any other string is treated as a bookmark, tag, or branch name. A bookmark
is a movable pointer to a revision. A tag is a permanent name associated
with a revision. A branch name denotes the tipmost open branch head of
that branch - or if they are all closed, the tipmost closed head of the
branch. Bookmark, tag, and branch names must not contain the ":"
character.

It specifically mentions bookmarks, tags, and branches, and it describes how each name resolves to a commit. To me, that makes it sound like some symbols can resolve to multiple commits. And indeed, revset aliases can already resolve to multiple commits.

Also, the namespace mechanism allows namemap() to return a collection of nodes. That also suggests to me (as a developer now, not a user) that a namespace name is supposed to be able to resolve to multiple nodes.

I don't mind trying to figure out a cleaner implementation (I agree that this is a little ugly), but I don't want a clean implementation to be the reason we choose one UX or another.

yuja added a comment.Jun 26 2018, 8:28 AM
I was also thinking we could do that, but it feels like a workaround to me. I think of bookmarks and tags as pointing to specific commits. I think of branches as being many commits. `hg help revisions` says this:
  Any other string is treated as a bookmark, tag, or branch name. A bookmark
  is a movable pointer to a revision. A tag is a permanent name associated
  with a revision. A branch name denotes the tipmost open branch head of
  that branch - or if they are all closed, the tipmost closed head of the
  branch. Bookmark, tag, and branch names must not contain the ":"
  character.
It specifically mentions bookmarks, tags, and branches, and it describes how each name resolves to a commit. To me, that makes it sound like some symbols can resolve to multiple commits. And indeed, revset aliases can already resolve to multiple commits.

I read it implying that any sort of symbol resolves to a single revision since
all three core "names" work in such way. I'm not against adding multi-node
symbol resolution because it seems useful, but I agree with Sean that it would
introduce inconsistency.

I have no idea about the namemap() API.

martinvonz abandoned this revision.Jul 5 2018, 3:09 AM

I think I've now convinced myself that this won't work. Consider these statements:

  1. A revset should resolve to the same revisions whatever command it's passed to
  2. hg co X should check out the "best" node with name X
  3. hg co X^ should check out the parent of the commit checked out by hg co X
  4. hg log -r X should include all commits in X

It doesn't seem to me like we can have all of them. We currently have 1, 2, and 3. I think I originally would have made it so we had 1, 3, and 4 (because singlenode() picked the highest revision returned from namemap()). Then I sent D3852 because I wanted to preserve 2, but then I realized that that would instead break 3 (because it would be calling singlenode() directly from revsingle(), but calling namemap() from the revset code).

I really don't want to break 1. We could break 2 by requiring a revset to have exactly one revision in revsingle(). Imagine we have a best(name) revset that resolved to the best node given the name (so e.g. best(stable) would find the highest revision on unclosed heads of the stable branch). One would then have to do hg co 'best(stable)', or we could use a ~ suffix (as Yuya suggested for the opposite thing). I think that sounds pretty reasonable, but we obviously can't do that because of BC.

Therefore, my conclusion is that we'll have to live with breaking 4 (as we already do), so I'll abandon this patch.

Revision Contents
Changeset List

PathPackages
Mmercurial/namespaces.py (18 lines)
Mmercurial/revset.py (13 lines)
Mmercurial/scmutil.py (19 lines)
AMtests/paritynamesext.py (26 lines)
Mtests/test-revset2.t (6 lines)

Diff 9016

mercurial/namespaces.py

# we only generate a template keyword if one does not already exist # we only generate a template keyword if one does not already exist
if namespace.name not in templatekw.keywords: if namespace.name not in templatekw.keywords:
templatekeyword = registrar.templatekeyword(templatekw.keywords) templatekeyword = registrar.templatekeyword(templatekw.keywords)
@templatekeyword(namespace.name, requires={'repo', 'ctx'}) @templatekeyword(namespace.name, requires={'repo', 'ctx'})
def generatekw(context, mapping): def generatekw(context, mapping):
return templatekw.shownames(context, mapping, namespace.name) return templatekw.shownames(context, mapping, namespace.name)
def singlenode(self, repo, name): def nodes(self, repo, name, allowmultiple=False):
""" """
Return the 'best' node for the given name. Best means the first node Return the 'best' node for the given name. Best means the first node
in the first nonempty list returned by a name-to-nodes mapping function in the first nonempty list returned by a name-to-nodes mapping function
in the defined precedence order. in the defined precedence order.
Raises a KeyError if there is no such node. Raises a KeyError if there is no such node.
""" """
for ns, v in self._names.iteritems(): for ns, v in self._names.iteritems():
n = v.namemap(repo, name) n = v.namemap(repo, name)
if n: if n:
if (allowmultiple and v.multinode) or len(n) == 1:
return n
# return max revision number # return max revision number
if len(n) > 1:
cl = repo.changelog cl = repo.changelog
maxrev = max(cl.rev(node) for node in n) maxrev = max(cl.rev(node) for node in n)
return cl.node(maxrev) return [cl.node(maxrev)]
return n[0]
raise KeyError(_('no such name: %s') % name) raise KeyError(_('no such name: %s') % name)
class namespace(object): class namespace(object):
"""provides an interface to a namespace """provides an interface to a namespace
Namespaces are basically generic many-to-many mapping between some Namespaces are basically generic many-to-many mapping between some
(namespaced) names and nodes. The goal here is to control the pollution of (namespaced) names and nodes. The goal here is to control the pollution of
jamming things into tags or bookmarks (in extension-land) and to simplify jamming things into tags or bookmarks (in extension-land) and to simplify
'nodemap': function that takes a node and returns a list of names 'nodemap': function that takes a node and returns a list of names
'deprecated': set of names to be masked for ordinary use 'deprecated': set of names to be masked for ordinary use
'builtin': bool indicating if this namespace is supported by core 'builtin': bool indicating if this namespace is supported by core
Mercurial. Mercurial.
""" """
def __init__(self, name, templatename=None, logname=None, colorname=None, def __init__(self, name, templatename=None, logname=None, colorname=None,
logfmt=None, listnames=None, namemap=None, nodemap=None, logfmt=None, listnames=None, namemap=None, nodemap=None,
deprecated=None, builtin=False): deprecated=None, builtin=False, multinode=False):
"""create a namespace """create a namespace
name: the namespace to be registered (in plural form) name: the namespace to be registered (in plural form)
templatename: the name to use for templating templatename: the name to use for templating
logname: the name to use for log output; if not specified templatename logname: the name to use for log output; if not specified templatename
is used is used
colorname: the name to use for colored log output; if not specified colorname: the name to use for colored log output; if not specified
logname is used logname is used
logfmt: the format to use for (i18n-ed) log output; if not specified logfmt: the format to use for (i18n-ed) log output; if not specified
it is composed from logname it is composed from logname
listnames: function to list all names listnames: function to list all names
namemap: function that inputs a name, output node(s) namemap: function that inputs a name, output node(s)
nodemap: function that inputs a node, output name(s) nodemap: function that inputs a node, output name(s)
deprecated: set of names to be masked for ordinary use deprecated: set of names to be masked for ordinary use
builtin: whether namespace is implemented by core Mercurial builtin: whether namespace is implemented by core Mercurial
multinode: whether namespace can have multiple nodes associated with
one name (`hg log -r that-name` will then list multiple
nodes)
""" """
self.name = name self.name = name
self.templatename = templatename self.templatename = templatename
self.logname = logname self.logname = logname
self.colorname = colorname self.colorname = colorname
self.logfmt = logfmt self.logfmt = logfmt
self.listnames = listnames self.listnames = listnames
self.namemap = namemap self.namemap = namemap
self.logfmt = ("%s:" % self.logname).ljust(13) + "%s\n" self.logfmt = ("%s:" % self.logname).ljust(13) + "%s\n"
if deprecated is None: if deprecated is None:
self.deprecated = set() self.deprecated = set()
else: else:
self.deprecated = deprecated self.deprecated = deprecated
self.builtin = builtin self.builtin = builtin
self.multinode = multinode
def names(self, repo, node): def names(self, repo, node):
"""method that returns a (sorted) list of names in a namespace that """method that returns a (sorted) list of names in a namespace that
match a given node""" match a given node"""
return sorted(self.nodemap(repo, node)) return sorted(self.nodemap(repo, node))
def nodes(self, repo, name): def nodes(self, repo, name):
"""method that returns a list of nodes in a namespace that """method that returns a list of nodes in a namespace that
match a given name. match a given name.
""" """
return sorted(self.namemap(repo, name)) return sorted(self.namemap(repo, name))

mercurial/revset.py

def _sortedb(xs): def _sortedb(xs):
return sorted(util.rapply(pycompat.maybebytestr, xs)) return sorted(util.rapply(pycompat.maybebytestr, xs))
# operator methods # operator methods
def stringset(repo, subset, x, order): def stringset(repo, subset, x, order):
if not x: if not x:
raise error.ParseError(_("empty string is not a valid revision")) raise error.ParseError(_("empty string is not a valid revision"))
x = scmutil.intrev(scmutil.revsymbol(repo, x)) ctxs = scmutil.revsymbolmultiple(repo, x)
if len(ctxs) == 1:
x = scmutil.intrev(ctxs[0])
if (x in subset if (x in subset
or x == node.nullrev and isinstance(subset, fullreposet)): or x == node.nullrev and isinstance(subset, fullreposet)):
return baseset([x]) return baseset([x])
return baseset() return baseset()
return subset & baseset(sorted(ctx.rev() for ctx in ctxs))
def rangeset(repo, subset, x, y, order): def rangeset(repo, subset, x, y, order):
m = getset(repo, fullreposet(repo), x) m = getset(repo, fullreposet(repo), x)
n = getset(repo, fullreposet(repo), y) n = getset(repo, fullreposet(repo), y)
if not m or not n: if not m or not n:
return baseset() return baseset()
return _makerangeset(repo, subset, m.first(), n.last(), order) return _makerangeset(repo, subset, m.first(), n.last(), order)

mercurial/scmutil.py

ambiguous nodeid prefix. ambiguous nodeid prefix.
""" """
try: try:
revsymbol(repo, symbol) revsymbol(repo, symbol)
return True return True
except error.RepoLookupError: except error.RepoLookupError:
return False return False
def _revsymbol(repo, symbol): def _revsymbol(repo, symbol, allowmultiple):
if not isinstance(symbol, bytes): if not isinstance(symbol, bytes):
msg = ("symbol (%s of type %s) was not a string, did you mean " msg = ("symbol (%s of type %s) was not a string, did you mean "
"repo[symbol]?" % (symbol, type(symbol))) "repo[symbol]?" % (symbol, type(symbol)))
raise error.ProgrammingError(msg) raise error.ProgrammingError(msg)
try: try:
if symbol in ('.', 'tip', 'null'): if symbol in ('.', 'tip', 'null'):
return [repo[symbol]] return [repo[symbol]]
return [repo[rev]] return [repo[rev]]
except error.FilteredLookupError: except error.FilteredLookupError:
raise raise
except (TypeError, LookupError): except (TypeError, LookupError):
pass pass
# look up bookmarks through the name interface # look up bookmarks through the name interface
try: try:
node = repo.names.singlenode(repo, symbol) nodes = repo.names.nodes(repo, symbol, allowmultiple)
rev = repo.changelog.rev(node) revs = [repo.changelog.rev(n) for n in nodes]
return [repo[rev]] return [repo[r2] for r2 in revs] # "r2" to keep pyflakes happy
except KeyError: except KeyError:
pass pass
node = resolvehexnodeidprefix(repo, symbol) node = resolvehexnodeidprefix(repo, symbol)
if node is not None: if node is not None:
rev = repo.changelog.rev(node) rev = repo.changelog.rev(node)
return [repo[rev]] return [repo[rev]]
raise error.RepoLookupError(_("unknown revision '%s'") % symbol) raise error.RepoLookupError(_("unknown revision '%s'") % symbol)
except error.WdirUnsupported: except error.WdirUnsupported:
return [repo[None]] return [repo[None]]
except (error.FilteredIndexError, error.FilteredLookupError, except (error.FilteredIndexError, error.FilteredLookupError,
error.FilteredRepoLookupError): error.FilteredRepoLookupError):
raise _filterederror(repo, symbol) raise _filterederror(repo, symbol)
def revsymbol(repo, symbol): def revsymbol(repo, symbol):
"""Returns a context given a single revision symbol (as string). """Returns a context given a single revision symbol (as string).
This is similar to revsingle(), but accepts only a single revision symbol, This is similar to revsingle(), but accepts only a single revision symbol,
i.e. things like ".", "tip", "1234", "deadbeef", "my-bookmark" work, but i.e. things like ".", "tip", "1234", "deadbeef", "my-bookmark" work, but
not "max(public())". not "max(public())".
""" """
ctxs = _revsymbol(repo, symbol) ctxs = _revsymbol(repo, symbol, allowmultiple=False)
assert len(ctxs) == 1 assert len(ctxs) == 1
return ctxs[0] return ctxs[0]
def revsymbolmultiple(repo, symbol):
"""Returns all contexts given a single revision symbol (as string).
This is similar to revsymbol(), but if "symbol" is a namespace symbol,
then this may return many context (if the namespace maps the symbol to
many revisions).
"""
return _revsymbol(repo, symbol, allowmultiple=True)
def _filterederror(repo, changeid): def _filterederror(repo, changeid):
"""build an exception to be raised about a filtered changeid """build an exception to be raised about a filtered changeid
This is extracted in a function to help extensions (eg: evolve) to This is extracted in a function to help extensions (eg: evolve) to
experiment with various message variants.""" experiment with various message variants."""
if repo.filtername.startswith('visible'): if repo.filtername.startswith('visible'):
# Check if the changeset is obsolete # Check if the changeset is obsolete

tests/paritynamesext.py

  • This file was added.
# Defines a namespace with names 'even' and 'odd'
from __future__ import absolute_import
from mercurial import (
namespaces,
)
def reposetup(ui, repo):
def namemap(repo, name):
if name == 'even':
parity = 0
elif name == 'odd':
parity = 1
else:
return []
return [repo[rev].node() for rev in repo if rev % 2 == parity]
def nodemap(repo, node):
return [repo[node].rev() % 2]
ns = namespaces.namespace(b'parity', templatename=b'parity',
logname=b'parity',
listnames=lambda r: ['even', 'odd'],
namemap=namemap, nodemap=nodemap,
multinode=True)
repo.names.addnamespace(ns)

tests/test-revset2.t

[255] [255]
$ log 'present(named("unknown"))' $ log 'present(named("unknown"))'
$ log 'present(named("re:unknown"))' $ log 'present(named("re:unknown"))'
$ log 'tag()' $ log 'tag()'
6 6
$ log 'named("tags")' $ log 'named("tags")'
6 6
$ hg --config extensions.revnamesext=$TESTDIR/paritynamesext.py log --template '{rev}\n' -r even
0
2
4
6
8
issue2437 issue2437
$ log '3 and p1(5)' $ log '3 and p1(5)'
3 3
$ log '4 and p2(6)' $ log '4 and p2(6)'
4 4
$ log '1 and parents(:2)' $ log '1 and parents(:2)'