From 02591d1559006eb41abf0c07d23214fd139be98c Mon Sep 17 00:00:00 2001
From: Max Horn <max@quendi.de>
Date: Sun, 23 Aug 2026 21:38:23 +0200
Subject: [PATCH] Download: do not remove a target file the caller brought

The clean-up added in #103 removes 'opt.target' after any failed method,
which also removes a file that was there before the call.  A failed
download then destroys a file it never created.

Remember whether the file existed when 'Download' was entered, and remove
it only if it did not.

This also lets the wget method drop its own removal: with the caller's
file protected in one place, the per-method clean-up is redundant, and
'RemoveFile' is now called from exactly one place instead of two that
disagreed about whether to signal an error when it fails.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 doc/download.xml |  5 ++++-
 lib/download.gi  | 20 +++++++++++---------
 tst/download.tst | 11 +++++++++++
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/doc/download.xml b/doc/download.xml
index cf8548c..ea3f4bc 100644
--- a/doc/download.xml
+++ b/doc/download.xml
@@ -69,7 +69,10 @@ The following components are supported.
   and the function writes the downloaded contents to this file;
   the returned record does not have a <C>result</C> component in this case.
   <P/>
-  If the download fails then this file is not left behind.
+  If the download fails then a file created by the attempt is not left
+  behind.  A file that was already there before the call is not removed,
+  since it is not ours to delete &ndash; but note that a failed attempt may
+  well have overwritten it, so its old contents are not guaranteed either.
 </Item>
 <Mark><C>verifyCert</C></Mark>
 <Item>
diff --git a/lib/download.gi b/lib/download.gi
index 4b9a6ce..eecbda0 100644
--- a/lib/download.gi
+++ b/lib/download.gi
@@ -173,11 +173,6 @@ Add( Download_Methods, rec(
     code:= Process( DirectoryCurrent(), exec, InputTextNone(), outstream, args );
     CloseStream( outstream );
     if code <> 0 then
-      # wget may have created the target file; try to remove it
-      if IsBound( opt.target ) and IsString( opt.target ) and
-         IsExistingFile( opt.target ) and RemoveFile( opt.target ) <> true then
-        Error( "Download cannot remove unwanted file ", opt.target );
-      fi;
       return rec( success:= false,
                   error:= Concatenation( "Process returned ", String( code ) ) );
     elif not ( IsBound( opt.target ) and IsString( opt.target ) ) then
@@ -245,7 +240,7 @@ InstallMethod( Download,
 InstallMethod( Download,
     [ "IsString", "IsRecord" ],
     function( url, opt )
-    local timeout, errors, r, res;
+    local timeout, hadTarget, errors, r, res;
 
     # Do not modify the caller's record when filling in the defaults below.
     opt:= ShallowCopy( opt );
@@ -264,6 +259,11 @@ InstallMethod( Download,
       fi;
     fi;
 
+    # Whether the caller brought the target file, as opposed to a method
+    # creating it below.  We remove only what we created.
+    hadTarget:= IsBound( opt.target ) and IsString( opt.target ) and
+                IsExistingFile( opt.target );
+
     # Run over the methods.
     errors:= [];
     for r in Download_Methods do
@@ -275,9 +275,11 @@ InstallMethod( Download,
         fi;
         # A failed method may have left a partial or bogus target file behind.
         # Remove it here, so that the guarantee holds for every method,
-        # including ones added to 'Download_Methods' from outside.
-        if IsBound( opt.target ) and IsString( opt.target ) and
-           IsExistingFile( opt.target ) then
+        # including ones added to 'Download_Methods' from outside -- but never
+        # remove a file the caller had before the call, which is not ours to
+        # delete.
+        if not hadTarget and IsBound( opt.target ) and IsString( opt.target )
+           and IsExistingFile( opt.target ) then
           RemoveFile( opt.target );
         fi;
         Info( InfoUtils, 2, "Download method ", r.name, " failed with\n",
diff --git a/tst/download.tst b/tst/download.tst
index 02856d3..2a18cec 100644
--- a/tst/download.tst
+++ b/tst/download.tst
@@ -164,6 +164,17 @@ true
 gap> IsExistingFile( file );
 false
 
+##  A file that was already there is not removed by a failed download:
+##  'Download' only cleans up after itself.
+gap> FileString( file, "mine" );;
+gap> res1:= Download( Concatenation( baseurl, "/missing" ),
+>                     rec( target:= file ) );;
+gap> res1.success;
+false
+gap> IsExistingFile( file );
+true
+gap> RemoveFile( file );;
+
 ##  test errors and redirects
 gap> res1:= Download( Concatenation( baseurl, "/missing" ) );;
 gap> res1.success = false;