show .diff/Index properly as ignored if we fallback

Moving the code responsible for parsing the Index file from ::Done into
the slightly earlier ::VerifyDone allows us to still "fail" the download
if we can't make use of the Index for whatever reason, so that the
progress log correctly displays "Ign" instead of "Get" for the file.

This also makes quiet a few debug messages proper error messages (but
those are still hidden by default for Ign lines).

3 files changed, 94 insertions, 111 deletions

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 17e4797d1..3ce0f25cf 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -53,18 +53,6 @@
 
 using namespace std;
 
-static void printHashSumComparison(std::string const &URI, HashStringList const &Expected, HashStringList const &Actual) /*{{{*/
-{
-   if (_config->FindB("Debug::Acquire::HashSumMismatch", false) == false)
-      return;
-   std::cerr << std::endl << URI << ":" << std::endl << " Expected Hash: " << std::endl;
-   for (HashStringList::const_iterator hs = Expected.begin(); hs != Expected.end(); ++hs)
-      std::cerr <<  "\t- " << hs->toStr() << std::endl;
-   std::cerr << " Actual Hash: " << std::endl;
-   for (HashStringList::const_iterator hs = Actual.begin(); hs != Actual.end(); ++hs)
-      std::cerr <<  "\t- " << hs->toStr() << std::endl;
-}
-									/*}}}*/
 static std::string GetPartialFileName(std::string const &file)		/*{{{*/
 {
    std::string DestFile = _config->FindDir("Dir::State::lists") + "partial/";
@@ -1675,7 +1663,8 @@ void pkgAcqMetaClearSig::Finished()					/*{{{*/
 bool pkgAcqMetaClearSig::VerifyDone(std::string const &Message,		/*{{{*/
 	 pkgAcquire::MethodConfig const * const Cnf)
 {
-   Item::VerifyDone(Message, Cnf);
+   if (Item::VerifyDone(Message, Cnf) == false)
+      return false;
 
    if (FileExists(DestFile) && !StartsWithGPGClearTextSignature(DestFile))
       return RenameOnError(NotClearsigned);
@@ -2051,13 +2040,11 @@ void pkgAcqDiffIndex::QueueOnIMSHit() const				/*{{{*/
    new pkgAcqIndexDiffs(Owner, TransactionManager, Target, UsedMirror, Target.URI);
 }
 									/*}}}*/
-static bool RemoveFileForBootstrapLinking(bool const Debug, std::string const &For, std::string const &Boot)/*{{{*/
+static bool RemoveFileForBootstrapLinking(std::string &ErrorText, std::string const &For, std::string const &Boot)/*{{{*/
 {
    if (FileExists(Boot) && RemoveFile("Bootstrap-linking", Boot) == false)
    {
-      if (Debug)
-	 std::clog << "Bootstrap-linking for patching " << For
-	    << " by removing stale " << Boot << " failed!" << std::endl;
+      strprintf(ErrorText, "Bootstrap for patching %s by removing stale %s failed!", For.c_str(), Boot.c_str());
       return false;
    }
    return true;
@@ -2065,6 +2052,7 @@ static bool RemoveFileForBootstrapLinking(bool const Debug, std::string const &F
 									/*}}}*/
 bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)	/*{{{*/
 {
+   available_patches.clear();
    ExpectedAdditionalItems = 0;
    // failing here is fine: our caller will take care of trying to
    // get the complete file if patching fails
@@ -2108,8 +2096,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)	/*{{{*/
 
    if (ServerHashes.usable() == false)
    {
-      if (Debug == true)
-	 std::clog << "pkgAcqDiffIndex: " << IndexDiffFile << ": Did not find a good hashsum in the index" << std::endl;
+      ErrorText = "Did not find a good hashsum in the index";
       return false;
    }
 
@@ -2117,11 +2104,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)	/*{{{*/
    HashStringList const TargetFileHashes = GetExpectedHashesFor(Target.MetaKey);
    if (TargetFileHashes.usable() == false || ServerHashes != TargetFileHashes)
    {
-      if (Debug == true)
-      {
-	 std::clog << "pkgAcqDiffIndex: " << IndexDiffFile << ": Index has different hashes than parser, probably older, so fail pdiffing" << std::endl;
-         printHashSumComparison(CurrentPackagesFile, ServerHashes, TargetFileHashes);
-      }
+      ErrorText = "Index has different hashes than parser (probably older)";
       return false;
    }
 
@@ -2139,10 +2122,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)	/*{{{*/
 
    if (ServerHashes == LocalHashes)
    {
-      // we have the same sha1 as the server so we are done here
-      if(Debug)
-	 std::clog << "pkgAcqDiffIndex: Package file " << CurrentPackagesFile << " is up-to-date" << std::endl;
-      QueueOnIMSHit();
+      available_patches.clear();
       return true;
    }
 
