[Courgette] Refactor BSDiff namespaces and bsdiff::search() interface.

Details: - Move BSDiff (but not PagedArray) from namespace courgette to bsdiff. - Change namespace courgette::qsuf to qsuf. - Change bsdiff:search() to return struct {pos, size} so we don't need awkward pointer passing; update callers. - Updated BSDiff callers. Also fix weird hybrid usage by setup_util.cc, which calls Courgette's BSDiff, but using OK from BSPatch. BUG=608885 Review-Url: https://codereview.chromium.org/2031193002 Cr-Commit-Position: refs/heads/master@{#407924}
feiyue1206 · Jul 26, 2016 · 7054b5a · 7054b5a
1 parent d35eb5b
commit 7054b5a
Show file tree

Hide file tree

Showing 15 changed files with 176 additions and 140 deletions.
diff --git a/chrome/installer/setup/setup_util.cc b/chrome/installer/setup/setup_util.cc
@@ -41,7 +41,6 @@
 #include "chrome/installer/util/util_constants.h"
 #include "courgette/courgette.h"
 #include "courgette/third_party/bsdiff/bsdiff.h"
-#include "third_party/bspatch/mbspatch.h"
 
 namespace installer {
 
@@ -149,8 +148,8 @@ int BsdiffPatchFiles(const base::FilePath& src,
   if (src.empty() || patch.empty() || dest.empty())
     return installer::PATCH_INVALID_ARGUMENTS;
 
-  const int patch_status = courgette::ApplyBinaryPatch(src, patch, dest);
-  const int exit_code = patch_status != OK ?
+  const int patch_status = bsdiff::ApplyBinaryPatch(src, patch, dest);
+  const int exit_code = patch_status != bsdiff::OK ?
                         patch_status + kBsdiffErrorOffset : 0;
 
   LOG_IF(ERROR, exit_code)

diff --git a/chrome/utility/chrome_content_utility_client.cc b/chrome/utility/chrome_content_utility_client.cc
@@ -266,9 +266,9 @@ void ChromeContentUtilityClient::OnPatchFileBsdiff(
   if (input_file.empty() || patch_file.empty() || output_file.empty()) {
     Send(new ChromeUtilityHostMsg_PatchFile_Finished(-1));
   } else {
-    const int patch_status = courgette::ApplyBinaryPatch(input_file,
-                                                         patch_file,
-                                                         output_file);
+    const int patch_status = bsdiff::ApplyBinaryPatch(input_file,
+                                                      patch_file,
+                                                      output_file);
     Send(new ChromeUtilityHostMsg_PatchFile_Finished(patch_status));
   }
   ReleaseProcessIfNeeded();

diff --git a/components/update_client/component_patcher_operation.cc b/components/update_client/component_patcher_operation.cc
@@ -208,8 +208,8 @@ void DeltaUpdateOpPatch::DoRun(const ComponentUnpacker::Callback& callback) {
 
   if (operation_ == kBsdiff) {
     DonePatching(callback,
-                 courgette::ApplyBinaryPatch(input_abs_path_, patch_abs_path_,
-                                             output_abs_path_));
+                 bsdiff::ApplyBinaryPatch(input_abs_path_, patch_abs_path_,
+                                          output_abs_path_));
   } else if (operation_ == kCourgette) {
     DonePatching(callback, courgette::ApplyEnsemblePatch(
                                input_abs_path_.value().c_str(),
@@ -224,7 +224,7 @@ void DeltaUpdateOpPatch::DonePatching(
     const ComponentUnpacker::Callback& callback,
     int result) {
   if (operation_ == kBsdiff) {
-    if (result == courgette::OK) {
+    if (result == bsdiff::OK) {
       callback.Run(ComponentUnpacker::kNone, 0);
     } else {
       callback.Run(ComponentUnpacker::kDeltaOperationFailure,

diff --git a/courgette/bsdiff_memory_unittest.cc b/courgette/bsdiff_memory_unittest.cc
@@ -25,17 +25,18 @@ void BSDiffMemoryTest::GenerateAndTestPatch(const std::string& old_text,
   new1.Init(new_text.c_str(), new_text.length());
 
   courgette::SinkStream patch1;
-  courgette::BSDiffStatus status = CreateBinaryPatch(&old1, &new1, &patch1);
-  EXPECT_EQ(courgette::OK, status);
+  bsdiff::BSDiffStatus status =
+      bsdiff::CreateBinaryPatch(&old1, &new1, &patch1);
+  EXPECT_EQ(bsdiff::OK, status);
 
   courgette::SourceStream old2;
   courgette::SourceStream patch2;
   old2.Init(old_text.c_str(), old_text.length());
   patch2.Init(patch1);
 
   courgette::SinkStream new2;
-  status = ApplyBinaryPatch(&old2, &patch2, &new2);
-  EXPECT_EQ(courgette::OK, status);
+  status = bsdiff::ApplyBinaryPatch(&old2, &patch2, &new2);
+  EXPECT_EQ(bsdiff::OK, status);
   EXPECT_EQ(new_text.length(), new2.Length());
   EXPECT_EQ(0, memcmp(new_text.c_str(), new2.Buffer(), new_text.length()));
 }

diff --git a/courgette/courgette_tool.cc b/courgette/courgette_tool.cc
@@ -277,9 +277,10 @@ void DisassembleAdjustDiff(const base::FilePath& model_file,
     old_source.Init(old_stream ? *old_stream : empty_sink);
     new_source.Init(new_stream ? *new_stream : empty_sink);
     courgette::SinkStream patch_stream;
-    courgette::BSDiffStatus status =
-        courgette::CreateBinaryPatch(&old_source, &new_source, &patch_stream);
-    if (status != courgette::OK) Problem("-xxx failed.");
+    bsdiff::BSDiffStatus status =
+        bsdiff::CreateBinaryPatch(&old_source, &new_source, &patch_stream);
+    if (status != bsdiff::OK)
+      Problem("-xxx failed.");
 
     std::string append = std::string("-") + base::IntToString(i);
 
@@ -327,7 +328,8 @@ void GenerateEnsemblePatch(const base::FilePath& old_file,
   courgette::Status status =
       courgette::GenerateEnsemblePatch(&old_stream, &new_stream, &patch_stream);
 
-  if (status != courgette::C_OK) Problem("-gen failed.");
+  if (status != courgette::C_OK)
+    Problem("-gen failed.");
 
   WriteSinkToFile(&patch_stream, patch_file);
 }
@@ -398,10 +400,11 @@ void GenerateBSDiffPatch(const base::FilePath& old_file,
   new_stream.Init(new_buffer.data(), new_buffer.length());
 
   courgette::SinkStream patch_stream;
-  courgette::BSDiffStatus status =
-      courgette::CreateBinaryPatch(&old_stream, &new_stream, &patch_stream);
+  bsdiff::BSDiffStatus status =
+      bsdiff::CreateBinaryPatch(&old_stream, &new_stream, &patch_stream);
 
-  if (status != courgette::OK) Problem("-genbsdiff failed.");
+  if (status != bsdiff::OK)
+    Problem("-genbsdiff failed.");
 
   WriteSinkToFile(&patch_stream, patch_file);
 }
@@ -418,10 +421,11 @@ void ApplyBSDiffPatch(const base::FilePath& old_file,
   patch_stream.Init(patch_buffer.data(), patch_buffer.length());
 
   courgette::SinkStream new_stream;
-  courgette::BSDiffStatus status =
-      courgette::ApplyBinaryPatch(&old_stream, &patch_stream, &new_stream);
+  bsdiff::BSDiffStatus status =
+      bsdiff::ApplyBinaryPatch(&old_stream, &patch_stream, &new_stream);
 
-  if (status != courgette::OK) Problem("-applybsdiff failed.");
+  if (status != bsdiff::OK)
+    Problem("-applybsdiff failed.");
 
   WriteSinkToFile(&new_stream, new_file);
 }

diff --git a/courgette/simple_delta.cc b/courgette/simple_delta.cc
@@ -15,10 +15,10 @@ namespace courgette {
 
 namespace {
 
-Status BSDiffStatusToStatus(BSDiffStatus status) {
+Status BSDiffStatusToStatus(bsdiff::BSDiffStatus status) {
   switch (status) {
-    case OK: return C_OK;
-    case CRC_ERROR: return C_BINARY_DIFF_CRC_ERROR;
+    case bsdiff::OK: return C_OK;
+    case bsdiff::CRC_ERROR: return C_BINARY_DIFF_CRC_ERROR;
     default: return C_GENERAL_ERROR;
   }
 }
@@ -27,14 +27,14 @@ Status BSDiffStatusToStatus(BSDiffStatus status) {
 
 Status ApplySimpleDelta(SourceStream* old, SourceStream* delta,
                         SinkStream* target) {
-  return BSDiffStatusToStatus(ApplyBinaryPatch(old, delta, target));
+  return BSDiffStatusToStatus(bsdiff::ApplyBinaryPatch(old, delta, target));
 }
 
 Status GenerateSimpleDelta(SourceStream* old, SourceStream* target,
                            SinkStream* delta) {
   VLOG(1) << "GenerateSimpleDelta " << old->Remaining()
           << " " << target->Remaining();
-  return BSDiffStatusToStatus(CreateBinaryPatch(old, target, delta));
+  return BSDiffStatusToStatus(bsdiff::CreateBinaryPatch(old, target, delta));
 }
 
 }  // namespace courgette
diff --git a/courgette/third_party/bsdiff/README.chromium b/courgette/third_party/bsdiff/README.chromium
@@ -19,13 +19,15 @@ incompatible with the original and it cannot be compiled outside the Chromium
 source tree or the Courgette project.
 
 List of changes made to original code:
-  - Wrapped functions in 'courgette' namespace.
+  - Wrapped functions in 'bsdiff' namespace.
   - Renamed .c files to .cc files.
-  - Added bsdiff.h header file.
+  - Added bsdiff.h and bsdiff_search.h header files.
   - Changed the code to use streams.h from courgette.
   - Changed the encoding of numbers to use the 'varint' encoding.
   - Reformatted code to be closer to Google coding standards.
   - Renamed variables.
   - Added comments.
-  - Extracted qsufsort into qsufsort.h in 'courgette::qsuf' namespace.
+  - Extracted qsufsort into qsufsort.h in 'qsuf' namespace.
   - Added unit tests for qsufsort.
+  - Fixed qsufsort pivoting issue: http://crbug.com/605565.
+  - Fixed search() comparison issue: http://crbug.com/620867.
diff --git a/courgette/third_party/bsdiff/bsdiff.h b/courgette/third_party/bsdiff/bsdiff.h
@@ -47,6 +47,11 @@
 #include "base/files/file_util.h"
 
 namespace courgette {
+class SourceStream;
+class SinkStream;
+}  // namespace courgette
+
+namespace bsdiff {
 
 enum BSDiffStatus {
   OK = 0,
@@ -57,22 +62,19 @@ enum BSDiffStatus {
   WRITE_ERROR = 5
 };
 
-class SourceStream;
-class SinkStream;
-
 // Creates a binary patch.
 //
-BSDiffStatus CreateBinaryPatch(SourceStream* old_stream,
-                               SourceStream* new_stream,
-                               SinkStream* patch_stream);
+BSDiffStatus CreateBinaryPatch(courgette::SourceStream* old_stream,
+                               courgette::SourceStream* new_stream,
+                               courgette::SinkStream* patch_stream);
 
 // Applies the given patch file to a given source file. This method validates
 // the CRC of the original file stored in the patch file, before applying the
 // patch to it.
 //
-BSDiffStatus ApplyBinaryPatch(SourceStream* old_stream,
-                              SourceStream* patch_stream,
-                              SinkStream* new_stream);
+BSDiffStatus ApplyBinaryPatch(courgette::SourceStream* old_stream,
+                              courgette::SourceStream* patch_stream,
+                              courgette::SinkStream* new_stream);
 
 // As above, but simply takes the file paths.
 BSDiffStatus ApplyBinaryPatch(const base::FilePath& old_stream,
@@ -94,5 +96,6 @@ typedef struct MBSPatchHeader_ {
 // null at end of string.
 #define MBS_PATCH_HEADER_TAG "GBSDIF42"
 
-}  // namespace
+}  // namespace bsdiff
+
 #endif  // COURGETTE_THIRD_PARTY_BSDIFF_BSDIFF_H_
diff --git a/courgette/third_party/bsdiff/bsdiff_apply.cc b/courgette/third_party/bsdiff/bsdiff_apply.cc
@@ -44,7 +44,17 @@
 #include "courgette/crc.h"
 #include "courgette/streams.h"
 
-namespace courgette {
+namespace {
+
+using courgette::CalculateCrc;
+using courgette::SinkStream;
+using courgette::SinkStreamSet;
+using courgette::SourceStream;
+using courgette::SourceStreamSet;
+
+}  // namespace
+
+namespace bsdiff {
 
 BSDiffStatus MBS_ReadHeader(SourceStream* stream, MBSPatchHeader* header) {
   if (!stream->Read(header->tag, sizeof(header->tag)))
@@ -215,4 +225,4 @@ BSDiffStatus ApplyBinaryPatch(const base::FilePath& old_file_path,
   return OK;
 }
 
-}  // namespace
+}  // namespace bsdiff
diff --git a/courgette/third_party/bsdiff/bsdiff_create.cc b/courgette/third_party/bsdiff/bsdiff_create.cc
@@ -65,7 +65,18 @@
 #include "courgette/third_party/bsdiff/paged_array.h"
 #include "courgette/third_party/bsdiff/qsufsort.h"
 
-namespace courgette {
+namespace {
+
+using courgette::CalculateCrc;
+using courgette::PagedArray;
+using courgette::SinkStream;
+using courgette::SinkStreamSet;
+using courgette::SourceStream;
+using courgette::SourceStreamSet;
+
+}  // namespace
+
+namespace bsdiff {
 
 static CheckBool WriteHeader(SinkStream* stream, MBSPatchHeader* header) {
   bool ok = stream->Write(header->tag, sizeof(header->tag));
@@ -165,26 +176,26 @@ BSDiffStatus CreateBinaryPatch(SourceStream* old_stream,
   int lastscan = 0, lastpos = 0, lastoffset = 0;
 
   int scan = 0;
-  int match_length = 0;
+  SearchResult match(0, 0);
 
   while (scan < newsize) {
-    int pos = 0;
     int oldscore = 0;  // Count of how many bytes of the current match at |scan|
                        // extend the match at |lastscan|.
+    match.pos = 0;
 
-    scan += match_length;
+    scan += match.size;
     for (int scsc = scan; scan < newsize; ++scan) {
-      match_length = courgette::search<PagedArray<int>&>(
-          I, old, oldsize, newbuf + scan, newsize - scan, &pos);
+      match = search<PagedArray<int>&>(
+          I, old, oldsize, newbuf + scan, newsize - scan);
 
-      for (; scsc < scan + match_length; scsc++)
+      for (; scsc < scan + match.size; scsc++)
         if ((scsc + lastoffset < oldsize) &&
             (old[scsc + lastoffset] == newbuf[scsc]))
           oldscore++;
 
-      if ((match_length == oldscore) && (match_length != 0))
+      if ((match.size == oldscore) && (match.size != 0))
         break;  // Good continuing match, case (1)
-      if (match_length > oldscore + 8)
+      if (match.size > oldscore + 8)
         break;  // New seed match, case (2)
 
       if ((scan + lastoffset < oldsize) &&
@@ -193,7 +204,7 @@ BSDiffStatus CreateBinaryPatch(SourceStream* old_stream,
       // Case (3) continues in this loop until we fall out of the loop (4).
     }
 
-    if ((match_length != oldscore) || (scan == newsize)) {  // Cases (2) and (4)
+    if ((match.size != oldscore) || (scan == newsize)) {  // Cases (2) and (4)
       // This next chunk of code finds the boundary between the bytes to be
       // copied as part of the current triple, and the bytes to be copied as
       // part of the next triple.  The |lastscan| match is extended forwards as
@@ -206,8 +217,8 @@ BSDiffStatus CreateBinaryPatch(SourceStream* old_stream,
       int lenb = 0;
       if (scan < newsize) {  // i.e. not case (4); there is a match to extend.
         int score = 0, Sb = 0;
-        for (int i = 1; (scan >= lastscan + i) && (pos >= i); i++) {
-          if (old[pos - i] == newbuf[scan - i])
+        for (int i = 1; (scan >= lastscan + i) && (match.pos >= i); i++) {
+          if (old[match.pos - i] == newbuf[scan - i])
             score++;
           if (score * 2 - i > Sb * 2 - lenb) {
             Sb = score;
@@ -247,7 +258,7 @@ BSDiffStatus CreateBinaryPatch(SourceStream* old_stream,
               old[lastpos + lenf - overlap + i]) {
             score++;
           }
-          if (newbuf[scan - lenb + i] == old[pos - lenb + i]) {
+          if (newbuf[scan - lenb + i] == old[match.pos - lenb + i]) {
             score--;
           }
           if (score > Ss) {
@@ -284,7 +295,7 @@ BSDiffStatus CreateBinaryPatch(SourceStream* old_stream,
 
       uint32_t copy_count = lenf;
       uint32_t extra_count = gap;
-      int32_t seek_adjustment = ((pos - lenb) - (lastpos + lenf));
+      int32_t seek_adjustment = ((match.pos - lenb) - (lastpos + lenf));
 
       if (!control_stream_copy_counts->WriteVarint32(copy_count) ||
           !control_stream_extra_counts->WriteVarint32(extra_count) ||
@@ -300,7 +311,7 @@ BSDiffStatus CreateBinaryPatch(SourceStream* old_stream,
 #endif
 
       lastscan = scan - lenb;  // Include the backward extension in seed.
-      lastpos = pos - lenb;    //  ditto.
+      lastpos = match.pos - lenb;    //  ditto.
       lastoffset = lastpos - lastscan;
     }
   }
@@ -339,4 +350,4 @@ BSDiffStatus CreateBinaryPatch(SourceStream* old_stream,
   return OK;
 }
 
-}  // namespace courgette
+}  // namespace bsdiff