add support for `jj help <alias>`

[jyn514]

fixes https://github.com/martinvonz/jj/issues/2866. built on https://github.com/martinvonz/jj/pull/3006 for convenience, but i can separate it out if desired.

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:

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

[thoughtpolice]

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]

• 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]

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]

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]

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.

[epage]

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

[jyn514]

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

[jyn514]

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.

[jyn514]

since @epage has already commented on this PR, i'd like to pull out this note from the commit message - do you have suggestions on how to fix this part?

• 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.

[jyn514]

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

[jyn514]

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]

@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]

@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]

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: https://github.com/ConnorGray/clap-markdown/issues/21 in the meantime i plan to use a workaround in clap so i don't use before_help.

[jyn514]

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 https://github.com/ConnorGray/clap-markdown/issues/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.