Download: do not remove a target file that existed beforehand

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>

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",

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;