Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix halt_error to print message without prefix in raw mode (fix #1902) #2632

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

itchyny
Copy link
Contributor

@itchyny itchyny commented Jun 26, 2023

This PR fixes #1902. The manual says that halt_error should print the input with no decoration . This is a regression of 2b4d51f, which is after 1.6.

@itchyny itchyny self-assigned this Jun 26, 2023
@itchyny itchyny added the bug label Jun 26, 2023
@itchyny itchyny added this to the 1.7 release milestone Jun 26, 2023
@wader
Copy link
Member

wader commented Jun 26, 2023

This got me thinking if we want halt_error to be able to output raw strings? ex:

$ jq -rn '"a\u0000b" | halt_error' 2>&1 | hexdump -C
00000000  6a 71 3a 20 65 72 72 6f  72 3a 20 61              |jq: error: a|
0000000c
$ jq -rn '"a\u0000b"' | hexdump -C
00000000  61 00 62 0a                                       |a.b.|
00000004

@itchyny
Copy link
Contributor Author

itchyny commented Jun 26, 2023

@wader Good catch, let's do so. The manual says as raw output.

@itchyny itchyny force-pushed the fix-halt-error-output branch from 8d68b83 to f6f2a7b Compare June 26, 2023 14:16
fprintf(stderr, "jq: error: %s", jv_string_value(error_message));
// No prefix should be added to the output of `halt_error`.
fwrite(jv_string_value(error_message), 1,
jv_string_length_bytes(jv_copy(error_message)), stderr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we don't leak the jv copy here as jv_string_length_bytes free/unref? I will have to read up on how the ownership works for the jv type and functions, eg why does jv_string_value not free/unref?

@wader
Copy link
Member

wader commented Jun 26, 2023

@itchyny 👍 good catch on notice the manual say raw string

fi
if [ "$($VALGRIND $Q $JQ -n '"x\u0000y\u0000z" | halt_error(1)' 2>&1)" != "$(printf 'x\x00y\x00z')" ]; then
echo "jq halt_error(1) had unexpected output" 1>&2
exit 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems linux build is not happy about this:

+ /home/runner/work/jq/jq/tests/../jq -n halt_error(1)
+ [ 1 -ne 1 ]
+ /home/runner/work/jq/jq/tests/../jq -n halt_error(11)
+ [ 11 -ne 11 ]
+ /home/runner/work/jq/jq/tests/../jq -n halt_error(1)
+ [ -n  ]
+ /home/runner/work/jq/jq/tests/../jq -n "xyz\n" | halt_error(1)
+ [ -n  ]
+ /home/runner/work/jq/jq/tests/../jq -n "xy" | halt_error(1)
+ echo z
+ [ xyz != xyz ]
+ /home/runner/work/jq/jq/tests/../jq -n "x\u0000y\u0000z" | halt_error(1)
+ printf x\x00y\x00z
+ [ xyz != x\x00y\x00z ]
+ echo jq halt_error(1) had unexpected output
jq halt_error(1) had unexpected output
+ exit 1

Hmm zero bytes gone from LHS value? shell doing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, NUL bytes are trimmed in command substitution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, any idea why macOS build was fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bash is old. It passes if Bash is 3.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 that explains things, great work!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If necessary, \0 can be used instead of \x00. That would work in any version bash or POSIX-compliant shell.

@itchyny itchyny force-pushed the fix-halt-error-output branch from f6f2a7b to d7839de Compare June 26, 2023 22:21
@itchyny
Copy link
Contributor Author

itchyny commented Jun 26, 2023

Thank you.

@itchyny itchyny changed the title Fix halt_error to print message without prefix (fix #1902) Fix halt_error to print message without prefix in raw mode (fix #1902) Jun 26, 2023
@itchyny itchyny merged commit 98835ee into jqlang:master Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

halt_error prints "jq: error:" in master
3 participants