add support for `jj help <alias>` #3015

jyn514 · 2024-02-10T16:04:27Z

to test the new completion support in bash, run source <(cargo run util completion bash). jj <TAB> should show any aliases you have in jj.toml.

Checklist

If applicable:

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

thoughtpolice · 2024-02-10T20:57:59Z

Two minor notes (that are mostly general gotchas, nothing specific)

I think we typically want the first line of the commit message to be of the form thing: description where "thing" is typically something like the name of the crate your patch touches (cli/lib), or cargo:, or docs:, something to that effect. In this case both of your commits would be cli: blah blah...
- We don't enforce this at all. It's not hard to do. But it's mostly just a "when in rome" thing, right now, I guess.
- Maybe we should, I'm not sure. I could write an easy patch for it at least.
You might be able to change the base of this PR to be your other branch, which will conveniently make it so that only the second commit appears here for review
- This is what a few of us do when we have "stacks" to submit where the individual pieces each warrant an individual submission, I know this is what Waleed does for instance.
- I say "might" because GitHub sometimes does weird things (in my experience working on nixpkgs) when you change the base branch. In this case I think it won't do anything odd.
- Normally we review the individual commits anyway, regardless of those choices, though. So there's nothing really stopping you from must submitting both of these changes in one PR, if you like.
- Or not. It's up to your better judgement, really...

Other than that, I'm a fan of long and explicative commit messages like yours, so I'll take a closer look shortly if someone else doesn't get to it. Just wanted to put it out there.

martinvonz · 2024-02-10T21:07:34Z

We don't enforce this at all. It's not hard to do. But it's mostly just a "when in rome" thing, right now, I guess.

I almost always do it, but I don't really care much about it. The goal, IMO, is to make the subject line concise and clear. I see the ": " as just a way of helping with that. It basically saves you from instead saying "In the code, ".

thoughtpolice · 2024-02-10T21:12:57Z

I almost always do it, but I don't really care much about it. The goal, IMO, is to make the subject line concise and clear. I see the ": " as just a way of helping with that. It basically saves you from instead saying "In the code, ".

Right. I think having us all do it definitely helps uphold a level of consistency too, which is mostly why I mention it. But we definitely have some deviations e.g. some changes inherently touch multiple crates, stuff like that.

On a related note there is some prior art in the Conventional Commits standard to help these sorts of concerns, but ultimately, I don't follow it to a T or anything. Just the syntax thing: description in some form is what I've ended up sticking to for 90% of my messages these days, unless I feel mentioning a specific component is useful, but for us I've not really done it.

epage · 2024-02-10T21:35:54Z

Looking over the issue, it looks like this isn't jj help alias but jj help <alias>, right?

As another point of comparison, cargo manually implements the help subcommand. Granted, it is also loading up man pages wherever possible.

https://github.com/rust-lang/cargo/blob/fc1d58fd0531a57a6b942a14cdcdbcb82ece16f3/src/bin/cargo/commands/help.rs#L21

epage · 2024-02-10T21:38:21Z

i tried to just stop using clap's API, which simplifies the code a lot and works with the new dynamically added alias subcommands. but that breaks global flags that precede the alias, things like jj --no-pager alias.

Don't worry, clap's built-in help doesn't deal with arguments as well. Something I would consider fixing at some point.

cli/src/cli_util.rs

epage · 2024-02-10T21:43:06Z

You might be able to change the base of this PR to be your other branch, which will conveniently make it so that only the second commit appears here for review

Doesn't that leave the PR in your own user repo, rather than martinvonz's?

On a related note there is some prior art in the Conventional Commits standard to help these sorts of concerns, but ultimately, I don't follow it to a T or anything. Just the syntax thing: description in some form is what I've ended up sticking to for 90% of my messages these days, unless I feel mentioning a specific component is useful, but for us I've not really done it.

I think that is more Linux like to prefix with the area. I've been wanting to get back to committed to improve some things. Maybe it'll also include jj support...

epage

For being the first time I've looked at this source code, I think this is likely fine. We'll see what I say if I stick with jj for several months :)

jyn514 · 2024-02-10T21:45:52Z

Looking over the issue, it looks like this isn't jj help alias but jj help <alias>, right?

right.

As another point of comparison, cargo manually implements the help subcommand. Granted, it is also loading up man pages wherever possible.

i don't think that would help here. it doesn't make the code much simpler, and it doesn't support aliases in jj util completion the way this PR does, because the help command doesn't integrate with clap_complete.

Don't worry, clap's built-in help doesn't deal with arguments as well. Something I would consider fixing at some point.

i think you're talking about jj --no-pager help? that already doesn't work before this PR, right. but i was talking about jj --no-pager <alias> - it works before this PR, but one of the approaches i tried regressed it. the current PR supports it at the cost of some additional complexity.

