Parse short options in order given #3194

thaliaarchi · 2024-10-29T04:51:33Z

Fixes:

Short options cannot be repeated in the same group: e.g., -nn fails, but --null-input --null-input works
Short options are processed in an arbitrary fixed order, not the order given by the user: e.g., -hV, -Vh, -h -V, and --help --version are processed as -h, but -V -h and --version --help are processed as -V
-L cannot be a part of a short option group (as the last one): e.g., -nLlib and -nL lib fail

Questions:

-L is the only option without a long form and, with this refactor, it would be easy to add. What should it be called?
- --module: this matches the terminology in the manual, but doesn't match the L mnemonic
- --library: this matches -L
- --import-lib: -L doesn't actually import, but rather adds to the search path
- --library-path: this is the conventional flag used by linkers, the inspiration for this option
- --library-dir: avoids confusion with thinking it's the filename of a .jq file or a :-separated path
Needs regression tests for the above fixes. Where should they go?

wader

Nice cleanup 👍

Needs regression tests for the above fixes. Where should they go?

As tests/shtest is already quite crowded (should maybe be split up?) i think a new "shtest" file might be a good idea

src/main.c

wader · 2024-10-30T08:07:43Z

src/main.c

-      // check for unknown options... if this argument was a short option
-      if (strlen(argv[i]) != short_opts + 1) {
-        fprintf(stderr, "%s: Unknown option %s\n", progname, argv[i]);
-        die();


Hmm not sure i follow how repeated short option end up here in current code :)

For short options, the way the current code processes flags is by checking in order: Does it have L? Does it have s? Does it have r? etc. For each of those checks, it scans with strchr until the first occurrence of the short option and increments the count int short_opts. That means multiple occurrences of the same flag under-count and leave short_opts <= strlen(argv[i]).

Either a loop around the option checks is needed, or a loop around the strchr scanning. Since some options have effects which matter when processed in a different order (e.g., -h and -V), it is more correct to process short options in order from the front just like long options.

thaliaarchi · 2024-10-30T11:15:25Z

As tests/shtest is already quite crowded (should maybe be split up?) i think a new "shtest" file might be a good idea

I ended up adding tests to tests/shtest, since it seems to fit with other regression tests there. Splitting shtest ought to be a separate PR, if we do it.

thaliaarchi · 2024-10-30T11:30:57Z

On further thought, I also like --library-dir, since it avoids confusion with thinking it's the filename of a .jq file or a :-separated path.

This option evidently comes from the tradition of ld. GNU ld, mold, and gold have a corresponding --library-path long option and a similar, but unrelated -l/--library option. lld and rustc have only -L. For consistency, I'll stick with --library-path.

wader · 2024-10-30T20:44:30Z

As tests/shtest is already quite crowded (should maybe be split up?) i think a new "shtest" file might be a good idea

I ended up adding tests to tests/shtest, since it seems to fit with other regression tests there. Splitting shtest ought to be a separate PR, if we do it.

Good with me

thaliaarchi · 2024-10-31T10:26:29Z

Just pushed a small fix for insufficient atoi error handling for --indent.

emanuele6 · 2024-10-31T12:47:01Z

src/main.c

