Skip to content

Download: pass target on to curlInterface as targetFile - #104

Open
fingolfin wants to merge 1 commit into
masterfrom
mh/download-curl-target
Open

Download: pass target on to curlInterface as targetFile#104
fingolfin wants to merge 1 commit into
masterfrom
mh/download-curl-target

Conversation

@fingolfin

@fingolfin fingolfin commented Aug 12, 2026

Copy link
Copy Markdown
Member

Since curlInterface can write the response body to a file itself, let it: the file then never passes through GAP's memory.

The option is called targetFile there, not target, because Download forwards its whole option record to DownloadURL. Sharing the name would mean a released utils plus a new curlInterface both writing the same file, and utils then reading a result component that is no longer there.

The IsBound( res.result ) guard ensures that a curlInterface that wrote the file itself returns no result, and an older one still does.

Needs gap-packages/curlInterface#62 for the memory saving; harmless without it.

Written with Claude Opus 5 via Claude Code.

CC @ThomasBreuer

@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.36%. Comparing base (3db5598) to head (9dd03f9).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
+ Coverage   86.33%   86.36%   +0.02%     
==========================================
  Files          32       32              
  Lines        1910     1914       +4     
==========================================
+ Hits         1649     1653       +4     
  Misses        261      261              
Files with missing lines Coverage Δ
lib/download.gi 93.77% <100.00%> (+0.12%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ThomasBreuer

Copy link
Copy Markdown
Contributor

Taking #103 into account:

Up to now, the curlInterface based method never wrote a file, therefore one need not care about removing corrupted files after failures. Once curlInterface writes files, do we have to remove such files here or can we assume that curlInterface has already dealt with them?

@fingolfin
fingolfin marked this pull request as draft August 16, 2026 10:38
@fingolfin

Copy link
Copy Markdown
Member Author

Last week I was a conference and went a bit overboard with allowing my AI to open PRs without me double checking everything. That was a mistake and I apologize if I wasted anyone's time by having them stare at slop. (The "reviewed by me" was inserted by the AI without consulting with me -- but I should have caught it. Argh).

Anyway: I've marked this PR as "draft" for now, please don't merge it -- I'll carefully review and edit it, and will mark it as "ready for review" once I am satisfied. Don't waste your time on it for now (that said, of course you are welcome to leave feedback, but don't feel obliged to).

Since curlInterface can write the body to a file itself, let it: the file
then never passes through memory. Older versions ignore the option and
still return the contents, which are written here as before.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@fingolfin
fingolfin force-pushed the mh/download-curl-target branch from 879d2c4 to 9dd03f9 Compare August 23, 2026 19:44
@fingolfin fingolfin changed the title Download: tolerate a curlInterface that honours 'target' itself Download: pass 'target' on to curlInterface as 'targetFile' Aug 23, 2026
@fingolfin fingolfin changed the title Download: pass 'target' on to curlInterface as 'targetFile' Download: pass target on to curlInterface as targetFile Aug 23, 2026
@fingolfin
fingolfin marked this pull request as ready for review August 23, 2026 23:24
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.

2 participants