FR: Fix inconsistencies in how subcommands handle revision and path args

[necauqua]

My proposal is that every command that works with a revset and an optional list of paths should have the following form:

jj subcommand revset* (-p path+ | -r revset+ | other-args)*

Here the first revset is an optional repeating positional arg, followed by (potentially) interleaved -p/--path(s) and -r/--revisions arguments, I described it with a pseudo-regex here.

This way, any of the following is valid:

• jj log
• jj log rev
• jj log rev1 rev2
• jj log -r rev
• jj log rev -p path1 path2
• jj log -r rev -p path1 path2
• jj log -p path1 path2 -r rev
• jj log -p path1 -r rev -p path2 path3
• jj log rev1 rev2 -p path1 path2
• jj log rev -r rev2 rev3
• jj log -p path1 -r rev -p path2 -r rev2
• etc

Another way to describe it is that we consume "positional" args to be either a revset or a path depending on the state, which is initially "accept as revset", which can be switched to "accept as path" by -p and back to "accept as revset" by -r (or technically any other non-positional arg) - and also -p or -r are not allowed at the end.
Forget about it, there's no stateful flags in reality 🙃

---

If command doesn't work with paths, we just dont have them, and now it's similar to how we have unused_revision: bool flag in some places (except it's not allowed at the end now too).
This should cover most commands, actually, all of them that work with commits I guess.

Multiple revsets also don't make sense for all commands - for those we can just check and error out - or we can join them by | and then check if they resolve to a single commit.
Note that something like jj new a b should work, but if a diverges we must pass all: to signify that we're sure we want to merge a divergent branch or whatever that is and also add b into the mix - basically we don't want unexpected (where it matters, e.g. log is ok) revision resolutions without all:.

The main thing everything should be consistent :)

[spectral54]

I'm not generally a huge fan of "argument that changes the behavior of the following arguments", unless I can convince myself that it's a mode selector for the entire command. So something like tar -x seems fine to me, because I treat that as equivalent to an "untar" command (or maybe because I just run it so frequently). Similarly, "subcommands" seem fine. But things like find and ps just have inscrutable behavior to me, so I end up memorizing the few usages I commonly need, and need to look up anything more than that.

I worry that something like jj log rev -p path1 rev2 would happen, and be very confusing for users, because they don't really realize that -p has side effects. It's not -p=path1, it's --treat-positional-args-after-this-as-paths. Every month or so I try to run hg log revset (without the -r) and get back no results (because revset isn't a file, so it trivially succeeds). I'd prefer to avoid mechanisms that could lead to that kind of confusion happening more frequently.

Maybe it'd be fine once I got used to it, but I don't know of prior art for this style of "something that looks like a flag changes the behavior/interpretation of things that look like positional args". Actually I guess I have one example: gdb --args? I feel like there's some extra caveats around gdb --args though.

[necauqua]

The thing is that -p doesn't have side-effects, perhaps my second description (when I said "another way to describe" I really meant "another way to view it as in case you didn't understand my clumsy description") was actually discouraging/misleading, ugh

-r and -p just accept multiple values, very simple :)

because revset isn't a file

That's liiiiiterally the reason I proposed this whole thing, it's the same in jj currently:

And then some other commands happily work with positional revset, in-con-sis-tent

[necauqua]

The main reason hg and subsequently jj use positional args for paths is for shell expansion (like jj log *.txt) to work.

Clap allows non-positional args to have multiple values, and in my experience revsets are used way more often that paths - so now jj log -p *.txt will work instead.

Having -r is not even necessary, it's just for completeness, so that you could really specify the revset and paths anywhere - e.g. when typing a command to not have to backtrack before paths or something, or to be additive in scripts, like for xargs or something

Again, I would honestly be happy with whatever as long as it is consistent, currently we only have a band aid of unused_revision: bool in some places which is a little patchy itself :)

[martinvonz]

I know clap supports parsing arguments like that. I went to look up the function because I was thinking we might find examples of applications that use it that way. Here's how to set it using the "builder API" (we mostly use the "derive API", i.e. attributes): https://docs.rs/clap/latest/clap/struct.Arg.html#method.num_args. The warning there is something to keep in mind. It means that we will not be able to allow jj --config-toml "key1=foo" "key2=bar", since that could conflict with subcommands.

[spectral54]

So based on that clap documentation, if I wrote jj log -p foo bar --no-pager baz, where --no-pager doesn't take an argument, this would interpret baz as a revision (after the next flag is found, the value list for -p is ended, and so baz is interpreted as a positional argument, which means as a revision)? That's a little confusing to me as well :(

Maybe this really indicates that there should be a jj filelog command, separate from log. :)

[yuja]

-r and -p just accept multiple values, very simple :)

I think the problem is that flags taking multiple values aren't so common (or at least, I'm not used to be.)

It would be also a bit weird if jj branch set -r REV NAME is interpreted as -r REV -r NAME. We can of course require -b NAME, but multiple -r arguments are useless here.

[necauqua]

It would be also a bit weird if jj branch set -r REV NAME is interpreted as -r REV -r NAME. We can of course require -b NAME, but multiple -r arguments are useless here.

Initially I wanted -r to accept one arg, I only really changed it so that something like jj new .. -r a b makes sense, but now I realise that with positional args it'll work anyway, for jj new at least. And most commands need one revision anyway

[necauqua]

That's a little confusing to me as well :(

Eh, maybe I worked with fancy rust apps using clap more, makes perfect sense to me 🙃

Well, your example would happen rarely (and log should probably print a little hint that revset "baz" matched no commits), while jj log rev happens all the time

