Skip to content

tool: buffer headers before remote filename selection - #22977

Closed
bagder wants to merge 3 commits into
masterfrom
bagder/josh-103
Closed

bagder wants to merge 3 commits into
masterfrom
bagder/josh-103

Conversation

@bagder

@bagder bagder commented Sep 17, 2026

Copy link
Copy Markdown
Member

No description provided.

@github-actions github-actions Bot added the tests label Sep 17, 2026
@bagder
bagder marked this pull request as ready for review September 17, 2026 06:47
@bagder
bagder requested a lite review from Copilot September 17, 2026 06:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Header buffering can duplicate headers and discard buffered headers for some error responses.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR buffers response headers until remote filename selection is complete, with regression coverage for Early Hints and ETag handling.

Changes:

  • Adds deferred header buffering.
  • Handles 103 responses and ETag-saving behavior.
  • Registers tests 3037 and 3038.
File summaries
File Changes
src/tool_cb_hdr.c Buffers headers during filename selection.
tests/data/test3037 Tests Early Hints behavior.
tests/data/test3038 Tests ETag and header buffering.
tests/data/Makefile.am Registers the new tests.
Review details

Suppressed comments (1)

src/tool_cb_hdr.c:518

  • This also buffers headers for 4xx/5xx responses, even though the content_disposition() branch is deliberately skipped for those response classes and cannot select a filename. For a zero-length error response (for example, 404 Content-Length: 0), the write callback never runs, so the output file is never opened and the buffered -i headers are discarded during cleanup. Restrict this deferral to response classes that can still complete the filename decision (including 1xx for Early Hints), or create/flush the output at the end of headers for error responses.
    if(hdrcbdata->honor_cd_filename)
      return buffer_header(hdrcbdata, str, cb);
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/tool_cb_hdr.c Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The new fallback incorrectly buffers and can discard headers for bodyless non-HTTP responses.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/tool_cb_hdr.c:522

  • Restrict this buffering fallback to HTTP(S). honor_cd_filename is set solely from -J plus -O (tool_operate.c:1229), even though -J only applies to HTTP, while this output block also handles RTSP and file. For an RTSP bodyless response, every -i header now takes this return path, no body callback flushes headlist, and cleanup discards the headers without creating/writing the remote-name output file.
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@bagder bagder closed this in c24a69d Sep 17, 2026
@bagder
bagder deleted the bagder/josh-103 branch September 17, 2026 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants