handle site-changing redirects as mirror changes

Redirectors like httpredir.debian.org orchestra the download from
multiple (hopefully close) mirrors while having only a single central
sources.list entry by using redirects. This has the effect that the
progress report always shows the source it started with, not the mirror
it ends up fetching from, which is especially problematic for error
reporting as having a report for a "Hashsum mismatch" for the redirector
URI is next to useless as nobody knows which URI it was really fetched
from (regardless of it coming from a user or via the report script) from
this output alone. You would need to enable debug output and hope for
the same situation to arise again…

We hence reuse the UsedMirror field of the mirror:// method and detect
redirects which change the site and declare this new site as the
UsedMirrror (and adapt the description).

The disadvantage is that there is no obvious mapping anymore (it is
relatively easy to guess through with some experience) from progress
lines to sources.list lines, so error messages need to take care to use
the Target description (rather than current Item description) if they
want to refer to the sources.list entry.

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 38f753cbb06078bb9ab86ee7ec1ac562b1accd08..8a566fea028aa98558971ea901b867344ead7c24 100644 (file)
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -457,7 +457,6 @@ void pkgAcquire::Item::Failed(string const &Message,pkgAcquire::MethodConfig con
 {
    if(ErrorText.empty())
       ErrorText = LookupTag(Message,"Message");
-   UsedMirror =  LookupTag(Message,"UsedMirror");
    if (QueueCounter <= 1)
    {
       /* This indicates that the file is not available right now but might
@@ -516,11 +515,10 @@ void pkgAcquire::Item::Start(string const &/*Message*/, unsigned long long const
 }
                                                                        /*}}}*/
 // Acquire::Item::Done - Item downloaded OK                            /*{{{*/
-void pkgAcquire::Item::Done(string const &Message, HashStringList const &Hashes,
+void pkgAcquire::Item::Done(string const &/*Message*/, HashStringList const &Hashes,
                            pkgAcquire::MethodConfig const * const /*Cnf*/)
 {
    // We just downloaded something..
-   UsedMirror = LookupTag(Message,"UsedMirror");
    if (FileSize == 0)
    {
       unsigned long long const downloadedSize = Hashes.FileSize();

diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc
index c0f93f9ce5305a9e1f44afe808135e187f98402a..dc03a8870a0a4360a104f0d348831c0b6b2a7c65 100644 (file)
--- a/apt-pkg/acquire-worker.cc
+++ b/apt-pkg/acquire-worker.cc
@@ -204,19 +204,22 @@ bool pkgAcquire::Worker::RunMessages()
         return _error->Error("Invalid message from method %s: %s",Access.c_str(),Message.c_str());
 
       string URI = LookupTag(Message,"URI");
-      pkgAcquire::Queue::QItem *Itm = 0;
+      pkgAcquire::Queue::QItem *Itm = NULL;
       if (URI.empty() == false)
         Itm = OwnerQ->FindItem(URI,this);
 
-      // update used mirror
-      string UsedMirror = LookupTag(Message,"UsedMirror", "");
-      if (!UsedMirror.empty() &&
-          Itm && 
-          Itm->Description.find(" ") != string::npos)
+      if (Itm != NULL)
       {
-         Itm->Description.replace(0, Itm->Description.find(" "), UsedMirror);
-         // FIXME: will we need this as well?
-         //Itm->ShortDesc = UsedMirror;
+        // update used mirror
+        string UsedMirror = LookupTag(Message,"UsedMirror", "");
+        if (UsedMirror.empty() == false)
+        {
+           for (pkgAcquire::Queue::QItem::owner_iterator O = Itm->Owners.begin(); O != Itm->Owners.end(); ++O)
+              (*O)->UsedMirror = UsedMirror;
+
+           if (Itm->Description.find(" ") != string::npos)
+              Itm->Description.replace(0, Itm->Description.find(" "), UsedMirror);
+        }
       }
 
       // Determine the message number and dispatch
@@ -248,17 +251,14 @@ bool pkgAcquire::Worker::RunMessages()
                break;
             }
 
-            string NewURI = LookupTag(Message,"New-URI",URI.c_str());
+           std::string const NewURI = LookupTag(Message,"New-URI",URI.c_str());
             Itm->URI = NewURI;
 
            ItemDone();
 
            // Change the status so that it can be dequeued
            for (pkgAcquire::Queue::QItem::owner_iterator O = Itm->Owners.begin(); O != Itm->Owners.end(); ++O)
-           {
-              pkgAcquire::Item *Owner = *O;
-              Owner->Status = pkgAcquire::Item::StatIdle;
-           }
+              (*O)->Status = pkgAcquire::Item::StatIdle;
            // Mark the item as done (taking care of all queues)
            // and then put it in the main queue again
            std::vector<Item*> const ItmOwners = Itm->Owners;
@@ -267,12 +267,28 @@ bool pkgAcquire::Worker::RunMessages()
            for (pkgAcquire::Queue::QItem::owner_iterator O = ItmOwners.begin(); O != ItmOwners.end(); ++O)
            {
               pkgAcquire::Item *Owner = *O;
-              pkgAcquire::ItemDesc desc = Owner->GetItemDesc();
+              pkgAcquire::ItemDesc &desc = Owner->GetItemDesc();
+              // if we change site, treat it as a mirror change
+              if (URI::SiteOnly(NewURI) != URI::SiteOnly(desc.URI))
+              {
+                 std::string const OldSite = desc.Description.substr(0, desc.Description.find(" "));
+                 if (likely(APT::String::Startswith(desc.URI, OldSite)))
+                 {
+                    std::string const OldExtra = desc.URI.substr(OldSite.length() + 1);
+                    if (likely(APT::String::Endswith(NewURI, OldExtra)))
+                    {
+                       std::string const NewSite = NewURI.substr(0, NewURI.length() - OldExtra.length());
+                       Owner->UsedMirror = URI::ArchiveOnly(NewSite);
+                       if (desc.Description.find(" ") != string::npos)
+                          desc.Description.replace(0, desc.Description.find(" "), Owner->UsedMirror);
+                    }
+                 }
+              }
               desc.URI = NewURI;
               OwnerQ->Owner->Enqueue(desc);
 
               if (Log != 0)
-                 Log->Done(Owner->GetItemDesc());
+                 Log->Done(desc);
            }
             break;
          }

diff --git a/test/integration/test-handle-redirect-as-used-mirror-change b/test/integration/test-handle-redirect-as-used-mirror-change
new file mode 100755 (executable)
index 0000000..08a39a5
--- /dev/null
+++ b/test/integration/test-handle-redirect-as-used-mirror-change
@@ -0,0 +1,23 @@
+#!/bin/sh
+set -e
+
+TESTDIR=$(readlink -f $(dirname $0))
+. $TESTDIR/framework
+setupenvironment
+configarchitecture 'amd64'
+configcompression '.' 'gz'
+
+buildsimplenativepackage 'unrelated' 'all' '0.5~squeeze1' 'unstable'
+
+setupaptarchive --no-update
+changetowebserver -o 'aptwebserver::redirect::replace::/redirectme/=http://0.0.0.0:8080/'
+rewritesourceslist 'http://localhost:8080/redirectme'
+
+testsuccessequal "Get:1 http://0.0.0.0:8080 unstable InRelease [$(stat -c %s aptarchive/dists/unstable/InRelease) B]
+Get:2 http://0.0.0.0:8080 unstable/main Sources [$(stat -c %s aptarchive/dists/unstable/main/source/Sources.gz) B]
+Get:3 http://0.0.0.0:8080 unstable/main amd64 Packages [$(stat -c %s aptarchive/dists/unstable/main/binary-amd64/Packages.gz) B]
+Get:4 http://0.0.0.0:8080 unstable/main Translation-en [$(stat -c %s aptarchive/dists/unstable/main/i18n/Translation-en.gz) B]
+Reading package lists..." aptget update
+
+testsuccessequal 'Hit:1 http://0.0.0.0:8080 unstable InRelease
+Reading package lists...' aptget update