* UpdateManager/Common/aptsources.py:

  - fix "add_component" to correcly add components even under difficult conditions
* tests/data/sources.list.testDistribution:
  - changed the sources.list to make it sufficiently difficult for aptsources
* tests/test_aptsources.py:
  - added other testcase for add_component()

4 files changed, 75 insertions, 53 deletions

diff --git a/TODO b/TODO
index 1507f002..72acb345 100644
--- a/TODO
+++ b/TODO
@@ -1,3 +1,9 @@
+UpdateManager.Common.aptsources.py:
+- make the distro detection in sources.list more clever by using the
+  origin informaton to avoid adding full uris to (unofficial/internal)
+  mirrors 
+
+Misc:
 - have a common error dialog readymade and rib out all those 
   GtkMessageDialogs
 - add download size to treeview
diff --git a/UpdateManager/Common/aptsources.py b/UpdateManager/Common/aptsources.py
index 09673376..36a9811d 100644
--- a/UpdateManager/Common/aptsources.py
+++ b/UpdateManager/Common/aptsources.py
@@ -72,28 +72,26 @@ def uniq(s):
   """ simple and efficient way to return uniq list """
   return list(set(s))
 
-
-
-# actual source.list entries
 class SourceEntry:
-
+  """ single sources.list entry """
   def __init__(self, line,file=None):
-    self.invalid = False
-    self.disabled = False
-    self.type = ""
-    self.uri = ""
-    self.dist = ""
-    self.comps = []
-    self.comment = ""
-    self.line = line
-    if file == None:
+    self.invalid = False            # is the source entry valid
+    self.disabled = False           # is it disabled ('#' in front)
+    self.type = ""                  # what type (deb, deb-src)
+    self.uri = ""                   # base-uri
+    self.dist = ""                  # distribution (dapper, edgy, etc)
+    self.comps = []                 # list of available componetns (may empty)
+    self.comment = ""               # (optional) comment
+    self.line = line                # the original sources.list line
+    if file == None:                
       file = apt_pkg.Config.FindDir("Dir::Etc")+apt_pkg.Config.Find("Dir::Etc::sourcelist")
-    self.file = file
+    self.file = file               # the file that the entry is located in
     self.parse(line)
-    self.template = None
+    self.template = None           
     self.children = []
 
-  def __eq__(self, other):
+  def __eq__(self, other):         
+    """ equal operator for two sources.list entries """
     return (self.disabled == other.disabled and
             self.type == other.type and
             self.uri == other.uri and
@@ -101,8 +99,9 @@ class SourceEntry:
             self.comps == other.comps)
 
 
-  # works mostely like split but takes [] into account
   def mysplit(self, line):
+    """ a split() implementation that understands the sources.list
+        format better and takes [] into account (for e.g. cdroms) """
     line = string.strip(line)
     pieces = []
     tmp = ""
@@ -129,9 +128,9 @@ class SourceEntry:
       pieces.append(tmp)
     return pieces
 
-
-  # parse a given source line and split it into the fields we need
   def parse(self,line):
+    """ parse a given sources.list (textual) line and break it up
+        into the field we have """
     line  = string.strip(self.line)
     #print line
     # check if the source is enabled/disabled
@@ -177,11 +176,8 @@ class SourceEntry:
     else:
       self.comps = []
 
-    #print self.__dict__
-
-
-  # set enabled/disabled
   def set_enabled(self, new_value):
+    """ set a line to enabled or disabled """
     self.disabled = not new_value
     # enable, remove all "#" from the start of the line
     if new_value == True:
@@ -214,15 +210,17 @@ class SourceEntry:
     line += "\n"
     return line
     
-# the SourceList file as a class
 class NullMatcher(object):
+  """ a Matcher that does nothing """
   def match(self, s):
     return True
 
 class SourcesList:
