Skip to content

Download: do not modify the caller's options record - #102

Open
fingolfin wants to merge 1 commit into
masterfrom
mh/download-copy-opt
Open

Download: do not modify the caller's options record#102
fingolfin wants to merge 1 commit into
masterfrom
mh/download-copy-opt

Conversation

@fingolfin

Copy link
Copy Markdown
Member

Filling in the defaults for verifyCert and maxTime writes into the record
the caller passed in, so a record reused across several calls keeps the
settings from the first one.

What makes this worth fixing rather than documenting is when it happens.
With the shipped preferences it does not happen at all -- DownloadMaxTime is
0, which is not an IsPosInt, and DownloadVerifyCertificate is true, so
neither branch is taken:

gap> LoadPackage( "utils" );;
gap> opt:= rec();;
gap> Download( "file:///nonexistent", opt );;
gap> opt;
rec(  )
gap> SetUserPreference( "utils", "DownloadMaxTime", 30 );
gap> opt:= rec();;
gap> Download( "file:///nonexistent", opt );;
gap> opt;
rec( maxTime := 30 )

So it is invisible to anyone testing a stock configuration, and shows up only
for users who have changed a preference. It bites soonest in a retry loop,
since the natural way to write one hoists the option record out of the loop.

The fix is one ShallowCopy, which is enough because only top-level
components are assigned. The via DownloadURL method already copies the
record before touching it, which suggests the behaviour was not intended.

The test sets DownloadMaxTime first, for the reason above; without the fix
it reports [ "maxTime" ] instead of [ ].

gap --packagedirs $PWD tst/testall.g: 0 failures in 22 files.

Found while writing https://github.com/gap-packages/ArtifactManager, which
currently passes a fresh record on every call to work around this.

CC @ThomasBreuer

Filling in the defaults for 'verifyCert' and 'maxTime' wrote into the record
the caller passed in, so a record reused across several calls kept the
settings from the first one.

Only visible when the preferences differ from their defaults -- with the
shipped values neither branch is taken -- which is why the test sets
DownloadMaxTime first.

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

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.30%. Comparing base (a4c23a8) to head (cb248cc).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
+ Coverage   86.24%   86.30%   +0.05%     
==========================================
  Files          32       32              
  Lines        1905     1906       +1     
==========================================
+ Hits         1643     1645       +2     
+ Misses        262      261       -1     
Files with missing lines Coverage Δ
lib/download.gi 93.53% <100.00%> (+0.53%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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