Repository navigation
glob: reduce the number of allocations for URLs that don't need globbing - #22947
Conversation
acec0de to
acec0de
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The upload ownership issue remains unresolved, along with the bracketed literal URL edge case.
Pull request overview
This pull request reduces allocations for URLs that do not require glob expansion by retaining them as literals.
Changes:
- Adds literal URL tracking and handling.
- Updates glob activity and cleanup logic.
- Adds regression test 2415.
File summaries
| File | Summary |
|---|---|
tests/data/test2415 |
Adds regression coverage for named glob references with literal URLs. |
tests/data/Makefile.am |
Registers the new test. |
src/tool_urlglob.h |
Adds literal URL state. |
src/tool_urlglob.c |
Implements the literal URL fast path; retains a moderate issue with upload-file ownership and a nit involving bracketed literal URLs. |
Review details
Suppressed comments (2)
src/tool_urlglob.c:584
- This shortcut only looks for the presence of bracket characters, so valid non-glob URLs such as
http://[::1]/(and[]literals) still take the allocation-heavy parser path.glob_parse()explicitly recognizes these as literal components viapeek_ipv6()/the empty-bracket case, so please extend the no-glob detection to skip those forms as well if this optimization is intended for all URLs that do not expand.
if(!strpbrk(url, "{}[]")) {
/* no glob syntax: borrow the URL instead of parsing a copy of it */
glob->literal = url;
src/tool_urlglob.c:638
- Because
glob_url()is also used for upload filenames, this branch makes every plain-T filego throughglob_next_url().setup_input_file()previously movedu->infileintostate->uploadfilewhen no glob was active, butliteralmakesglob_inuse()true, so thiscurlx_strdup()adds an avoidable allocation and copy for every non-glob upload. Please preserve the ownership fast path for literal upload files or limit this state change to URL patterns.
*globbed = curlx_strdup(glob->literal);
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Analysis of PR #22947 at acec0dea: Test 3231 failed, but it has been 69.6% flaky lately, so it's probably NOT a fault of the PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (4, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Generated by Testclutch |
acec0de to
acec0de
Compare
acec0de to
acec0de
Compare
For URLs that don't need glob-expansion, copy the URL pointer as a literal.