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

Authored by martinvonz on Jun 11 2018, 7:10 PM.

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
martinvonz created this revision.Jun 11 2018, 7:10 PM
durin42 accepted this revision as: durin42.Jun 12 2018, 1:41 PM
durin42 added subscribers: lothiraldan, smf, 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.)

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.

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?

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

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

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.

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.