From dedcbaad51106a62a72721a68a27c132d3c04a82 Mon Sep 17 00:00:00 2001
From: Felipe Leme <felipeal@google.com>
Date: Mon, 20 Mar 2017 11:00:11 -0700
Subject: [PATCH] Don't display bugreport progress when it recedes.

Also fixed InvalidNumberArgs that broke when usage() was moved out from
bugreport.cpp.

Fixes: 26354314
Bug: 28054087
Test: m -j32 adb_test && ./out/host/linux-x86/nativetest64/adb_test/adb_test --gtest_filter=BugreportTest.*

Change-Id: I7be5ef7de0fb0d339dc80a2abc816e1c905deb22
---
 adb/bugreport.cpp      | 13 ++++++++++++-
 adb/bugreport_test.cpp | 35 +++++++++++++++++++++++++++++++----
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/adb/bugreport.cpp b/adb/bugreport.cpp
index a5c312bbe..9dc981102 100644
--- a/adb/bugreport.cpp
+++ b/adb/bugreport.cpp
@@ -47,7 +47,8 @@ class BugreportStandardStreamsCallback : public StandardStreamsCallbackInterface
           invalid_lines_(),
           show_progress_(show_progress),
           status_(0),
-          line_() {
+          line_(),
+          last_progress_(0) {
         SetLineMessage("generating");
     }
 
@@ -66,6 +67,7 @@ class BugreportStandardStreamsCallback : public StandardStreamsCallbackInterface
     void OnStderr(const char* buffer, int length) {
         OnStream(nullptr, stderr, buffer, length);
     }
+
     int Done(int unused_) {
         // Process remaining line, if any.
         ProcessLine(line_);
@@ -145,6 +147,11 @@ class BugreportStandardStreamsCallback : public StandardStreamsCallbackInterface
             size_t idx1 = line.rfind(BUGZ_PROGRESS_PREFIX) + strlen(BUGZ_PROGRESS_PREFIX);
             size_t idx2 = line.rfind(BUGZ_PROGRESS_SEPARATOR);
             int progress = std::stoi(line.substr(idx1, (idx2 - idx1)));
+            if (progress <= last_progress_) {
+                // Ignore.
+                return;
+            }
+            last_progress_ = progress;
             int total = std::stoi(line.substr(idx2 + 1));
             br_->UpdateProgress(line_message_, progress, total);
         } else {
@@ -180,6 +187,10 @@ class BugreportStandardStreamsCallback : public StandardStreamsCallbackInterface
     // Temporary buffer containing the characters read since the last newline (\n).
     std::string line_;
 
+    // Last displayed progress.
+    // Since dumpstate progress can recede, only forward progress should be displayed
+    int last_progress_;
+
     DISALLOW_COPY_AND_ASSIGN(BugreportStandardStreamsCallback);
 };
 
diff --git a/adb/bugreport_test.cpp b/adb/bugreport_test.cpp
index 112928523..b500c49c1 100644
--- a/adb/bugreport_test.cpp
+++ b/adb/bugreport_test.cpp
@@ -50,9 +50,7 @@ bool do_sync_pull(const std::vector<const char*>& srcs, const char* dst, bool co
 
 // Empty functions so tests don't need to be linked against commandline.cpp
 DefaultStandardStreamsCallback DEFAULT_STANDARD_STREAMS_CALLBACK(nullptr, nullptr);
-int usage() {
-    return -42;
-}
+
 int send_shell_command(TransportType transport_type, const char* serial, const std::string& command,
                        bool disable_shell_protocol, StandardStreamsCallbackInterface* callback) {
     ADD_FAILURE() << "send_shell_command() should have been mocked";
@@ -155,7 +153,7 @@ class BugreportTest : public ::testing::Test {
 // Tests when called with invalid number of arguments
 TEST_F(BugreportTest, InvalidNumberArgs) {
     const char* args[] = {"bugreport", "to", "principal"};
-    ASSERT_EQ(-42, br_.DoIt(kTransportLocal, "HannibalLecter", 3, args));
+    ASSERT_EQ(1, br_.DoIt(kTransportLocal, "HannibalLecter", 3, args));
 }
 
 // Tests the 'adb bugreport' option when the device does not support 'bugreportz' - it falls back
@@ -282,6 +280,35 @@ TEST_F(BugreportTest, OkProgress) {
     ASSERT_EQ(0, br_.DoIt(kTransportLocal, "HannibalLecter", 2, args));
 }
 
+// Tests 'adb bugreport file.zip' when it succeeds and displays progress, even if progress recedes.
+TEST_F(BugreportTest, OkProgressAlwaysForward) {
+    ExpectBugreportzVersion("1.1");
+    ExpectProgress(1, 100);
+    ExpectProgress(50, 100);
+    ExpectProgress(75, 100);
+    // clang-format off
+    EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz -p", false, _))
+        // NOTE: DoAll accepts at most 10 arguments, and we're almost reached that limit...
+        .WillOnce(DoAll(
+            WithArg<4>(WriteOnStdout("BEGIN:/device/bugreport.zip\n")),
+            WithArg<4>(WriteOnStdout("PROGRESS:1/100\n")),
+            WithArg<4>(WriteOnStdout("PROGRESS:50/100\n")),
+            // 25 should be ignored becaused it receded.
+            WithArg<4>(WriteOnStdout("PROGRESS:25/100\n")),
+            WithArg<4>(WriteOnStdout("PROGRESS:75/100\n")),
+            // 75 should be ignored becaused it didn't change.
+            WithArg<4>(WriteOnStdout("PROGRESS:75/100\n")),
+            WithArg<4>(WriteOnStdout("OK:/device/bugreport.zip")),
+            WithArg<4>(ReturnCallbackDone())));
+    // clang-format on
+    EXPECT_CALL(br_, DoSyncPull(ElementsAre(StrEq("/device/bugreport.zip")), StrEq("file.zip"),
+                                true, StrEq("pulling file.zip")))
+        .WillOnce(Return(true));
+
+    const char* args[] = {"bugreport", "file.zip"};
+    ASSERT_EQ(0, br_.DoIt(kTransportLocal, "HannibalLecter", 2, args));
+}
+
 // Tests 'adb bugreport dir' when it succeeds and destination is a directory.
 TEST_F(BugreportTest, OkDirectory) {
     ExpectBugreportzVersion("1.1");