I think we typically want the first line of the commit message to be of the form thing: description where "thing" is typically something like the name of the crate your patch touches (cli/lib), or cargo:, or docs:, something to that effect. In this case both of your commits would be cli: blah blah...

oh right, my bad - i'll fix that real quick

martinvonz

Thanks!

cli/tests/test_alias.rs

cli/src/cli_util.rs

jyn514 · 2024-02-11T15:28:26Z

fixes #2866. built on #3006 for convenience, but i can separate it out if desired.

oh, that's right, i put this on top of #3006 because otherwise clap panics when i try to define help for an alias that has the same name as a built-in command. i can fix it, but the work is redundant if #3006 merges. going to wait to merge this until we decide on the approach there.

matts1 · 2024-03-07T01:17:37Z

cli/tests/test_alias.rs

+
+    let stdout = test_env.jj_cmd_success(&repo_path, &["help", "b"]);
+    insta::assert_snapshot!(stdout.lines().take(3).join("\n"), @r###"
+    Alias for ["log", "-r", "@", "-T", "branches"]


This can be a bit hard to parse as a human, particularly for someone coming fresh into jj.

How would you feel about alias for "jj log -r @ -T branches" or something along the lines of that? I think that would be nicer for default aliases in particular. A user just starting on jj who runs jj help checkout might not quite understand alias for ["new"], but I imagine they'd understand alias for "jj new" perfectly.

We might need to use shlex.try_join to ensure that we don't print out wierd stuff when the user writes an alias as ["command", "with spaces", "or", "with_function_call()"]

We should also use that library here:
https://github.com/martinvonz/jj/blob/462c19736a9b3293b4cc9957404504718e19ef84/cli/src/cli_util.rs#L1464-L1465

done. i haven't modified the existing code in start_repo_transaction because frankly i'm not sure how it works and i've already spent quite a while on this PR; i'd prefer to open a follow-up issue instead.

jyn514 · 2024-05-17T16:02:20Z

apparently clap_markdown doesn't support before_help :( maybe i could update the help to prepend the alias line manually instead.

i consider rewriting the `help` command, but that breaks some other things (e.g. tab completion doesn't work, because `clap_complete` doesn't know about the aliases). instead, i found that i can add each alias to the `clap::Command` dynamically at runtime. this works! the help it shows is useful and easy to modify, and it's only about 25 lines of code. the problem is this breaks `resolve_aliases` pretty badly. the replacement loop uses *clap's* API to determine the arguments that come after the alias, and clap does not expose a way to get the raw arguments, only the parsed arguments. it happened to work before because the commands were all `External`, in which case clap doesn't do any parsing. i tried to just stop using clap's API, which simplifies the code a lot and works with the new dynamically added alias subcommands. but that breaks global flags that precede the alias, things like `jj --no-pager alias`. instead, i waited to add the aliases to `Command` until after replacing the string arguments. that breaks because `expand_args()` gets called twice, once before and once after processing `--repository`. so even if i do this afterwards in the `resolve_aliases` function, the *next* call will still break because it parses differently. to work around *that*, i clone the app before passing it to `expand_args`, so that the modifications don't affect the --repository parsing. it's kind of hacky, but i haven't found a simpler solution. --- ## future work * `jj alias -h` shows the help for the *resolved* alias, not the "Alias for ..." short help. that happens because the alias is replaced with its definition before passing it to get_matches. it would be nice to make it consistent with `help`. * `jj help recursive-alias` will only "peel" one level of aliasing in the help; it would be nice to show the fully expanded command instead.

jyn514 · 2024-05-17T20:21:09Z

jj help without arguments shows builtin aliases in a different format than user-defined aliases (builtins look like [aliases: st] in the short help). i tried to make these consistent, but ran into trouble that i'm not sure is fixable.

never mind, i found a workaround

martinvonz · 2024-05-20T22:57:14Z

@jyn514, do you remember what the status of this PR is? I skimmed the comments above but it's not obvious to me if we're waiting for some changes from you or if it's ready and the reviewers forgot to review it.

jyn514 · 2024-05-21T09:08:10Z

@martinvonz this still breaks the util markdown-help subcommand. unfortunately we do not seem to test the way in which it breaks. i need to investigate, add tests, and either fix clap-markdown or change the way i implement the clap help.

jyn514 · 2024-05-23T13:43:29Z

just to document what's going on: here is a test case that makes CI fail with my current PR:

diff --git a/cli/tests/test_generate_md_cli_help.rs b/cli/tests/test_generate_md_cli_help.rs
index a752a09ef2...a1c9ce1dae 100644
--- a/cli/tests/test_generate_md_cli_help.rs
+++ b/cli/tests/test_generate_md_cli_help.rs
@@ -27,0 +27,1 @@
+    test_env.add_config(r#"aliases.foo = ["git", "fetch"]"#);

thread \'main\' panicked at /home/jyn/.local/lib/cargo/registry/src/index.crates.io-6f17d22bba15001f/clap-markdown-0.1.3/src/lib.rs:241:5:
assertion failed: command.get_before_help().is_none()

i've opened an issue upstream: ConnorGray/clap-markdown#21
in the meantime i plan to use a workaround in clap so i don't use before_help.

jyn514 · 2024-05-23T13:48:26Z

oh fun, clap's help is autogenerated and can't be overridden statically without duplicating their whole default template. i suppose that's why before_help exists in the first place.

so, i can either:

copy/paste lots of the guts of clap into our code, or
fix panics if before_help is set ConnorGray/clap-markdown#21. unfortunately that repo hasn't been updated in a couple years so we may have to fork it to get a response in a reasonable amount of time.

jyn514 force-pushed the push-pvuyvlzusnnw branch 2 times, most recently from 8c7bfab to 3e61bad Compare February 10, 2024 16:33

epage reviewed Feb 10, 2024

View reviewed changes

cli/src/cli_util.rs Outdated Show resolved Hide resolved

epage approved these changes Feb 10, 2024

View reviewed changes

jyn514 force-pushed the push-pvuyvlzusnnw branch from 3e61bad to 735c3ee Compare February 10, 2024 21:47

jyn514 changed the title ~~add support for jj help alias~~ add support for jj help <alias> Feb 10, 2024

martinvonz approved these changes Feb 10, 2024

View reviewed changes

cli/tests/test_alias.rs Show resolved Hide resolved

yuja reviewed Feb 11, 2024

View reviewed changes

cli/src/cli_util.rs Outdated Show resolved Hide resolved

cli/src/cli_util.rs Outdated Show resolved Hide resolved

yuja reviewed Feb 11, 2024

View reviewed changes

cli/src/cli_util.rs Show resolved Hide resolved

jyn514 force-pushed the push-pvuyvlzusnnw branch from 735c3ee to 5070a4c Compare February 11, 2024 14:11

jyn514 mentioned this pull request Feb 11, 2024

give a warning when redefining a built-in command #3006

Merged

4 tasks

ilyagr mentioned this pull request Mar 7, 2024

cli: deprecate jj checkout and jj merge #2842

Merged

3 tasks

matts1 reviewed Mar 7, 2024

View reviewed changes

jyn514 force-pushed the push-pvuyvlzusnnw branch from 5070a4c to bdfcd0f Compare May 17, 2024 15:34

This comment was marked as resolved.

Sign in to view

jyn514 force-pushed the push-pvuyvlzusnnw branch from bdfcd0f to 76e8b73 Compare May 17, 2024 15:56

jyn514 force-pushed the push-pvuyvlzusnnw branch from 76e8b73 to 5b8b6f2 Compare May 17, 2024 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for `jj help <alias>` #3015

add support for `jj help <alias>` #3015

jyn514 commented Feb 10, 2024 •

edited

Loading

thoughtpolice commented Feb 10, 2024

martinvonz commented Feb 10, 2024

thoughtpolice commented Feb 10, 2024

epage commented Feb 10, 2024

epage commented Feb 10, 2024

epage commented Feb 10, 2024

epage left a comment

jyn514 commented Feb 10, 2024

martinvonz left a comment

jyn514 commented Feb 11, 2024

matts1 Mar 7, 2024 •

edited

Loading

martinvonz Mar 7, 2024

jyn514 May 17, 2024

This comment was marked as resolved.

jyn514 commented May 17, 2024

jyn514 commented May 17, 2024

martinvonz commented May 20, 2024

jyn514 commented May 21, 2024

jyn514 commented May 23, 2024

jyn514 commented May 23, 2024

add support for jj help <alias> #3015

Are you sure you want to change the base?

add support for jj help <alias> #3015

Conversation

jyn514 commented Feb 10, 2024 • edited Loading

Checklist

thoughtpolice commented Feb 10, 2024

martinvonz commented Feb 10, 2024

thoughtpolice commented Feb 10, 2024

epage commented Feb 10, 2024

epage commented Feb 10, 2024

epage commented Feb 10, 2024

epage left a comment

Choose a reason for hiding this comment

jyn514 commented Feb 10, 2024

martinvonz left a comment

Choose a reason for hiding this comment

jyn514 commented Feb 11, 2024

matts1 Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

martinvonz Mar 7, 2024

Choose a reason for hiding this comment

jyn514 May 17, 2024

Choose a reason for hiding this comment

This comment was marked as resolved.

jyn514 commented May 17, 2024

jyn514 commented May 17, 2024

martinvonz commented May 20, 2024

jyn514 commented May 21, 2024

jyn514 commented May 23, 2024

jyn514 commented May 23, 2024

add support for `jj help <alias>` #3015

add support for `jj help <alias>` #3015

jyn514 commented Feb 10, 2024 •

edited

Loading

matts1 Mar 7, 2024 •

edited

Loading