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 result component in this case.

- 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 – but note that a failed attempt may + well have overwritten it, so its old contents are not guaranteed either. verifyCert 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;