Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: default to log when no subcommand is provided #1525

Merged
merged 1 commit into from Apr 17, 2023

Conversation

elasticdog
Copy link
Collaborator

@elasticdog elasticdog commented Apr 14, 2023

This is a convenience optimization to improve the default user experience, since jj log is a frequently run command. Accessing the help information explicitly still follows normal CLI conventions, and instructions are displayed appropriately if the user happens to make a mistake. Discoverability should not be adversely harmed.

Note that this behavior mirrors what Sapling does, where sl will display the smartlog by default.

If people are onboard with this change, where else would you recommend that the behavior be documented?

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

@joyously
Copy link

Since the working copy is "committed" at some invocations of jj (unknown to me) but not others, there needs to be consistency.
If there is a status command, that would be preferable to a log, and it should not commit anything.

@martinvonz
Copy link
Owner

Since the working copy is "committed" at some invocations of jj (unknown to me) but not others

FYI, it's practically all.

there needs to be consistency. If there is a status command, that would be preferable to a log, and it should not commit anything.

It already does that...

@arxanas
Copy link
Collaborator

arxanas commented Apr 14, 2023

@joyously by the way, it might not be obvious from this PR, but it's changing jj to match Sapling's behavior, where it shows the graph log when you invoke sl without any arguments. This seems to have been a popular UI decision on their part, since patch stack users are constantly viewing the commit graph. (I realize now that this explanation is entirely in the commit message, but Github doesn't make it easy to notice that, so maybe you missed it as well 😅.)

@joyously
Copy link

it might not be obvious from this PR, but it's changing jj to match Sapling's behavior, where it shows the graph log when you invoke sl without any arguments.

I sort of got that from the comments.
I was just trying to emphasize that new users would be more likely to not specify a subcommand, and that a plain invocation like that should not affect the repo.
In the sense that no subcommand is given, a status makes more sense than a log, or maybe it should be a combination when neither is specified.

@ilyagr
Copy link
Collaborator

ilyagr commented Apr 15, 2023

One hesitation I have is that I actually find the output of jj without arguments useful. I think it's especially useful for a new user. Potentially, it will become more useful if we can better structure the list of commands.

I find jj help and jj --help a bit too verbose; otherwise they'd be a good alternative.

One option I can think of is to have jj print a hint in addition to the log:

Hint: use `jj -h` for a list of jj's commands. Set `ui.default-command = "log"` config to disable this message.

Another alternative I've been thinking about is encouraging alias "jl=jj log". We could even have jj util shell-setup that includes jj util completion and also sets up this alias. On the other hand, I'm not sure if that is discoverable enough.

Finally, we could look into making jj help work the same as jj -h, and then have jj help --full. I'm not sure whether clap supports that.


I don't think these worries have to prevent us from merging this and trying it out, but I thought I'd share them.

@elasticdog
Copy link
Collaborator Author

Okay, I've pushed a new version where the default command is resolved earlier in the process per @martinvonz's suggestion. That did solve the previous limitation of only being able to parse global options, and it will now pass through any options as expected. The behavior can be configured by setting ui.default-command to a string with the desired subcommand name, subcommand alias, or user-defined alias (which can be used if you want multiple arguments to be injected).

The one minor issue I've seen with the approach is that specifying jj -T show won't work where jj log -T show will, as I naively bail on using a default command if any arguments match an existing valid subcommand (jj show, in this case). I'm open to any suggestions for more robust handling, but it's a pretty small edge case.

Copy link
Owner

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think this feature is a bit weird because of the inconsistencies (like the jj -T show case you mentioned).

I also think @ilyagr had a good point about showing the short help to new users, but maybe we can improve that by printing warning (as Ilya said).

lib/src/settings.rs Outdated Show resolved Hide resolved
tests/test_alias.rs Outdated Show resolved Hide resolved
@elasticdog
Copy link
Collaborator Author

I came up with a less-naive approach and now check the ArgMatches directly, so I believe all inconsistencies have been removed (jj -T show works, and all previous tests have been restored). Let me know what you think.

@elasticdog
Copy link
Collaborator Author

Related to the preference for short help, it might be possible, but it would take a bit of work. Clap's auto-generated help subcommand actually used to default to short help and users could explicitly override it to prefer the long help...but then, upstream removed the option and forcibly defaulted to long. After that, the ArgActions feature became supported, which might provide a clean way to customize things (see clap-rs/clap#4687). A final resort would be to disable the built-in auto-generated command, but then I think we'd be on the hook to maintain the desired help output ourselves (see: clap-rs/clap#4056).

@elasticdog
Copy link
Collaborator Author

elasticdog commented Apr 16, 2023

Oh, and I think we can easily print out the above suggested hint if the setting isn't in place. Give me a bit and I'll push that change and tests.

EDIT: done.

@elasticdog elasticdog force-pushed the push-stzptnsmvtqv branch 3 times, most recently from bf4259d to 17aeb6a Compare April 17, 2023 01:46
docs/config.md Show resolved Hide resolved
src/cli_util.rs Show resolved Hide resolved
src/cli_util.rs Outdated Show resolved Hide resolved
@elasticdog elasticdog force-pushed the push-stzptnsmvtqv branch 2 times, most recently from 31d9817 to d71d77f Compare April 17, 2023 15:45
Copy link
Owner

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty simple now :) Thanks!

docs/config.md Show resolved Hide resolved
src/cli_util.rs Outdated Show resolved Hide resolved
This is a convenience optimization to improve the default user
experience, since `jj log` is a frequently run command. Accessing the
help information explicitly still follows normal CLI conventions, and
instructions are displayed appropriately if the user happens to make a
mistake. Discoverability should not be adversely harmed.

Note that this behavior mirrors what Sapling does [2], where `sl` will
display the smartlog by default.

[1] clap-rs/clap#975
[2] https://sapling-scm.com/docs/overview/smartlog
@elasticdog elasticdog merged commit 6c627fb into martinvonz:main Apr 17, 2023
14 checks passed
@elasticdog elasticdog deleted the push-stzptnsmvtqv branch April 17, 2023 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants