[PATCH] D40606: [Support/TarWriter] - Don't allow TarWriter to add the same file more than once.

[Rafael Avila de Espindola via llvm-commits]

LGTM

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> grimar updated this revision to Diff 125298.
> grimar added a comment.
>
> Addressed review comments:
>
> - Split test into three.
> - Use llvm::sys::fs::file_size API for finding output tar file size.
>
>
> https://reviews.llvm.org/D40606
>
> Files:
>   include/llvm/Support/TarWriter.h
>   lib/Support/TarWriter.cpp
>   unittests/Support/TarWriterTest.cpp
>
>
> Index: unittests/Support/TarWriterTest.cpp
> ===================================================================
> --- unittests/Support/TarWriterTest.cpp
> +++ unittests/Support/TarWriterTest.cpp
> @@ -120,4 +120,60 @@
>    StringRef Pax = StringRef((char *)(Buf.data() + 512), 512);
>    EXPECT_TRUE(Pax.startswith("211 path=/" + std::string(200, 'x')));
>  }
> +
> +TEST_F(TarWriterTest, SingleFileSize) {
> +  SmallString<128> Path;
> +  std::error_code EC =
> +      sys::fs::createTemporaryFile("TarWriterTest", "tar", Path);
> +  EXPECT_FALSE((bool)EC);
> +
> +  Expected<std::unique_ptr<TarWriter>> TarOrErr = TarWriter::create(Path, "");
> +  EXPECT_TRUE((bool)TarOrErr);
> +  std::unique_ptr<TarWriter> Tar = std::move(*TarOrErr);
> +  Tar->append("FooPath", "foo");
> +  Tar.reset();
> +
> +  uint64_t TarSize;
> +  EC = sys::fs::file_size(Path, TarSize);
> +  EXPECT_FALSE((bool)EC);
> +  EXPECT_EQ(TarSize, 2048);
> +}
> +
> +TEST_F(TarWriterTest, TwoDifferentFilesSize) {
> +  SmallString<128> Path;
> +  std::error_code EC =
> +      sys::fs::createTemporaryFile("TarWriterTest", "tar", Path);
> +  EXPECT_FALSE((bool)EC);
> +
> +  Expected<std::unique_ptr<TarWriter>> TarOrErr = TarWriter::create(Path, "");
> +  EXPECT_TRUE((bool)TarOrErr);
> +  std::unique_ptr<TarWriter> Tar = std::move(*TarOrErr);
> +  Tar->append("FooPath", "foo");
> +  Tar->append("BarPath", "bar");
> +  Tar.reset();
> +
> +  uint64_t TarSize;
> +  EC = sys::fs::file_size(Path, TarSize);
> +  EXPECT_FALSE((bool)EC);
> +  EXPECT_EQ(TarSize, 3072);
> +}
> +
> +TEST_F(TarWriterTest, TwoSamePathFilesSize) {
> +  SmallString<128> Path;
> +  std::error_code EC =
> +      sys::fs::createTemporaryFile("TarWriterTest", "tar", Path);
> +  EXPECT_FALSE((bool)EC);
> +
> +  Expected<std::unique_ptr<TarWriter>> TarOrErr = TarWriter::create(Path, "");
> +  EXPECT_TRUE((bool)TarOrErr);
> +  std::unique_ptr<TarWriter> Tar = std::move(*TarOrErr);
> +  Tar->append("FooPath", "foo");
> +  Tar->append("FooPath", "bar");
> +  Tar.reset();
> +
> +  uint64_t TarSize;
> +  EC = sys::fs::file_size(Path, TarSize);
> +  EXPECT_FALSE((bool)EC);
> +  EXPECT_EQ(TarSize, 2048);
>  }
> +} // namespace
> Index: lib/Support/TarWriter.cpp
> ===================================================================
> --- lib/Support/TarWriter.cpp
> +++ lib/Support/TarWriter.cpp
> @@ -173,6 +173,10 @@
>    // Write Path and Data.
>    std::string Fullpath = BaseDir + "/" + sys::path::convert_to_slash(Path);
>  
> +  // We do not want to include the same file more than once.
> +  if (!Files.insert(Fullpath).second)
> +    return;
> +
>    StringRef Prefix;
>    StringRef Name;
>    if (splitUstar(Fullpath, Prefix, Name)) {
> Index: include/llvm/Support/TarWriter.h
> ===================================================================
> --- include/llvm/Support/TarWriter.h
> +++ include/llvm/Support/TarWriter.h
> @@ -11,6 +11,7 @@
>  #define LLVM_SUPPORT_TAR_WRITER_H
>  
>  #include "llvm/ADT/StringRef.h"
> +#include "llvm/ADT/StringSet.h"
>  #include "llvm/Support/Error.h"
>  #include "llvm/Support/raw_ostream.h"
>  
> @@ -26,6 +27,7 @@
>    TarWriter(int FD, StringRef BaseDir);
>    raw_fd_ostream OS;
>    std::string BaseDir;
> +  StringSet<> Files;
>  };
>  }
>  
>
>
> Index: unittests/Support/TarWriterTest.cpp
> ===================================================================
> --- unittests/Support/TarWriterTest.cpp
> +++ unittests/Support/TarWriterTest.cpp
> @@ -120,4 +120,60 @@
>    StringRef Pax = StringRef((char *)(Buf.data() + 512), 512);
>    EXPECT_TRUE(Pax.startswith("211 path=/" + std::string(200, 'x')));
>  }
> +
> +TEST_F(TarWriterTest, SingleFileSize) {
> +  SmallString<128> Path;
> +  std::error_code EC =
> +      sys::fs::createTemporaryFile("TarWriterTest", "tar", Path);
> +  EXPECT_FALSE((bool)EC);
> +
> +  Expected<std::unique_ptr<TarWriter>> TarOrErr = TarWriter::create(Path, "");
> +  EXPECT_TRUE((bool)TarOrErr);
> +  std::unique_ptr<TarWriter> Tar = std::move(*TarOrErr);
> +  Tar->append("FooPath", "foo");
> +  Tar.reset();
> +
> +  uint64_t TarSize;
> +  EC = sys::fs::file_size(Path, TarSize);
> +  EXPECT_FALSE((bool)EC);
> +  EXPECT_EQ(TarSize, 2048);
> +}
> +
> +TEST_F(TarWriterTest, TwoDifferentFilesSize) {
> +  SmallString<128> Path;
> +  std::error_code EC =
> +      sys::fs::createTemporaryFile("TarWriterTest", "tar", Path);
> +  EXPECT_FALSE((bool)EC);
> +
> +  Expected<std::unique_ptr<TarWriter>> TarOrErr = TarWriter::create(Path, "");
> +  EXPECT_TRUE((bool)TarOrErr);
> +  std::unique_ptr<TarWriter> Tar = std::move(*TarOrErr);
> +  Tar->append("FooPath", "foo");
> +  Tar->append("BarPath", "bar");
> +  Tar.reset();
> +
> +  uint64_t TarSize;
> +  EC = sys::fs::file_size(Path, TarSize);
> +  EXPECT_FALSE((bool)EC);
> +  EXPECT_EQ(TarSize, 3072);
> +}
> +
> +TEST_F(TarWriterTest, TwoSamePathFilesSize) {
> +  SmallString<128> Path;
> +  std::error_code EC =
> +      sys::fs::createTemporaryFile("TarWriterTest", "tar", Path);
> +  EXPECT_FALSE((bool)EC);
> +
> +  Expected<std::unique_ptr<TarWriter>> TarOrErr = TarWriter::create(Path, "");
> +  EXPECT_TRUE((bool)TarOrErr);
> +  std::unique_ptr<TarWriter> Tar = std::move(*TarOrErr);
> +  Tar->append("FooPath", "foo");
> +  Tar->append("FooPath", "bar");
> +  Tar.reset();
> +
> +  uint64_t TarSize;
> +  EC = sys::fs::file_size(Path, TarSize);
> +  EXPECT_FALSE((bool)EC);
> +  EXPECT_EQ(TarSize, 2048);
>  }
> +} // namespace
> Index: lib/Support/TarWriter.cpp
> ===================================================================
> --- lib/Support/TarWriter.cpp
> +++ lib/Support/TarWriter.cpp
> @@ -173,6 +173,10 @@
>    // Write Path and Data.
>    std::string Fullpath = BaseDir + "/" + sys::path::convert_to_slash(Path);
>  
> +  // We do not want to include the same file more than once.
> +  if (!Files.insert(Fullpath).second)
> +    return;
> +
>    StringRef Prefix;
>    StringRef Name;
>    if (splitUstar(Fullpath, Prefix, Name)) {
> Index: include/llvm/Support/TarWriter.h
> ===================================================================
> --- include/llvm/Support/TarWriter.h
> +++ include/llvm/Support/TarWriter.h
> @@ -11,6 +11,7 @@
>  #define LLVM_SUPPORT_TAR_WRITER_H
>  
>  #include "llvm/ADT/StringRef.h"
> +#include "llvm/ADT/StringSet.h"
>  #include "llvm/Support/Error.h"
>  #include "llvm/Support/raw_ostream.h"
>  
> @@ -26,6 +27,7 @@
>    TarWriter(int FD, StringRef BaseDir);
>    raw_fd_ostream OS;
>    std::string BaseDir;
> +  StringSet<> Files;
>  };
>  }
>