+          further_args_are_strings = 0;
+          further_args_are_json = 1;
+        } else if (isoption(&text, 0, "arg", is_short)) {
+          // FIXME: For --arg and --argjson we should check that the varname is acceptable


This FIXME is wrong: we most definitely should not do that. I made a PR to remove it, but then it wasn't merged.

Can we at least not readd this FIXME with an updated text? ^^

I was thinking of removing it for the same reason, since the comment predates $ARGS. However, I looked into it and saw that syntactically invalid names are still added to the bytecode. Should there be validation for that or does it not matter?

Turns out I was wrong in assuming that named args are in the bytecode of a program, so I see no need to add validation:

$ jq -n 42 --arg 'bad!' 12345 --debug-dump-disasm 0000 TOP 0001 LOADK 42 0003 RET 42

I've pushed a commit removing the comment and updating the manual, and I reverted the change to the text of the comment in my refactor.

We should be good to merge now!

Fixes: * Short options cannot be repeated in the same group: e.g., `-nn` fails, but `--null-input --null-input` works * Short options are processed in an arbitrary fixed order, not the order given by the user: e.g., `-hV`, `-Vh`, `-h -V`, and `--help --version` are processed as `-h`, but `-V -h` and `--version --help` are processed as `-V` * `-L` cannot be a part of a short option group (as the last one): e.g., `-nLlib` and `-nL lib` fail

-L is the only option without a long form.

thaliaarchi · 2024-11-02T00:58:52Z

I've dropped commit “Handle input errors for --indent” from the patch series, because I found a similar number parsing bug with tonumber. It will appear in a separate PR shortly.

How much reviewer consensus is needed for a merge? I'm ready on my end.

When jq is not supplied with a program, it usually exits with an error and usage text, except for when jq is not connected to a TTY (i.e., not interactive), in which case a default program of `.` is used. Combined with `-f`, it then tries to read `.` as the filename. $ jq jq - commandline JSON processor [version 1.7.1] … $ jq -f jq - commandline JSON processor [version 1.7.1] … $ jq | cat ^C $ jq -f | cat jq: Could not open .: It's a directory

This comment predates the addition of $ARGS in jq 1.6, so there was no way to access named arguments with an invalid identifier. Rather than forbid such names, update the manual to clarify.

thaliaarchi force-pushed the option-fixes branch from 87fb89e to 84f0b4f Compare October 29, 2024 04:55

thaliaarchi changed the title ~~Parse CLI options in order given~~ Parse short options in order given Oct 29, 2024

thaliaarchi force-pushed the option-fixes branch from 84f0b4f to 192e821 Compare October 29, 2024 07:06

wader reviewed Oct 30, 2024

View reviewed changes

thaliaarchi force-pushed the option-fixes branch from 00b2cd0 to f12c2bd Compare October 30, 2024 11:13

thaliaarchi force-pushed the option-fixes branch 2 times, most recently from 0c93d7e to a81f64a Compare October 30, 2024 11:30

thaliaarchi force-pushed the option-fixes branch 2 times, most recently from 40a20f8 to 0c93d7e Compare October 30, 2024 11:53

wader approved these changes Oct 30, 2024

View reviewed changes

wader requested review from itchyny, nicowilliams and emanuele6 October 30, 2024 20:46

thaliaarchi force-pushed the option-fixes branch from 0c93d7e to b1e9739 Compare October 31, 2024 10:25

emanuele6 reviewed Oct 31, 2024

View reviewed changes

thaliaarchi added 2 commits November 1, 2024 02:47

Add --library-path long option for -L

c9f59f7

-L is the only option without a long form.

thaliaarchi force-pushed the option-fixes branch from dc2a5ce to 6d42a4e Compare November 1, 2024 09:47

thaliaarchi requested a review from emanuele6 November 1, 2024 09:53

wader approved these changes Nov 1, 2024

View reviewed changes

thaliaarchi force-pushed the option-fixes branch from 6d42a4e to b276a86 Compare November 2, 2024 00:54

thaliaarchi mentioned this pull request Nov 2, 2024

Fix whitespace in number parsing #3195

Open

thaliaarchi added 3 commits November 8, 2024 09:43

Remove dead flag

1e916ed

Remove outdated FIXME

01ff8af

This comment predates the addition of $ARGS in jq 1.6, so there was no way to access named arguments with an invalid identifier. Rather than forbid such names, update the manual to clarify.

thaliaarchi force-pushed the option-fixes branch from b276a86 to 01ff8af Compare November 8, 2024 17:44

emanuele6 approved these changes Nov 8, 2024

View reviewed changes

emanuele6 merged commit eb9bdca into jqlang:master Nov 8, 2024
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse short options in order given #3194

Parse short options in order given #3194

thaliaarchi commented Oct 29, 2024 •

edited

Loading

wader left a comment

wader Oct 30, 2024

thaliaarchi Oct 30, 2024

thaliaarchi commented Oct 30, 2024 •

edited

Loading

thaliaarchi commented Oct 30, 2024 •

edited

Loading

wader commented Oct 30, 2024

thaliaarchi commented Oct 31, 2024

emanuele6 Oct 31, 2024

emanuele6 Oct 31, 2024

thaliaarchi Oct 31, 2024 •

edited

Loading

thaliaarchi Nov 1, 2024 •

edited

Loading

thaliaarchi commented Nov 2, 2024

Parse short options in order given #3194

Parse short options in order given #3194

Conversation

thaliaarchi commented Oct 29, 2024 • edited Loading

wader left a comment

Choose a reason for hiding this comment

wader Oct 30, 2024

Choose a reason for hiding this comment

thaliaarchi Oct 30, 2024

Choose a reason for hiding this comment

thaliaarchi commented Oct 30, 2024 • edited Loading

thaliaarchi commented Oct 30, 2024 • edited Loading

wader commented Oct 30, 2024

thaliaarchi commented Oct 31, 2024

emanuele6 Oct 31, 2024

Choose a reason for hiding this comment

emanuele6 Oct 31, 2024

Choose a reason for hiding this comment

thaliaarchi Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

thaliaarchi Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

thaliaarchi commented Nov 2, 2024

thaliaarchi commented Oct 29, 2024 •

edited

Loading

thaliaarchi commented Oct 30, 2024 •

edited

Loading

thaliaarchi commented Oct 30, 2024 •

edited

Loading

thaliaarchi Oct 31, 2024 •

edited

Loading

thaliaarchi Nov 1, 2024 •

edited

Loading