Collect unprinted Ign errors for display in Err output

Instead of deciding if the first or the last error in a retry chain is
the best one to display if the entire chain fails in the end we teach
our progress reporting to collect the ignored errors (if it hasn't shown
them) and print them all together in the Err line.

We de-duplicate the collection which makes it harder to match the Ign
line with the error messages, but for these cases its probably better to
enable direct display of the text anyhow – and repeating the same message
multiple times is boring and probably not that helpful for a user
wanting to know why all the downloads failed even they asked apt to
retry 429 times…

References: 841243df877b39dfcb907a7fab9c4e8b4f65d715

4 files changed, 86 insertions, 19 deletions

diff --git a/apt-private/acqprogress.cc b/apt-private/acqprogress.cc
index b4b16e6a9..b8b540e75 100644
--- a/apt-private/acqprogress.cc
+++ b/apt-private/acqprogress.cc
@@ -123,39 +123,63 @@ void AcqTextStatus::Fail(pkgAcquire::ItemDesc &Itm)
    AssignItemID(Itm);
    clearLastLine();
 
-   bool ShowErrorText = true;
+   bool ShowErrorText = false;
    if (Itm.Owner->Status == pkgAcquire::Item::StatDone || Itm.Owner->Status == pkgAcquire::Item::StatIdle)
    {
       // TRANSLATOR: Very short word to be displayed for files in 'apt-get update'
       // which failed to download, but the error is ignored (compare "Err:")
       ioprintf(out, _("Ign:%lu %s"), Itm.Owner->ID, Itm.Description.c_str());
-      if (Itm.Owner->ErrorText.empty() ||
-	    _config->FindB("Acquire::Progress::Ignore::ShowErrorText", false) == false)
-	 ShowErrorText = false;
+      if (not Itm.Owner->ErrorText.empty())
+      {
+	 if (_config->FindB("Acquire::Progress::Ignore::ShowErrorText", false))
+	    ShowErrorText = true;
+	 else
+	 {
+	    if (auto const ignored = IgnoredErrorTexts.find(Itm.Owner->ID); ignored != IgnoredErrorTexts.end())
+	    {
+	       if (std::find(ignored->second.begin(), ignored->second.end(), Itm.Owner->ErrorText) == ignored->second.end())
+		  ignored->second.emplace_back(Itm.Owner->ErrorText);
+	    }
+	    else
+	       IgnoredErrorTexts.emplace(Itm.Owner->ID, std::vector<std::string>{Itm.Owner->ErrorText});
+	 }
+      }
    }
    else
    {
       // TRANSLATOR: Very short word to be displayed for files in 'apt-get update'
       // which failed to download and the error is critical (compare "Ign:")
       ioprintf(out, _("Err:%lu %s"), Itm.Owner->ID, Itm.Description.c_str());
+      ShowErrorText = true;
    }
 
-   if (ShowErrorText)
+   if (ShowErrorText && not Itm.Owner->ErrorText.empty())
    {
-      std::string::size_type line_start = 0;
-      std::string::size_type line_end;
-      while ((line_end = Itm.Owner->ErrorText.find_first_of("\n\r", line_start)) != std::string::npos) {
-	 out << std::endl << "  " << Itm.Owner->ErrorText.substr(line_start, line_end - line_start);
-	 line_start = Itm.Owner->ErrorText.find_first_not_of("\n\r", line_end + 1);
-	 if (line_start == std::string::npos)
-	    break;
+      auto const printErrorText = [](std::ostream &out, std::string_view errortext) {
+	 while (not errortext.empty())
+	 {
+	    auto const line_end = errortext.find_first_of("\n\r");
+	    out << "\n  " << errortext.substr(0, line_end);
+	    if (line_end == std::string_view::npos)
+	       break;
+	    errortext.remove_prefix(line_end);
+	    auto const line_start = errortext.find_first_not_of("\n\r");
+	    if (line_start == std::string_view::npos)
+	       break;
+	    errortext.remove_prefix(line_start);
+	 }
+      };
+      if (auto const ignored = IgnoredErrorTexts.find(Itm.Owner->ID); ignored != IgnoredErrorTexts.end())
+      {
+	 for (auto const &text : ignored->second)
+	    printErrorText(out, text);
+	 if (std::find(ignored->second.begin(), ignored->second.end(), Itm.Owner->ErrorText) == ignored->second.end())
+	    printErrorText(out, Itm.Owner->ErrorText);
       }
-      if (line_start == 0)
-	 out << std::endl << "  " << Itm.Owner->ErrorText;
-      else if (line_start != std::string::npos)
-	 out << std::endl << "  " << Itm.Owner->ErrorText.substr(line_start);
+      else
+	 printErrorText(out, Itm.Owner->ErrorText);
    }
-   out << std::endl;
+   out << '\n' << std::flush;
 
    Update = true;
 }