@@ -2159,7 +2139,6 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)	/*{{{*/
       types.push_back(*type);
 
    // parse all of (provided) history
-   vector<DiffInfo> available_patches;
    bool firstAcceptedHashes = true;
    for (auto type = types.crbegin(); type != types.crend(); ++type)
    {
@@ -2214,9 +2193,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)	/*{{{*/
 
    if (unlikely(available_patches.empty() == true))
    {
-      if (Debug)
-	 std::clog << "pkgAcqDiffIndex: " << IndexDiffFile << ": "
-	    << "Couldn't find any patches for the patch series." << std::endl;
+      ErrorText = "Couldn't find any patches for the patch series";
       return false;
    }
 
@@ -2318,9 +2295,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)	/*{{{*/
 
    if (foundStart == false || unlikely(available_patches.empty() == true))
    {
-      if (Debug)
-	 std::clog << "pkgAcqDiffIndex: " << IndexDiffFile << ": "
-	    << "Couldn't find the start of the patch series." << std::endl;
+      ErrorText = "Couldn't find the start of the patch series";
       return false;
    }
 
@@ -2329,9 +2304,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)	/*{{{*/
 	    patch.patch_hashes.usable() == false ||
 	    patch.download_hashes.usable() == false)
       {
-	 if (Debug)
-	    std::clog << "pkgAcqDiffIndex: " << IndexDiffFile << ": provides no usable hashes for " << patch.file
-	       << " so fallback to complete download" << std::endl;
+	 strprintf(ErrorText, "Provides no usable hashes for %s", patch.file.c_str());
 	 return false;
       }
 
@@ -2339,9 +2312,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)	/*{{{*/
    unsigned long const fileLimit = _config->FindI("Acquire::PDiffs::FileLimit", 0);
    if (fileLimit != 0 && fileLimit < available_patches.size())
    {
-      if (Debug)
-	 std::clog << "Need " << available_patches.size() << " diffs (Limit is " << fileLimit
-	    << ") so fallback to complete download" << std::endl;
+      strprintf(ErrorText, "Need %lu diffs, but limit is %lu", available_patches.size(), fileLimit);
       return false;
    }
 
@@ -2371,25 +2342,18 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)	/*{{{*/
 	 unsigned long long const sizeLimit = downloadSizeIdx * sizeLimitPercent;
 	 if ((sizeLimit/100) < downloadSize)
 	 {
-	    if (Debug)
-	       std::clog << "Need " << downloadSize << " compressed bytes (Limit is " << (sizeLimit/100) << ", "
-		  << "original is " << downloadSizeIdx << ") so fallback to complete download" << std::endl;
+	    strprintf(ErrorText, "Need %llu compressed bytes, but limit is %llu and original is %llu", downloadSize, (sizeLimit/100), downloadSizeIdx);
 	    return false;
 	 }
       }
    }
 
