Skip to content

Commit

Permalink
Don't error on rm if clone fails to create cache in prefetch
Browse files Browse the repository at this point in the history
  • Loading branch information
@tkelman
tkelman committed Nov 26, 2015
1 parent f3e8093 commit eb31eef
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion base/pkg/cache.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ function prefetch(pkg::AbstractString, url::AbstractString, sha1s::Vector)
info("Cloning cache of $pkg from $url")
try Git.run(`clone -q --mirror $url $cache`)
catch
rm(cache, recursive=true)
isdir(cache) && rm(cache, recursive=true)
rethrow()
end
end
Expand Down

4 comments on commit eb31eef

@tkelman
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this failure case was fixed for master (libgit2) in e4ac8bd

@eschnett
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of isdir(...) && rm(...), wouldn't it be better to say try rm(...) end? This avoids a race condition if the cache directory is deleted between the isdir and `rm.

@tkelman
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in a catch where an error is being thrown anyway, and just makes the error from git get rethrown instead of an error from rm.

The raciness of this idiom is a bigger issue and is probably worth reconsidering a force keyword argument which would be cleaner imo than putting try end everywhere.

@eschnett
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for force, in particular if it only ignored non-existing files, and would still throw if the file exists and cannot be deleted.

Please sign in to comment.