diff --git a/apt-private/acqprogress.h b/apt-private/acqprogress.h
index 87b957e4b..b4015861c 100644
--- a/apt-private/acqprogress.h
+++ b/apt-private/acqprogress.h
@@ -14,6 +14,8 @@
 
 #include <iostream>
 #include <string>
+#include <unordered_map>
+#include <vector>
 
 class APT_PUBLIC AcqTextStatus : public pkgAcquireStatus
 {
@@ -22,6 +24,7 @@ class APT_PUBLIC AcqTextStatus : public pkgAcquireStatus
    size_t LastLineLength;
    unsigned long ID;
    unsigned long Quiet;
+   std::unordered_map<unsigned long, std::vector<std::string>> IgnoredErrorTexts;
 
    APT_HIDDEN void clearLastLine();
    APT_HIDDEN void AssignItemID(pkgAcquire::ItemDesc &Itm);
diff --git a/test/integration/test-bug-869859-retry-downloads b/test/integration/test-bug-869859-retry-downloads
index 333ce816e..821d7bc3e 100755
--- a/test/integration/test-bug-869859-retry-downloads
+++ b/test/integration/test-bug-869859-retry-downloads
@@ -48,7 +48,17 @@ Ign:1 http://localhost:${APTHTTPPORT} stable/main all testpkg all 1
   429  Unknown HTTP code
 Err:1 http://localhost:${APTHTTPPORT} stable/main all testpkg all 1
   429  Unknown HTTP code
-E: Failed to fetch http://localhost:${APTHTTPPORT}/pool/testpkg_1_all.deb  429  Unknown HTTP code" apt download testpkg -o acquire::retries=3 -o debug::acquire::retries=true -q
+E: Failed to fetch http://localhost:${APTHTTPPORT}/pool/testpkg_1_all.deb  429  Unknown HTTP code" apt download testpkg -o acquire::retries=3 -o debug::acquire::retries=true -q -o Acquire::Progress::Ignore::ShowErrorText=true
+testfailure test -f testpkg_1_all.deb
+testsecondsgreaterequal 5 testfailureequal "Delaying http://localhost:${APTHTTPPORT} stable/main all testpkg all 1 by 1 seconds
+Ign:1 http://localhost:${APTHTTPPORT} stable/main all testpkg all 1
+Delaying http://localhost:${APTHTTPPORT} stable/main all testpkg all 1 by 2 seconds
+Ign:1 http://localhost:${APTHTTPPORT} stable/main all testpkg all 1
+Delaying http://localhost:${APTHTTPPORT} stable/main all testpkg all 1 by 4 seconds
+Ign:1 http://localhost:${APTHTTPPORT} stable/main all testpkg all 1
+Err:1 http://localhost:${APTHTTPPORT} stable/main all testpkg all 1
+  429  Unknown HTTP code
+E: Failed to fetch http://localhost:${APTHTTPPORT}/pool/testpkg_1_all.deb  429  Unknown HTTP code" apt download testpkg -o acquire::retries=3 -o debug::acquire::retries=true -q -o Acquire::Progress::Ignore::ShowErrorText=false
 testfailure test -f testpkg_1_all.deb
 
 msgmsg 'Retry 429 TooManyRequests after Retry-After time'
@@ -66,6 +76,25 @@ testsuccess apt download testpkg -o acquire::retries=3 -o acquire::retries::dela
 testsuccess test -f testpkg_1_all.deb
 rm -f testpkg_1_all.deb
 
+msgmsg 'Hard failure after retry request'
+webserverconfig 'aptwebserver::failrequest::pool/unavailable_1_all.deb' '1'
+testfailureequal "Ign:1 http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb
+  429  Unknown HTTP code
+Err:1 http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb
+  404  Not Found
+E: Failed to fetch http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb  404  Not Found
+E: Download Failed" apthelper download-file "http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb" 'unavailable_1_all.deb' -o acquire::retries=2 -o acquire::retries::delay=false -o Acquire::Progress::Ignore::ShowErrorText=true
+testfailure test -f unavailable_1_all.deb
+webserverconfig 'aptwebserver::failrequest::pool/unavailable_1_all.deb' '1'
+testfailureequal "Ign:1 http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb
+Err:1 http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb
+  429  Unknown HTTP code
+  404  Not Found
+E: Failed to fetch http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb  404  Not Found
+E: Download Failed" apthelper download-file "http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb" 'unavailable_1_all.deb' -o acquire::retries=2 -o acquire::retries::delay=false -o Acquire::Progress::Ignore::ShowErrorText=false
+testfailure test -f unavailable_1_all.deb
+
+
 msgmsg 'Do not try everything again, hard failures keep hard failures'
 webserverconfig 'aptwebserver::failrequest' '404'
 webserverconfig 'aptwebserver::failrequest::pool/testpkg_1_all.deb' '2'
diff --git a/test/integration/test-handle-redirect-as-used-mirror-change b/test/integration/test-handle-redirect-as-used-mirror-change
index a6f8b788f..1ce1232e2 100755
--- a/test/integration/test-handle-redirect-as-used-mirror-change
+++ b/test/integration/test-handle-redirect-as-used-mirror-change
@@ -67,7 +67,18 @@ Get:3 http://0.0.0.0:${APTHTTPPORT} unstable/main all Packages [$(stat -c %s apt
 Get:4 http://0.0.0.0:${APTHTTPPORT} unstable/main Translation-en [$(stat -c %s aptarchive/dists/unstable/main/i18n/Translation-en.gz) B]
 Reading package lists...
 Building dependency tree...
-All packages are up to date." apt update
+All packages are up to date." apt update -o Acquire::Progress::Ignore::ShowErrorText=true
+rm -rf rootdir/var/lib/apt/lists
+testsuccessequal "Get:1 http://0.0.0.0:${APTHTTPPORT}/storage unstable InRelease [$(stat -c %s aptarchive/storage/dists/unstable/InRelease) B]
+Ign:2 http://0.0.0.0:${APTHTTPPORT}/storage unstable/main Sources
+Ign:3 http://0.0.0.0:${APTHTTPPORT}/storage unstable/main all Packages
+Ign:4 http://0.0.0.0:${APTHTTPPORT}/storage unstable/main Translation-en
+Get:2 http://0.0.0.0:${APTHTTPPORT} unstable/main Sources [$(stat -c %s aptarchive/dists/unstable/main/source/Sources.gz) B]
+Get:3 http://0.0.0.0:${APTHTTPPORT} unstable/main all Packages [$(stat -c %s aptarchive/dists/unstable/main/binary-all/Packages.gz) B]
+Get:4 http://0.0.0.0:${APTHTTPPORT} unstable/main Translation-en [$(stat -c %s aptarchive/dists/unstable/main/i18n/Translation-en.gz) B]
+Reading package lists...
+Building dependency tree...
+All packages are up to date." apt update -o Acquire::Progress::Ignore::ShowErrorText=false
 
 find aptarchive -name 'InRelease' -delete
 rm -rf rootdir/var/lib/apt/lists