Replace --allow-large-revsets by all: prefix

[martinvonz]

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• I have updated the config schema (src/config-schema.json)
• I have added tests to cover my changes

[PhilipMetzger]

This looks great, although I'm left wondering if we should do a soft deprecation for the --allow-large-revsets option and then remove it.

cc @ilyagr, for a final look.

[martinvonz]

This looks great, although I'm left wondering if we should do a soft deprecation for the --allow-large-revsets option and then remove it.

Good idea. I kept the flag and made it print an error message. I'm not sure if that counts as soft deprecation, but I think it's good enough.

[ocharles]

Out of interest, what's the motivation? Fewer CLI flags?

[martinvonz]

Out of interest, what's the motivation? Fewer CLI flags?

Sorry, I really should have said in the commit message. Will update it. But yes, it's so we have a generic way without needing a new flag for each revset you might want to allow duplicates for. For some commands, you might even need multiple such flags otherwise (--allow-large-revset-in-destination and --allow-large-revset-in-source or similar), which becomes very awkward.

[chooglen]

In my mental model of revsets, multiple() feels a bit like an oddball because it doesn't affect which revisions the revset resolves to. I think of 'whether or not multiple revisions are allowed' as a concern for whatever consumes the revset. Does that match your mental model?

[necauqua]

In my mental model it's just a metadata flag, which the consumer could or could not care about, similar to what -L was but embedded in the revset.

Generally you could just always allow those, but this flag is just 'yes-I-know-what-Im-doing' confirmation (and also like a divergent changeid or branch would be caught by this too).

Avoiding having -L-in-dest and -L-in-src is very nice.

[chooglen]

In my mental model it's just a metadata flag

Yes, exactly! I would prefer for such flags to be expressed in its own syntax than to reuse the revset 'function' syntax.
In the meantime, I'm ok to document that this 'function' isn't really a 'function' and punt on what the metadata syntax should be (probably until we decide to add another one).

[PhilipMetzger]

Good idea. I kept the flag and made it print an error message. I'm not sure if that counts as soft deprecation, but I think it's good enough.

That's definitely a soft deprecation in my book, as we discourage further use before removing it.

[necauqua]

expressed in its own syntax

Yes I was thinking about having some sigil for this or something, but just using the function syntax for everything is also not bad, idk - and way, way less hacky, even if still a bit hacky, than having those -L flags I always disliked

[martinvonz]

expressed in its own syntax

Yes I was thinking about having some sigil for this or something, but just using the function syntax for everything is also not bad, idk - and way, way less hacky, even if still a bit hacky, than having those -L flags I always disliked

I think we talked about using % or maybe other symbols like *. I feel like jj rebase -d 'x|y%' or jj rebase -d '*x|y' looks too much like regular operators (and this sigil we're talking about would only be allowed at the top level), but maybe we can find a better syntax. We can't use many:x|y because : is already an operator. I suppose we could use it if we replaced the current : operator by :: to match hg :)

[necauqua]

Taking inspiration from Rust itself - and the fact that you were discussing having other things like sized which would take extra arguments - we could have an alternative function application syntax, which is like multiple!(..) where the ! (or other symbol or other form of parens etc) denotes that this is not a revset function but rather is something different, just like Rust does with macros.

Just a random idea to consider :)

Well since those flags or whatever are toplevel-only, the syntax you were describing (something like prefix many:) is basically that, I guess

Never used hg in my life but having less syntax difference in things that are (it seems, idk 🙃) extremely similar is a good thing imo

[ilyagr]

It might take me a bit of time to think about it properly and/or try out jj with that patch. Feel free to just merge it without waiting for me.

I suppose we could use it if we replaced the current : operator by :: to match hg :)

something like prefix many:

I like these ideas since they are shorter to type and don't require quotes.

This also suggests a possible solution to another minor issue I have. I often intentionally omit --allow-large-revsets to preview what the command is going to do before doing it. (Then, I just add -L to the command). One more flexible way to solve this usecase would be to have another operator show:. Using show:revset should act like many:revset but also print out the revisions that revset expanded to. I can then use jj undo if I the list of revisions is not what I expected.

[ilyagr]

To be clear, what I said isn't meant to be blocking.

[martinvonz]

I feel strongly that we should replace the --allow-large-revsets by some syntax in each revset (or possibly outside the revset, depending on how you define the scope of the revset syntax), but I don't feel strongly about that syntax.

I like many:x|y (or all:x|y, perhaps) because it can be reused for file pattern syntax later if we decide to copy hg's way of doing it (things like glob:**.c). The main drawback is that we'd need to replace the current : operator by ::, which is a somewhat large breaking change. Still, if we think that syntax is the best solution, then I think it's better to change it now than to be stuck with something we don't like as much - I think we still have few enough users that we can do that.

We can have foo:bar continue to work and just print a warning. The only case we'd need to break right away is if exactly many:foo, but it's unlikely to affect many users because I don't think a branch named many is very common.

