Accept early args after help command

[Grillo-0]

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

[martinvonz]

Thanks!

By the way, this is a good step towards #2866, in case you want to work on that next :)

[Grillo-0]

Thanks!

By the way, this is a good step towards #2866, in case you want to work on that next :)

Interesting, I'll check it out ;)

[yuja]

Just an idea. Maybe we could use a hack similar to resolve_aliases()? Something like:

1. walk subcommands with allow_external_subcommands(true).disable_help_subcommand(true)
2. if command_name is "help", stop there and find "--no-pager"
3. move "--no-pager" earlier and reparse args normally

[Grillo-0]

I used git add -f to add the new snapshots. Please tell me if this is not the intend way to do this.

[Grillo-0]

Just an idea. Maybe we could use a hack similar to resolve_aliases()? Something like:

1. walk subcommands with `allow_external_subcommands(true).disable_help_subcommand(true)`

2. if `command_name` is `"help"`, stop there and find `"--no-pager"`

3. move `"--no-pager"` earlier and reparse args normally

This could work, it is a smaller change I think. I can change to this if you guys think it's better.

[martinvonz]

I used git add -f to add the new snapshots. Please tell me if this is not the intend way to do this.

Those should not have to be committed. You should be able to just delete them.

This could work, it is a smaller change I think. I can change to this if you guys think it's better.

Sounds good to me, thanks.

[Grillo-0]

I used git add -f to add the new snapshots. Please tell me if this is not the intend way to do this.

Those should not have to be committed. You should be able to just delete them.

Strange, I did this because the CI was failing due to the lack of those snapshots

[martinvonz]

Strange, I did this because the CI was failing due to the lack of those snapshots

Ah, it's the insta::assert_snapshot!(). We use inline snapshots instead. Search for , @" and you'll find lots of examples. However, maybe we don't actually need to test the full output? You already test that the output matches the --help output. Maybe that's sufficient?

[martinvonz]

Sorry about the delay! I blame it on the travel and week full of meetings. I'll review this now.

[yuja]

The current state looks good to me, thanks!

[Grillo-0]

Can I merge this?

[ilyagr]

I think this may have broken jj git help push. jj help git push still works.

If there is no easy fix, that's probably OK. If there is, it might be worth fixing. (I haven't read the long discussion in here yet, so I don't know if it was already discussed)

I'm used to this working from other clap programs, I don't know how many people would feel this way. OTOH, it shouldn't be that hard to get used to it.

Update: I see it in the changelog, so I'm guessing it was discussed. So, never mind, I'll post something else if I have more thoughts.

[ilyagr]

I'll also add a reference to a slightly more problematic bug (IMO), #4746. Yuya pointed me to read the discussion here for that; I have not done that yet, but I'm guessing there might not be an easy fix for that.

Update: I just realized this PR is what made it possible for --no-pager to work at all in the first place. Thank you!

[ilyagr]

Another problem is that jj help <tab> completion no longer works, at least in fish. I wonder if it'd be possible to have a clap option to keep the command, but only for completion purposes.

I'm not sure whether to promote this to its own issue as a bug/regression. I do like jj help <tab> for exploring docs. #4737 could also help with this.

[Grillo-0]

Update: I see it in the changelog, so I'm guessing it was discussed. So, never mind, I'll post something else if I have more thoughts.

Yeah, it was discussed, here is the discussion:
#4518 (comment)

[Grillo-0]

Another problem is that jj help <tab> completion no longer works, at least in fish. I wonder if it'd be possible to have a clap option to keep the command, but only for completion purposes.

I'm not sure whether to promote this to its own issue as a bug/regression. I do like jj help <tab> for exploring docs. #4737 could also help with this.

Good catch! For me, it is a regression.

Another way to solve this would be to list all the possible commands to show help with value_parser (like it was suggested by @yuja on #4630 (comment)).

[Grillo-0]

I'll also add a reference to a slightly more problematic bug (IMO), #4746. Yuya pointed me to read the discussion here for that; I have not done that yet, but I'm guessing there might not be an easy fix for that.

Maybe we could make the help flags boolean flags that call the new help command if they are present?

[yuja]

Another way to solve this would be to list all the possible commands to show help with value_parser (like it was suggested by @yuja on #4630 (comment)).

I guess it can't handle multi-level subcommands?

I think dynamic completion is the way to go. Maybe we could also cheat that clap's help command exists only in "util completion", but that would disable completion for "help -k ".

I'll also add a reference to a slightly more problematic bug (IMO), #4746. Yuya pointed me to read the discussion here for that; I have not done that yet, but I'm guessing there might not be an easy fix for that.

Maybe we could make the help flags boolean flags that call the new help command if they are present?

I don't know if clap's -h/--help handling is intuitive, but I wouldn't add messy workaround for this small problem. -h terminates parsing, which I think is okay?

[ilyagr]

I don't know if clap's -h/--help handling is intuitive, but I wouldn't add messy workaround for this small problem.

I am still not up to date on exactly how messy different workarounds are, and whether this can be fixed in jj as opposed to clap, but I put a couple of thoughts about why I think this problem matters in #4746 (comment) . I wouldn't call it a major issue that we should panic about, but I wouldn't call it a minor issue either.

For example, in my mind, it is more serious than the issue of tab-completing help, which is inconvenient but is not confusing to beginners.

-h terminates parsing, which I think is okay?

I think it'd be OK if we taught people about it. For example, we could say in the beginning or end of the help text:

$ jj rebase --help --no-pager -d abracadabra
Warning. the following arguments were ignored: `--no-pager -d abracadabra`.
<Normal help text follows>

This would address the beginner problem (people would still be able to learn that the order of arguments is meant to be irrelevant, since they see jj warn them in the exceptional cases when it is relevant), but it feels a bit too annoying in general. So, I think making the order irrelevant would be better (if possible).