Circumvented bug that made tar fail on NFS file systems in get_dicom_script.pl and added option -id #409
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…acquisition protocol.
cmadjar
requested changes
Mar 11, 2019
There was a problem hiding this comment.
@nicolasbrossard See my one comment on the PR. Also, could we sit together tomorrow and you could show me how to properly test the PR? My /data directory is an NFS mount and I did not have any issue with your script before. Would not mind being able to reproduce the issue you found before approving the PR.
Thanks!
cmadjar
requested changes
Mar 15, 2019
There was a problem hiding this comment.
@nicolasbrossard I added a few comments based on your changes.
One additional comment: can you update the Description of the PR to say that you also modified the behaviour with the -p option and explain it? That will make my life easier at the time of writing the release notes. Thanks! :)
nicolasbrossard
changed the title
-id
nicolasbrossard
changed the title
-id
cmadjar
reviewed
Mar 18, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR provides a fix for the following use case:
Let's say a user wants to extract a very large set of C files. Since the extraction algorithm extracts all the wanted files in a temporary directory and then archives the entire set in a C<.tar.gz> file, the temporary directory has to be able to (temporarily) store a huge amount of a data. Now suppose that on the user's system, the temporary directory can only be on a NFS file system because of space issues. The script will fail since C has been known to incorrectly report that some files changed during the archiving process if the files are located on an NFS file system, stopping the script dead in its tracks.
The solution is to extract small portions of the data set at a time in the temporary directory, archive them and then immediately delete them from the temporary directory. This reduces the free space requirement imposed on this directory, which in turn provides a fix for the problem described above, assuming that the user has access to a non-NFS temporary directory with sufficient free space.