Conversation
1 - clarify the documentation
2 - make curl exit with an error for unknown protocols to make users
better notice when they get it wrong
Verified in test 322
There was a problem hiding this comment.
Pull request overview
This PR updates --proto handling to better communicate misuse: documentation clarifies modifier usage and the CLI now treats unknown protocol tokens as a fatal parameter error (validated with a new regression test).
Changes:
- Add test322 to ensure
--protowith multiple modifiers fails with exit code 2 and expected stderr. - Change
--protoparsing to error out (not just warn) on unrecognized protocol tokens. - Clarify
--protodocumentation to indicate a single modifier (rather than “zero or more”).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/data/test322 | New regression test for --proto with multiple modifiers (expects PARAM_BAD_USE). |
| tests/data/Makefile.am | Registers test322 in the test list. |
| src/tool_paramhlp.c | Makes unknown protocol tokens in --proto a hard error (PARAM_BAD_USE). |
| docs/cmdline-opts/proto.md | Adjusts wording about modifier count (but still needs a follow-up doc fix—see comment). |
Suppressed comments (1)
docs/cmdline-opts/proto.md:25
- The text later in this document still says unknown/disabled protocols only produce a warning, but the implementation now errors out and exits (see updated proto2num behavior and test322 expectations). Update the documentation to match the new fatal-error behavior.
prefixed by a modifier. Available modifiers are:
## +
Permit this protocol in addition to protocols already permitted (this is
the default if no modifier is used).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Analysis of PR #22687 at ac295499: Test 322 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 12 different CI jobs (the link just goes to one of them). Generated by Testclutch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1 - clarify the documentation
2 - make curl exit with an error for unknown protocols to make users
better notice when they get it wrong
Verified in test 322