[arxanas]

The main reason hg and subsequently jj use positional args for paths is for shell expansion (like jj log *.txt) to work.

We have to enclose revset expressions in quotes anyways pretty quickly when they have parentheses. Isn't it fine to force users to write jj log -p '*.txt' in the same way?

Supporting shell expansion seems like a pretty niche feature. I would rather have -r/-p be consistent with other CLI applications an remove the positional argument footgun, and sacrifice shell expansion support (particularly since there seems to be a workable alternative?).

[martinvonz]

The main reason hg and subsequently jj use positional args for paths is for shell expansion (like jj log *.txt) to work.

We have to enclose revset expressions in quotes anyways pretty quickly when they have parentheses. Isn't it fine to force users to write jj log -p '*.txt' in the same way?

It's not that hg and jj support shell expansion themselves, it's that by using positional arguments, they allow the shell to do it. For example, so if you run jj log *.txt in your shell, what jj actually sees might be jj log foo.txt bar.txt.

[arxanas]

@martinvonz Sorry, I didn't state/justify my assumptions. Git will expand *.txt on its own, separately from shell expansion, which means that you can write git log -- '*.txt' and have it do the right thing. I'm assuming from your answer that Mercurial doesn't do glob-expansion by default, but presumably there is some syntax you can use to enable that kind of pattern?

I would much prefer that we support glob patterns in -p somehow and force users to quote their glob patterns, rather than let -p take multiple arguments just so that we can make use of shell expansion. Besides glob patterns being expanded by the shell, is there any other reason to prefer positional/variadic arguments?

I assume there is some rare workflow where you really want your glob expansion to match an untracked file because you really want to traverse the history of that file in the past when it was tracked, and also there are multiple files matching the glob pattern... and I don't feel it's worth optimizing for that.

Should this FR be a discussion? We might benefit from semi-threaded replies.

[ilyagr]

I don't have any conclusion in mind, but here are a few thoughts.

Besides glob patterns being expanded by the shell, is there any other reason to prefer positional/variadic arguments?

Shells can support fancier expansions, though I suppose we could reimplementing them. I don't think the glob library supp (Update: @arxanas noticed I left over an obsolete sentence fragment, ignore it)

I do think jj log *.md docs/*.md can be convenient. Would you require quoting both?

I believe some users will get confused about the difference between jj log -p *.txt and jj log -p '*.txt'. So, we'd go from having trouble with supplying good messages when people forget -r to having trouble giving good error messages when people do jj log -p *.txt (unless we make it a policy not to have any positional arguments for such commands, in which case it seems we might as well go with @necauqua 's original idea of jj log -p path1 path2 -r rev).

I discovered that https://docs.rs/glob/latest/glob/struct.Pattern.html does support jj log **/*.rs. It does not support jj diff {cli,lib,.}/Cargo.toml, but that might be not super-useful anyway.

---

One weird, but maybe workable, alternative would be:

echo **/*.rs | jj log --files-from-stdin

This could be combined with the idea of implementing some simple (or not so simple) globbing in jj and doing jj log -p '*.txt'.

[arxanas]

I don't think the glob library supp

Sentence cut off here?

I do think jj log *.md docs/*.md can be convenient. Would you require quoting both?

Yes, in my ideal setup, you would have to write

jj log -p '*.md' -p 'docs/*.md'

(or simply {,docs/}*.md, but it seems like the glob library doesn't support that, according to your statement).

unless we make it a policy not to have any positional arguments for such commands, in which case it seems we might as well go with @necauqua 's original idea of jj log -p path1 path2 -r rev).

I think it would be good to not have positional arguments for such commands, but I don't agree that we should support variadic -r/-p just for this one use-case.

[necauqua]

Note that positional revset is good (and we won't have -r variadic, just 1, probably even just leave the unused_revision: bool thingy) - it's just always irritating to have to do jj log -r rev or jj diff -r rev instead of jj log rev, especially when you could do jj abandon rev or jj new rev

[martinvonz]

Sorry about the delay.

@martinvonz Sorry, I didn't state/justify my assumptions. Git will expand *.txt on its own, separately from shell expansion, which means that you can write git log -- '*.txt' and have it do the right thing. I'm assuming from your answer that Mercurial doesn't do glob-expansion by default, but presumably there is some syntax you can use to enable that kind of pattern?

Mercurial does have glob expansion, but you need to prefix with glob:.

I would much prefer that we support glob patterns in -p somehow and force users to quote their glob patterns, rather than let -p take multiple arguments just so that we can make use of shell expansion. Besides glob patterns being expanded by the shell, is there any other reason to prefer positional/variadic arguments?

I agree that supporting globs in patterns is useful. My point was just that by requiring -p before each path means that you can't do jj log -p foo.*. I don't like the idea of making that work by allowing jj log -p foo.x foo.y. Rather, I was trying use that as an argument for not having a -p at all. To be clear, I'm not sure that we shouldn't have -p, but we should keep this case in mind.

[arxanas]

I agree that supporting globs in patterns is useful. My point was just that by requiring -p before each path means that you can't do jj log -p foo.*. I don't like the idea of making that work by allowing jj log -p foo.x foo.y. Rather, I was trying use that as an argument for not having a -p at all. To be clear, I'm not sure that we shouldn't have -p, but we should keep this case in mind.

A simple solution might be -p/--path for single arguments (the common case) and --paths accepting any number of arguments, particularly useful for shell expansion, without surprising users who reach for the short option.

Removing all positional path arguments in favor of explicit -p/--path/--paths arguments might be a different issue, then.