Fix crash when prefix of "lock" subcommand is used

[gridbugs]

No description provided.

[Leonidas-from-XIV]

Could you explain how to get to this error? I can't figure out how this would trigger (the original code uses assert false probably for this exact reason - the code is believed to be unreadable).

[Leonidas-from-XIV]

I think this would lead to a weird case where opam-monorepo locksmith goes into this case.

I am somewhat confused how you got into this place, since I would've expected opam-monorepo locksmith not resolving to this module to start with.

[gridbugs]

It will only match prefixes of "lock", not strings which "lock" is a prefix of. cmdliner accepts prefixes of subcommands already. This code here is a sanity check that the command was a opam monorepo lock subcommand when saving the command to the lockfile.

[Leonidas-from-XIV]

Ah, you're right, I've misread the code. Honestly, I think the previous code is fine and we/Cmdliner should not accept prefix matches. If the user uses opam monorepo lo and we introduce a new command that starts with lo then suddenly things that worked don't anymore because it is impossible to disambiguate. Which can be annoying in e.g. scripts like the CI.

Edit: Heh, we already have a conflict in l, so lo is required anyway.

[gridbugs]

Could you explain how to get to this error? I can't figure out how this would trigger (the original code uses assert false probably for this exact reason - the code is believed to be unreadable).

You can trigger this by running opam monorepo loc. The output looks like it will work but then it crashes right at the end (presumably when it tries to add the command to the lockfile). For example:

$ opam monorepo loc
opam-monorepo: [WARNING] Package x is defined multiple times in the repository:
- /home/s/src/opam-monorepo/tmp/x.opam
- /home/s/src/opam-monorepo/test/bin/error-on-directory-conflict.t/x.opam
We kept /home/s/src/opam-monorepo/test/bin/error-on-directory-conflict.t/x.opam and discarded the others
==> Using 1 locally scanned package as the target.
[WARNING] Unknown variable "ocaml-system:version"
[WARNING] Unknown variable "ocaml-base-compiler:version"
[WARNING] Unknown variable "ocaml-variants:version"
==> Found 53 opam dependencies for the target package.
==> Querying opam database for their metadata and Dune compatibility.
==> Calculating exact pins for each of them.
opam-monorepo: internal error, uncaught exception:
               File "cli/lock.ml", line 519, characters 9-15: Assertion failed
               Raised at Duniverse_cli__Lock.raw_cli_args in file "cli/lock.ml", line 519, characters 9-21
               Called from Duniverse_cli__Lock.run in file "cli/lock.ml", line 554, characters 17-32
               Called from Cmdliner_term.app.(fun) in file "duniverse/cmdliner/src/cmdliner_term.ml", line 24, characters 19-24
               Called from Cmdliner_term.app.(fun) in file "duniverse/cmdliner/src/cmdliner_term.ml", line 22, characters 12-19
               Called from Cmdliner_eval.run_parser in file "duniverse/cmdliner/src/cmdliner_eval.ml", line 34, characters 37-44

[Leonidas-from-XIV]

It seems like the prefix matching of Cmdliner can't be disabled at the moment, so I created a ticket: dbuenzli/cmdliner#178

[Leonidas-from-XIV]

I'm ok with merging this if you think that this fix is useful in the meantime, the code itself is fine.

[gridbugs]

I think I agree that I'd rather not allow any prefixes for subcommands. Until we can totally disable prefixes in cmdliner I think this change is still valuable as it's currently confusing that opam monorepo lo fails in the way it does (it should either immediately fail due to an invalid subcommand or completely work), especially when all other subcommands can be prefixed.

[Leonidas-from-XIV]

Turns out the change in Cmdliner was refused, so we'd have to do our own code to disable this behavior :-(