I'm also fine with going with multiple() as implemented (or renaming it to e.g. unbounded()).

[yuja]

I like many:x|y (or all:x|y, perhaps) because it can be reused for file pattern syntax later if we decide to copy hg's way of doing it (things like glob:**.c).

+1

The main drawback is that we'd need to replace the current : operator by ::, which is a somewhat large breaking change.

This might be actually better for newcomers since : has different use case in both hg and git. We can allow foo:bar range as you said, but I prefer to remove ambiguous syntax at some point.

[ilyagr]

If we're OK with many:, perhaps we should rename multiple to many, just because it's shorter to type?

I wrote out some thoughts below, and my conclusion from them is that I'd rename multiple to many for now, submit this, and see how we like it. I like the many: idea quite a bit, and I wouldn't mind switching to it right now too much, but it seems a little underbaked.

Various thoughts I had.

• I like many: as well, mainly because I'm still excited about show:.

  It's not entirely clear how many: is distinguished form a function call. In my mind, we could consider it different because it affects the interpretation of the entire revset. I'm not sure if glob: would make sense in this interpretation.

  Alternatively, we could consider it a function and funtion:argument could be an alternative function call syntax with the explicit purpose of being easier to type in the shell and not requiring the user to track so many pairs of parentheses. It would have low precedence, similar to Haskell's $.

• Other symbols that come to mind are many~revset (would require to change negation to ~~ or something) and many%revset (seems like it would just work). I think this looks a bit uglier, but I'm guessing I'd get used to it quickly.

• I'm OK with switching to many: and :: immediately.

• I'm OK with trying out many() first and then revisiting this a bit later. I'd probably implement show() at some point, since I think I'll miss that part of --allow-large-revsets functionality. I think undo is now reliable enough to rely on it and give the user ways to be informed about potential errors instead of trying to prevent all user errors.

• I'm OK with having both many and many%revset for a while to see if we like %.

[ilyagr]

Replying to myself, after I saw Yuya's message.

It's not entirely clear how many: is distinguished form a function call. In my mind, we could consider it different because it affects the interpretation of the entire revset. I'm not sure if glob: would make sense in this interpretation.

It's encouraging to me that @yuja doesn't seem concerned about this. Do you have a mental model of what should be a function call and what should be a something: operator (or something%)? What are other examples of something: operators we'd want to have?

This is not blocking for this PR (in any of its potential forms).

[martinvonz]

It's encouraging to me that @yuja doesn't seem concerned about this. Do you have a mental model of what should be a function call and what should be a something: operator (or something%)? What are other examples of something: operators we'd want to have?

IMO, what justifies a different syntax for this is that it's about changing the behavior of the revset as whole - jj rebase -d 'multiple(x)|y' would be syntactically valid but have no effect (at least the way I've implemented it in this PR. I haven't thought about what other prefixes we might have. On the other hand, if we add a sized() revset function, then that would make sense when applied to a sub-expression. We could treat a top-level sized() specially, but many: seems clearer.

(I think I actually like all: better than many:, btw.)

[yuja]

Do you have a mental model of what should be a function call and what should be a something: operator (or something%)? What are other examples of something: operators we'd want to have?

I think <kind>:<expr> changes the interpretation of <expr>. I don't think we'll have many <kind>s for revset, but if we had a Git-compatible revset parser, it might be like git:HEAD^. More practical example is string pattern like branches(glob:"foo/*").

[ilyagr]

There another usecases in my mind for this kind of syntax with operators like glob% or regex%, both outside revsets and inside revset functions like branches(). This would not be global. I suppose such operators could be functions taking strings, but that's a bit long to type.

[ilyagr]

Re %: that symbol might require quoting on Windows. I'm not sure what the rule is if it's in the middle of a word, so there's a chance it's OK.

[martinvonz]

Re %: that symbol might require quoting on Windows. I'm not sure what the rule is if it's in the middle of a word, so there's a chance it's OK.

I would go with : anyway. It seems like a more obvious choice, and it's what hg chose for file patterns.

I wrote out some thoughts below, and my conclusion from them is that I'd rename multiple to many for now

My current thinking is that we should do (and I think you said you're okay with this too):

1. Allow :: as an alternative to :
2. Print a warning when : is used
3. Replace --allow-large-revsets by all:, still allowing : everywhere (only exactly the all: prefix will be stripped)
4. Drop support for :

I don't know how much time I think we need between each step. I think we can probably do the first 3 in quick succession.

[martinvonz]

This PR now instead replaces : by :: and replaces --allow-large-revsets by a all: prefix

[PhilipMetzger]

LGTM, but wait on yuya or ilya before merging

[ilyagr]

Looks great overall, I added some optional comments. I'll probably finish reviewing later (though I also think there's no need to block this on my review). Either way, it seems like you'll rework this a bit based on Yuya's comments.

[ilyagr]

LGTM.

I consider the "make --allow-large-revset no longer also allow duplicates" an experiment -- I think it might be better to always allow those, but merging it and testing it out might be easier than thinking it through.