-  def __init__(self, withMatcher=True,
+  """ represents the full sources.list + sources.list.d file """
+  def __init__(self,
+               withMatcher=True,
                matcherPath="/usr/share/update-manager/channels/"):
-    self.list = []      # of Type SourceEntries
+    self.list = []          # the actual SourceEntries Type 
     if withMatcher:
       self.matcher = SourceEntryMatcher(matcherPath)
     else:
@@ -230,6 +228,7 @@ class SourcesList:
     self.refresh()
 
   def refresh(self):
+    """ update the list of known entries """
     self.list = []
     # read sources.list
     dir = apt_pkg.Config.FindDir("Dir::Etc")
@@ -245,6 +244,8 @@ class SourcesList:
             self.matcher.match(source)
 
   def __iter__(self):
+    """ simple iterator to go over self.list, returns SourceEntry
+        types """
     for entry in self.list:
       yield entry
     raise StopIteration
@@ -298,6 +299,7 @@ class SourcesList:
     return new_entry
 
   def remove(self, source_entry):
+    """ remove the specified entry from the sources.list """
     self.list.remove(source_entry)
 
   def restoreBackup(self, backup_ext):
@@ -568,7 +570,6 @@ class Distribution:
                 self.source_code_sources.append(source)
             else:
                 self.disabled_sources.append(source)
-
     self.download_comps = set(comps)
     self.cdrom_comps = set(cdrom_comps)
     enabled_comps.extend(comps)
@@ -644,27 +645,28 @@ class Distribution:
     sourceslist:  an aptsource.sources_list
     comp:         the component that should be enabled
     """
-    def add_component_only_once(source, workpile):
+    def add_component_only_once(source, comps_per_dist):
         """
         Check if we already added the component to the repository, since
         a repository could be splitted into different apt lines. If not
         add the component
         """
-        if not (workpile.has_key(source.uri) and\
-                source.dist in workpile[source.uri]):
-            if comp not in source.comps:
-                source.comps.append(comp)
-                if workpile.has_key(source.uri):
-                       workpile[source.uri].append(source.dist)
-                else:
-                       workpile[source.uri] = [source.dist]
+        if comp in comps_per_dist[source.dist]:
+          return
+        source.comps.append(comp)
+        comps_per_dist[source.dist].add(comp)
 
     sources = []
     sources.extend(self.main_sources)
     sources.extend(self.child_sources)
     sources.extend(self.source_code_sources)
-    # store repos to which the new component has been added
-    workpile = {}
+    # store what comps are enabled already per distro (where distro is
+    # e.g. "dapper", "dapper-updates")
+    comps_per_dist = {}
+    for s in sources:
+      if not comps_per_dist.has_key(s.dist):
+        comps_per_dist[s.dist] = set()
+      map(comps_per_dist[s.dist].add, s.comps)
     # check if there is a main source at all
     if len(self.main_sources) < 1:
         # create a new main source
@@ -672,12 +674,12 @@ class Distribution:
     else:
         # add the comp to all main, child and source code sources
         for source in sources:
-             add_component_only_once(source, workpile)
+             add_component_only_once(source, comps_per_dist)
 
     if self.get_source_code == True:
         for source in self.source_code_sources:
             if comp not in source.comps: 
-                add_component_only_once(source, workpile)
+                add_component_only_once(source, comps_per_dist)
 
 
   def disable_component(self, sourceslist, comp):
diff --git a/tests/data/sources.list.testDistribution b/tests/data/sources.list.testDistribution
index dd900645..031e4c51 100644
--- a/tests/data/sources.list.testDistribution
+++ b/tests/data/sources.list.testDistribution
@@ -1,14 +1,9 @@
-# comment 1
 deb http://de.archive.ubuntu.com/ubuntu/ edgy main
-# comment 2
 deb http://de.archive.ubuntu.com/ubuntu/ edgy restricted
-# comment 3
 deb http://de.archive.ubuntu.com/ubuntu/ edgy universe
-# comment 4
 deb http://de.archive.ubuntu.com/ubuntu/ edgy-updates universe multiverse
-# comment 5
+deb http://de.archive.ubuntu.com/ubuntu/ edgy-updates restricted
 deb http://de.archive.ubuntu.com/ubuntu/ edgy-security main
-# comment 6
+deb http://de.archive.ubuntu.com/ubuntu/ edgy-security multiverse
 deb http://ftp.debian.org/debian sid main
-# comment 7
 deb http://de.archive.ubuntu.com/ubuntu/ breezy main
\ No newline at end of file
diff --git a/tests/test_aptsources.py b/tests/test_aptsources.py
index d5b5ff9f..1db99e3c 100644
--- a/tests/test_aptsources.py
+++ b/tests/test_aptsources.py
@@ -83,17 +83,36 @@ class TestAptSources(unittest.TestCase):
         distro.get_sources(sources)
         comp = "restricted"
         distro.enable_component(sources, comp)
-        found = 0
+        found = {}
         for entry in sources:
             if (entry.type == "deb" and
                 entry.uri == "http://de.archive.ubuntu.com/ubuntu/" and
-                entry.dist == "edgy"):
+                "edgy" in entry.dist):
+                for c in entry.comps:
+                    if c == comp:
+                        if not found.has_key(entry.dist):
+                            found[entry.dist] = 0
+                        found[entry.dist] += 1
+        #print "".join([s.str() for s in sources])
+        for key in found:
+            self.assertEqual(found[key], 1)
+
+        # add a not-already available component
+        comp = "multiverse"
+        distro.enable_component(sources, comp)
+        found = {}
+        for entry in sources:
+            if (entry.type == "deb" and
+                entry.uri == "http://de.archive.ubuntu.com/ubuntu/" and
+                "edgy" in entry.dist):
                 for c in entry.comps:
                     if c == comp:
-                        found += 1
-        print "".join([s.str() for s in sources])
-        self.assertEqual(found, 1)
-        
+                        if not found.has_key(entry.dist):
+                            found[entry.dist] = 0
+                        found[entry.dist] += 1
+        #print "".join([s.str() for s in sources])
+        for key in found:
+            self.assertEqual(found[key], 1)
 
 if __name__ == "__main__":
     unittest.main()