-   // we have something, queue the diffs
-   string::size_type const last_space = Description.rfind(" ");
-   if(last_space != string::npos)
-      Description.erase(last_space, Description.size()-last_space);
-
    /* decide if we should download patches one by one or in one go:
       The first is good if the server merges patches, but many don't so client
       based merging can be attempt in which case the second is better.
       "bad things" will happen if patches are merged on the server,
       but client side merging is attempt as well */
-   bool pdiff_merge = _config->FindB("Acquire::PDiffs::Merge", true);
+   pdiff_merge = _config->FindB("Acquire::PDiffs::Merge", true);
    if (pdiff_merge == true)
    {
       // reprepro adds this flag if it has merged patches on the server
@@ -2404,53 +2368,24 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)	/*{{{*/
 	 return false;
       std::string const PartialFile = GetPartialFileNameFromURI(Target.URI);
       std::string const PatchedFile = GetKeepCompressedFileName(PartialFile + "-patched", Target);
-      if (RemoveFileForBootstrapLinking(Debug, CurrentPackagesFile, PartialFile) == false ||
-	    RemoveFileForBootstrapLinking(Debug, CurrentPackagesFile, PatchedFile) == false)
+      if (RemoveFileForBootstrapLinking(ErrorText, CurrentPackagesFile, PartialFile) == false ||
+	    RemoveFileForBootstrapLinking(ErrorText, CurrentPackagesFile, PatchedFile) == false)
 	 return false;
       for (auto const &ext : APT::Configuration::getCompressorExtensions())
       {
-	 if (RemoveFileForBootstrapLinking(Debug, CurrentPackagesFile, PartialFile + ext) == false ||
-	       RemoveFileForBootstrapLinking(Debug, CurrentPackagesFile, PatchedFile + ext) == false)
+	 if (RemoveFileForBootstrapLinking(ErrorText, CurrentPackagesFile, PartialFile + ext) == false ||
+	       RemoveFileForBootstrapLinking(ErrorText, CurrentPackagesFile, PatchedFile + ext) == false)
 	    return false;
       }
       std::string const Ext = Final.substr(CurrentPackagesFile.length());
       std::string const Partial = PartialFile + Ext;
       if (symlink(Final.c_str(), Partial.c_str()) != 0)
       {
-	 if (Debug)
-	    std::clog << "Bootstrap-linking for patching " << CurrentPackagesFile
-	       << " by linking " << Final << " to " << Partial << " failed!" << std::endl;
+	 strprintf(ErrorText, "Bootstrap for patching by linking %s to %s failed!", Final.c_str(), Partial.c_str());
 	 return false;
       }
    }
 
-   std::string indexURI = Desc.URI;
-   auto const byhashidx = indexURI.find("/by-hash/");
-   if (byhashidx != std::string::npos)
-      indexURI = indexURI.substr(0, byhashidx - strlen(".diff"));
-   else
-   {
-      auto end = indexURI.length() - strlen(".diff/Index");
-      if (CurrentCompressionExtension != "uncompressed")
-	 end -= (1 + CurrentCompressionExtension.length());
-      indexURI = indexURI.substr(0, end);
-   }
-
-   if (pdiff_merge == false)
-      new pkgAcqIndexDiffs(Owner, TransactionManager, Target, UsedMirror, indexURI, available_patches);
-   else
-   {
-      diffs = new std::vector<pkgAcqIndexMergeDiffs*>(available_patches.size());
-      for(size_t i = 0; i < available_patches.size(); ++i)
-	 (*diffs)[i] = new pkgAcqIndexMergeDiffs(Owner, TransactionManager,
-               Target, UsedMirror, indexURI,
-	       available_patches[i],
-	       diffs);
-   }
-
-   Complete = false;
-   Status = StatDone;
-   Dequeue();
    return true;
 }
 									/*}}}*/
