Skip to content

Add a targetFile option for downloading straight to a file - #62

Draft
fingolfin wants to merge 1 commit into
masterfrom
mh/download-to-file
Draft

Add a targetFile option for downloading straight to a file#62
fingolfin wants to merge 1 commit into
masterfrom
mh/download-to-file

Conversation

@fingolfin

@fingolfin fingolfin commented Aug 12, 2026

Copy link
Copy Markdown
Member

DownloadURL builds the whole response body as a GAP string, so the largest
file it can fetch is bounded by memory.

With opt.target set to a filename the body goes straight to that file via
CURLOPT_WRITEDATA, and the result record has no result. A 200 MB download
costs 158 MB peak RSS instead of 334 MB; the written file's SHA256 matches.
On failure the file is removed.

The option is called targetFile, not target, on purpose: utils uses
target for its own Download option and forwards the whole record to
DownloadURL, so taking that name would break Download(url, rec(target := ...))
for anyone on an older utils. With a name utils does not forward, all four
version combinations work, and gap-packages/utils# makes utils pass
target on as targetFile to get the memory saving.

Tested against GAP 4.12.2, the declared minimum, and current GAP.

Two open questions:

  • CURL_REQUEST goes from 8 arguments to 9. It is undocumented and
    CurlRequest fills in the new one, but direct callers break —
    tst/errors.tst asserted the arity. Keep it at 8 instead?
  • Removing the target on failure forecloses CURLOPT_RESUME_FROM, which needs
    the partial file. I chose the safe default; resume would want an opt-in.

Written with Claude Opus 5 via Claude Code; reviewed by me.

'DownloadURL' and friends built the whole response body as a GAP string and
returned it, so the largest file one could fetch was bounded by memory. That
matters for the use case this package is increasingly put to: fetching data
sets on behalf of other packages.

With opt.target set to a filename, the body is written straight to that file
via CURLOPT_WRITEDATA as it arrives, and the result record has no 'result'
component. Downloading 200 MB now costs 158 MB peak RSS instead of 334 MB.

If the request fails the file is removed, so a caller may test whether it
exists to decide whether it got the data.

Note that CURL_REQUEST now takes 9 arguments rather than 8. It is not
documented and CurlRequest fills in the new one, but anyone calling the
kernel function directly has to adjust.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.69767% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.2%. Comparing base (6db0e56) to head (e1eec7d).

Files with missing lines Patch % Lines
src/curl.c 89.1% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master     #62     +/-   ##
========================================
- Coverage    95.3%   94.2%   -1.2%     
========================================
  Files           3       3             
  Lines         152     190     +38     
  Branches       15      22      +7     
========================================
+ Hits          145     179     +34     
- Misses          3       5      +2     
- Partials        4       6      +2     
Files with missing lines Coverage Δ
gap/curl.gd 100.0% <ø> (ø)
gap/curl.gi 100.0% <100.0%> (ø)
src/curl.c 91.9% <89.1%> (-1.3%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fingolfin
fingolfin marked this pull request as draft August 16, 2026 07:24
@fingolfin fingolfin changed the title Add a target option for downloading straight to a file Add a targetFile option for downloading straight to a file Aug 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant