Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create intermediate directories when expanding #27

Merged
merged 2 commits into from
Jan 14, 2019

Conversation

luispedro
Copy link
Contributor

This matches the behaviour of command-line tar as well as other tar utilities.

closes #26

I tested it on Linux, but also created a Windows version of the code (which I cannot test). Are there automated tests on Windows?

@lehins
Copy link
Collaborator

lehins commented Jan 13, 2019

@luispedro thanks for the PR. You are right that it be good to have the prefix directory created whenever a directory type entry is missing from the tarball itself. That said, such a scenario isn't a well formed tarball, for that reason, IMHO, it is better to create that directory only when lenient untarring is taking place. I'll comment directly on the commits with the few minor adjustments.

@@ -111,6 +111,7 @@ restoreFileInternal lenient fi@FileInfo {..} = do
return (either ((excs ++) . pure) (const excs) eExc)
unless (null excs) $ yield (return (fi, excs))
FTNormal -> do
liftIO $ Dir.createDirectoryIfMissing True $ Posix.takeDirectory fpStr
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make it create upon lenient:

when lenient $ liftIO $ Dir.createDirectoryIfMissing True $ Posix.takeDirectory fpStr

Same approach probably should be added to FTSymbolicLink as well

@@ -67,6 +68,7 @@ restoreFileInternal lenient fi@FileInfo {..} = do
(`when` Dir.setModificationTime fpStr modTime))
return (fi, either ((excs ++) . pure) (const excs) eExc)
FTNormal -> do
Dir.createDirectoryIfMissing True $ FilePath.takeDirectory fpStr
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing liftIO as well as when lenient conditional, just as in Unix.hs module.

This matches the behaviour of command-line `tar` as well as other tar
utilities.

closes snoyberg#26
tests/Spec.hs Outdated
around (withTempTarFiles baseTmp) $
it "create-intermediate" $ \(fpIn, hIn, outDir, fpOut) -> do
hClose hIn
extractTarball "tests/files/subdir.tar" (Just outDir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

extractTarballLenient should be used here instead for two reasons:

  • the attempt to restore ownership to luispedro:luispedro with uid and gid: 1001:1001 for the test file subdir.tar causes the tests on travis to fail (since such user is missing from there)
  • test file is malformed (which is osk, since it is what we are trying to test here) and just as in the comment it should be restored leniently.

This is after feedback on the earlier version of the PR
@lehins
Copy link
Collaborator

lehins commented Jan 13, 2019

Also, to answer your question question:

Are there automated tests on Windows?

Yes there are. AppVeyor is setup and is currently failing for this PR due to my comment

I do have a question about the test file subdir.tar. What program did you use to create that tarball? It is really odd (note the 10K size on a single file tarball with "Hello World" in it), since instead of two blank 512 blocks in it at the end, it is padded with 18 of them.

@luispedro
Copy link
Contributor Author

I do have a question about the test file subdir.tar. What program did you use to create that tarball?

tar (GNU tar) 1.29

On my Ubuntu machine:

$ mkdir -p dir/subdir
$ echo "Hello World" > dir/subdir/file.txt
$ tar cf testing.tar dir/subdir/file.txt
$ ls -l testing.tar 
-rw-rw-r-- 1 luispedro luispedro 10240 Jan 14 01:02 testing.tar

Note, if I use tar cf testing.tar dir, then tar will create the entries for the subdirectories. In either case, when expanding, tar always creates the intermediate dirs.

@lehins
Copy link
Collaborator

lehins commented Jan 14, 2019

Very interesting, didn't know about the default record size of 20 blocks for gnu tar:

In a standard tar file (no options), the block size is 512 and the record size is 10240, for a blocking factor of 20.

So, doing:

$ tar cf testing.tar dir/subdir/file.txt --blocking-factor=1

would create a 2K file, instead of 10K.
Here is the history behind it, in case you are interested: https://www.gnu.org/software/tar/manual/html_node/Blocking.html

In any case, travis CI is still failing, cause the test file wasn't added to the cabal file's extra-source-files. But don't worry about, I'll fix that and make a release in a little bit.

@lehins lehins merged commit d68fe8f into snoyberg:master Jan 14, 2019
@luispedro luispedro deleted the expand_subdirs branch January 14, 2019 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating files should create the with the containing directory
2 participants