- Notifications
You must be signed in to change notification settings - Fork1.6k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Validate JSON for --jsonarg#2658
Conversation
| fprintf(stderr, "%s: invalid JSON text passed to --jsonargs\n", progname); | ||
| die(); | ||
| } | ||
| ARGS = jv_array_append(ARGS, v); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A bit of duplication here. I guess one could extract to some kind of parse_or_die function? could be use for--argjson code also. Maybe for later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nah, it's fine.
| exit 1 | ||
| fi | ||
| # this tests the args_done code path "--" | ||
| $JQ -n --jsonargs null -- invalid && EC=$? || EC=$? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure if this should be exit 2 (usage error?) or invalid input error?
I also noticed that we skip using $VALGRIND is some places, should we fix that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We're not very consistent regarding exit status codes. It'd be nice if a) we documentedjq's exit codes besides in the-e case, b) maybe made sure that the various usage and system error exit codes differ from the ones for-e?. But that's not for this issue.
And yes, we should fix the non-use of valgrind, but we should do so separately.
Thanks! |
Fixes#2572