@@ -2462,6 +2397,10 @@ void pkgAcqDiffIndex::Failed(string const &Message,pkgAcquire::MethodConfig cons
    Status = StatDone;
    ExpectedAdditionalItems = 0;
 
+   // queue for final move - this should happen even if we fail
+   // while parsing (e.g. on sizelimit) and download the complete file.
+   TransactionManager->TransactionStageCopy(this, DestFile, GetFinalFilename());
+
    if(Debug)
       std::clog << "pkgAcqDiffIndex failed: " << Desc.URI << " with " << Message << std::endl
 		<< "Falling back to normal index file acquire" << std::endl;
@@ -2469,6 +2408,21 @@ void pkgAcqDiffIndex::Failed(string const &Message,pkgAcquire::MethodConfig cons
    new pkgAcqIndex(Owner, TransactionManager, Target);
 }
 									/*}}}*/
+bool pkgAcqDiffIndex::VerifyDone(std::string const &Message, pkgAcquire::MethodConfig const * const)/*{{{*/
+{
+   string const FinalFile = GetFinalFilename();
+   if(StringToBool(LookupTag(Message,"IMS-Hit"),false))
+      DestFile = FinalFile;
+
+   if (ParseDiffIndex(DestFile))
+      return true;
+
+   Status = StatError;
+   if (ErrorText.empty())
+      ErrorText = "Couldn't parse pdiff index";
+   return false;
+}
+									/*}}}*/
 void pkgAcqDiffIndex::Done(string const &Message,HashStringList const &Hashes,	/*{{{*/
 			   pkgAcquire::MethodConfig const * const Cnf)
 {
@@ -2477,20 +2431,46 @@ void pkgAcqDiffIndex::Done(string const &Message,HashStringList const &Hashes,	/
 
    Item::Done(Message, Hashes, Cnf);
 
-   string const FinalFile = GetFinalFilename();
-   if(StringToBool(LookupTag(Message,"IMS-Hit"),false))
-      DestFile = FinalFile;
-
-   if(ParseDiffIndex(DestFile) == false)
+   if (available_patches.empty())
    {
-      Failed("Message: Couldn't parse pdiff index", Cnf);
-      // queue for final move - this should happen even if we fail
-      // while parsing (e.g. on sizelimit) and download the complete file.
-      TransactionManager->TransactionStageCopy(this, DestFile, FinalFile);
-      return;
+      // we have the same sha1 as the server so we are done here
+      if(Debug)
+	 std::clog << "pkgAcqDiffIndex: Package file is up-to-date" << std::endl;
+      QueueOnIMSHit();
+   }
+   else
+   {
+      // we have something, queue the diffs
+      string::size_type const last_space = Description.rfind(" ");
+      if(last_space != string::npos)
+	 Description.erase(last_space, Description.size()-last_space);
+
+      std::string indexURI = Desc.URI;
+      auto const byhashidx = indexURI.find("/by-hash/");
+      if (byhashidx != std::string::npos)
+	 indexURI = indexURI.substr(0, byhashidx - strlen(".diff"));
+      else
+      {
+	 auto end = indexURI.length() - strlen(".diff/Index");
+	 if (CurrentCompressionExtension != "uncompressed")
+	    end -= (1 + CurrentCompressionExtension.length());
+	 indexURI = indexURI.substr(0, end);
+      }
+
+      if (pdiff_merge == false)
+	 new pkgAcqIndexDiffs(Owner, TransactionManager, Target, UsedMirror, indexURI, available_patches);
+      else
+      {
+	 diffs = new std::vector<pkgAcqIndexMergeDiffs*>(available_patches.size());
+	 for(size_t i = 0; i < available_patches.size(); ++i)
+	    (*diffs)[i] = new pkgAcqIndexMergeDiffs(Owner, TransactionManager,
+		  Target, UsedMirror, indexURI,
+		  available_patches[i],
+		  diffs);
+      }
    }
 
-   TransactionManager->TransactionStageCopy(this, DestFile, FinalFile);
+   TransactionManager->TransactionStageCopy(this, DestFile, GetFinalFilename());
 
    Complete = true;
    Status = StatDone;
diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h
index 7741dbf22..343144122 100644
--- a/apt-pkg/acquire-item.h
+++ b/apt-pkg/acquire-item.h
@@ -686,6 +686,20 @@ class APT_HIDDEN pkgAcqIndex : public pkgAcqBaseIndex
              std::string const &Message, pkgAcquire::MethodConfig const * const Cnf);
 };
 									/*}}}*/
+struct APT_HIDDEN DiffInfo {						/*{{{*/
+   /** The filename of the diff. */
+   std::string file;
+
+   /** The hashes of the file after the diff is applied */
+   HashStringList result_hashes;
+
+   /** The hashes of the diff */
+   HashStringList patch_hashes;
+
+   /** The hashes of the compressed diff */
+   HashStringList download_hashes;
+};
+									/*}}}*/
 /** \brief An item that is responsible for fetching an index file of	{{{
  *  package list diffs and starting the package list's download.
  *
@@ -699,6 +713,8 @@ class APT_HIDDEN pkgAcqDiffIndex : public pkgAcqIndex
 {
    void * const d;
    std::vector<pkgAcqIndexMergeDiffs*> * diffs;
+   std::vector<DiffInfo> available_patches;
+   bool pdiff_merge;
 
  protected:
    /** \brief If \b true, debugging information will be written to std::clog. */
@@ -718,6 +734,7 @@ class APT_HIDDEN pkgAcqDiffIndex : public pkgAcqIndex
  public:
    // Specialized action members
    virtual void Failed(std::string const &Message, pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE;
+   virtual bool VerifyDone(std::string const &Message, pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE;
    virtual void Done(std::string const &Message, HashStringList const &Hashes,
 		     pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE;
    virtual std::string DescURI() const APT_OVERRIDE {return Target.URI + "Index";};
@@ -752,20 +769,6 @@ class APT_HIDDEN pkgAcqDiffIndex : public pkgAcqIndex
    APT_HIDDEN void QueueOnIMSHit() const;
 };
 									/*}}}*/
-struct APT_HIDDEN DiffInfo {						/*{{{*/
-   /** The filename of the diff. */
-   std::string file;
-
-   /** The hashes of the file after the diff is applied */
-   HashStringList result_hashes;
-
-   /** The hashes of the diff */
-   HashStringList patch_hashes;
-
-   /** The hashes of the compressed diff */
-   HashStringList download_hashes;
-};
-									/*}}}*/
 /** \brief An item that is responsible for fetching client-merge patches {{{
  *  that need to be applied to a given package index file.
  *
diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage
index 430551fa4..e7b052d49 100755
--- a/test/integration/test-pdiff-usage
+++ b/test/integration/test-pdiff-usage
@@ -50,7 +50,7 @@ wasmergeused() {
 	if echo "$*" | grep -q -- '-o test::cannot-use-pdiff=1'; then
 		msgtest 'Check if pdiff was' 'not used'
 		cp -a rootdir/tmp/testsuccess.output rootdir/tmp/aptupdate.output
-		testsuccess --nomsg grep "diff/Index with Message: Couldn't parse pdiff index" rootdir/tmp/aptupdate.output
+		testsuccess --nomsg grep "^Ign:.*  Packages\.diff/Index" rootdir/tmp/aptupdate.output
 		return;
 	fi
 
@@ -336,7 +336,7 @@ SHA256-Download:
 	generatereleasefiles '+1hour'
 	signreleasefiles
 	wasmergeused "$@" -o test::cannot-use-pdiff=1
-	testsuccess grep 'bytes (Limit is' rootdir/tmp/aptupdate.output
+	testsuccess grep 'bytes, but limit is' rootdir/tmp/aptupdate.output
 	testnopackage oldstuff
 	testsuccessequal "$(cat "${PKGFILE}-new")
 " aptcache show apt newstuff