fix(importer-curl): read combined short flags as separate options - #577
Open
NgoQuocViet2001 wants to merge 1 commit into
Open
fix(importer-curl): read combined short flags as separate options#577NgoQuocViet2001 wants to merge 1 commit into
NgoQuocViet2001 wants to merge 1 commit into
Conversation
`curl -fsSL https://example.com` imported with a URL of `sSL`. The tokenizer split any single-dash token longer than two characters after the first character, on the assumption it was a flag with a squished value like `-XPOST`. curl instead reads a short cluster left to right, one option per character, until an option that takes a value — that one swallows the rest. `-fsSL` is four boolean flags, so splitting after `-f` left `sSL` as a positional argument, and the first positional is what the importer reads as the URL. The real URL was dropped. `-fsSL` is how most install instructions invoke curl, and `-sS` in scripts, so the imported request pointed at a fragment of the flag cluster rather than anywhere real. Single flags were unaffected, which is why the existing cases pass. Expand a cluster by walking its characters, ending at the first flag that takes a value. Which short flags those are is derived from SUPPORTED_FLAGS minus BOOLEAN_FLAGS, so it does not drift from the tables above it. `-XPOST` still resolves to `-X POST`, and `-sSXPOST` now resolves to `-s -S -X POST`.
Greptile SummaryThe PR replaces the unconditional first-character split for compact cURL options with short-cluster expansion that stops when a supported value-taking option is encountered.
Confidence Score: 5/5The PR appears safe to merge, with the changed parser addressing the reported short-flag cluster failures without an identified changed-code regression. The expansion now emits each boolean short option separately and stops at supported value-taking options, preserving the intended URL and method in the covered command forms.
|
| Filename | Overview |
|---|---|
| plugins/importer-curl/src/index.ts | Replaces unconditional compact-option splitting with cURL-style short-cluster expansion derived from the existing supported-option tables. |
| plugins/importer-curl/tests/index.test.ts | Adds regression tests for an all-boolean cluster and a cluster ending in an attached method value. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Compact short-option token] --> B{Current option takes a value?}
B -->|No| C[Emit option]
C --> D{Characters remain?}
D -->|Yes| B
D -->|No| E[Continue with next token]
B -->|Yes| F[Emit option]
F --> G{Attached remainder exists?}
G -->|Yes| H[Emit remainder as value]
G -->|No| E
Reviews (1): Last reviewed commit: "fix(importer-curl): read combined short ..." | Re-trigger Greptile
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.
curl -fsSL https://example.comimports with a URL ofsSL. The real URL is dropped.Why
The tokenizer split any single-dash token longer than two characters after its first character, assuming a squished value:
That is right for
-XPOSTbut wrong for a cluster of boolean flags. curl reads a short cluster left to right, one option per character, until an option that takes a value — that one swallows the remainder.-fsSLis four boolean flags, so splitting after-fleavessSLas a positional argument.-f,-s,-Sand-Lare not inSUPPORTED_FLAGS, so they are skipped, andsSLlands insingletons— wheresingletons[0]is read as the URL:-fsSLis how most install instructions invoke curl and-sSis common in scripts, so a pasted command silently imports a request pointing at a fragment of the flag cluster. Single flags were never affected, which is why the existing cases pass.The change
expandShortFlagswalks the cluster and stops at the first flag that takes a value, which then consumes the rest:-fsSL→-f -s -S -L-XPOST→-X POST(unchanged)-sSXPOST→-s -S -X POST-Xalone →-X, so the existing "next token is the value" path still appliesWhich short flags take a value is derived rather than restated, so it cannot drift from the tables above it:
That resolves to
b d F H X utoday.importCommand's own squished-argument branch is left alone — it no longer sees a squished token from this path, but it is harmless as a second reading.Tests
Two cases in
tests/index.test.ts: a pure boolean cluster, and a cluster ending in a value flag. Negative control — restoring the old splitter fails exactly those two and leaves the other 52 green:With the fix,
vp test --run testsinplugins/importer-curl: 54 passed (54), 2 files.oxlintreports nothing for the two changed files.Adjacent, not fixed here
An unsupported flag that takes a value still leaves its value as a positional, so
curl -o out.txt https://example.comimports with the URLout.txt. That is the same "first positional becomes the URL" path, but closing it needs a table of curl's value-taking options rather than a parsing rule, so I left it out of this change. Happy to follow up if you want it.I did not run
vp fmt— both files already fail--checkon a cleanmainin my environment (it reports "No config found, using defaults"), so reformatting would have buried the fix in unrelated churn. The pre-commit hook'svp check --fixdid run over the two staged files.