From: Fabian Groffen <grobian@gentoo.org>
Date: Mon, 23 Jul 2007 17:18:12 +0000 (-0000)
Subject:    Merged from trunk 7353:7371
X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=8d1c06a25733ab9fbf5fa1a79068097ea23fa056;p=portage.git

   Merged from trunk 7353:7371

   +------------------------------------------------------------------------+
   | 7354      | Run pychecker over everything, fix obvious thing slike     |
   | WarnerBro | modules not in use, variables not being used, or global    |
   |           | exceptions not being deleted. Also fix imports for style   |
   |           | (1 per line)                                               |
   |-----------+------------------------------------------------------------|
   | 7355      | remove unused vars from xpak, fix bug in compose. Also fix |
   | WarnerBro | tests for portage sets, fix misnamed variable.             |
   |-----------+------------------------------------------------------------|
   | 7356      | Add notes about import statements (clarity) and fix        |
   | WarnerBro | spelling error in release-notes                            |
   |-----------+------------------------------------------------------------|
   | 7357      | Fix spacing issues, imports                                |
   | WarnerBro |                                                            |
   |-----------+------------------------------------------------------------|
   | 7358      | For bug #184774, put lha's -q option last since otherwise  |
   | zmedico   | the option parser now requires level 0, 1, or 2 to be      |
   |           | specified.                                                 |
   |-----------+------------------------------------------------------------|
   | 7360      | add test for CommandOutputSet                              |
   | WarnerBro |                                                            |
   |-----------+------------------------------------------------------------|
   | 7361      | add docstring, fix up imports (style), add keyword         |
   | WarnerBro | expansion                                                  |
   |-----------+------------------------------------------------------------|
   | 7362      | fix missing import, default value for value in case we try |
   | WarnerBro | to .join on a variable that hasn't been assigned to yet    |
   |-----------+------------------------------------------------------------|
   | 7363      | Fix hardcoded /usr/bin/echo to use                         |
   | zmedico   | portage.process.find_binary().                             |
   |-----------+------------------------------------------------------------|
   | 7364      | fix spacing, comments, default args                        |
   | WarnerBro |                                                            |
   |-----------+------------------------------------------------------------|
   | 7365      | Move common file functionality into a FileLoader class and |
   | WarnerBro | delegate parsing to subclasses via a lineParser func, no   |
   |           | idea how this will work performance wise. Fix errors in    |
   |           | parser (blank lines are not errors ;))                     |
   |-----------+------------------------------------------------------------|
   | 7366      | Move classes out of __init__.py, they do not belong there. |
   | WarnerBro | Fix import style, and import statements broken by class    |
   |           | moves.                                                     |
   |-----------+------------------------------------------------------------|
   | 7367      | Fix imports for style, deprecate grab*, what is            |
   | WarnerBro | foo.metadata and what does it contain                      |
   |-----------+------------------------------------------------------------|
   | 7368      | Validator should be a function, not a class, fix           |
   | WarnerBro | spacing/tab issues. Fix parsing errors for                 |
   |           | KeyValuePairLoader. Add a GenericFileLoader class that     |
   |           | uses more than 1 loader in a vain attempt to figure out    |
   |           | what kind of file it is. Fix inheritance by renaming       |
   |           | UserConfigKlass                                            |
   |-----------+------------------------------------------------------------|
   | 7369      | Fix imports for style, slight optimization for grab*       |
   | WarnerBro | function, doc fixups.                                      |
   |-----------+------------------------------------------------------------|
   | 7370      | fix broken import                                          |
   | WarnerBro |                                                            |
   |-----------+------------------------------------------------------------|
   | 7371      | add more tests for keyvaluepairloader (it had a bug        |
   | WarnerBro | previously) and fix up other misc tests                    |
   +------------------------------------------------------------------------+

svn path=/main/branches/prefix/; revision=7372
---

diff --git a/DEVELOPING b/DEVELOPING
index 01eb9bd42..725f1ae1e 100644
--- a/DEVELOPING
+++ b/DEVELOPING
@@ -65,3 +65,40 @@ except KeyError:
 	dict[foo] = default_value
 
 The get call is nicer (compact) and faster (try,except are slow).
+
+Imports
+-------
+
+Import things one per line
+
+YES:
+  import os
+  import time
+  import sys
+
+NO:
+  import os,sys,time
+
+When importing from a module, you may import more than 1 thing at a time.
+
+YES:
+  from portage.module import foo, bar, baz
+
+Multiline imports are ok (for now :))
+
+Try to group system and package imports separately.
+
+YES:
+  import os
+  import sys
+  import time
+
+  from portage.locks import lockfile
+  from portage.versions import vercmp
+
+NO:
+  import os
+  import portage
+  import portage.util
+  import time
+  import sys
diff --git a/RELEASE-NOTES b/RELEASE-NOTES
index f6989a14d..ff3370ef2 100644
--- a/RELEASE-NOTES
+++ b/RELEASE-NOTES
@@ -16,7 +16,7 @@ portage-2.2
   used in ACCEPT_KEYWORDS.
   For packages that don't specify any other KEYWORDS you can use the new ** token
   as documented in portage(5) to disable KEYWORDS filtering completely.
-* Portage now warns if a ebild repository does not have a name, as several new
+* Portage now warns if an ebuild repository does not have a name, as several new
   features in 2.2 make use of or require named repositories. The repository name
   is stored in profiles/repo_name in each repository.
 
diff --git a/bin/ebuild.sh b/bin/ebuild.sh
index ec930e4a6..483755aa1 100755
--- a/bin/ebuild.sh
+++ b/bin/ebuild.sh
@@ -378,7 +378,7 @@ unpack() {
 				unrar x -idq -o+ "${srcdir}/${x}" || die "$myfail"
 				;;
 			LHa|LHA|lha|lzh)
-				lha xqf "${srcdir}/${x}" || die "$myfail"
+				lha xfq "${srcdir}/${x}" || die "$myfail"
 				;;
 			deb)
 				# Unpacking .deb archives can not always be done with
diff --git a/pym/emerge/__init__.py b/pym/emerge/__init__.py
index 6ed424f50..e746ce130 100644
--- a/pym/emerge/__init__.py
+++ b/pym/emerge/__init__.py
@@ -53,7 +53,7 @@ from portage.const import EPREFIX
 from portage.data import secpass
 from portage.util import normalize_path as normpath
 from portage.util import writemsg
-from portage.sets import InternalPackageSet
+from portage.sets.dbapi import InternalPackageSet
 from portage.sets.profiles import PackagesSystemSet as SystemSet
 from portage.sets.files import WorldSet
 
diff --git a/pym/portage/checksum.py b/pym/portage/checksum.py
index 4b3e82712..3e9bb8a68 100644
--- a/pym/portage/checksum.py
+++ b/pym/portage/checksum.py
@@ -30,14 +30,14 @@ def _generate_hash_function(hashtype, hashobject, origin="unknown"):
 		blocksize = HASHING_BLOCKSIZE
 		data = f.read(blocksize)
 		size = 0L
-		sum = hashobject()
+		checksum = hashobject()
 		while data:
-			sum.update(data)
+			checksum.update(data)
 			size = size + len(data)
 			data = f.read(blocksize)
 		f.close()
 
-		return (sum.hexdigest(), size)
+		return (checksum.hexdigest(), size)
 	hashfunc_map[hashtype] = pyhash
 	hashorigin_map[hashtype] = origin
 	return pyhash
diff --git a/pym/portage/env/config.py b/pym/portage/env/config.py
index a354dcf7d..c990d9f0e 100644
--- a/pym/portage/env/config.py
+++ b/pym/portage/env/config.py
@@ -6,19 +6,18 @@
 from UserDict import UserDict
 from portage.env.loaders import KeyListFileLoader, KeyValuePairFileLoader, ItemFileLoader
 
-class UserConfigKlass(UserDict,object):
+class ConfigLoaderKlass(UserDict, object):
 	"""
 	A base class stub for things to inherit from.
 	Users may want a non-file backend.
 	"""
 	
-	data = {}
-	
 	def __init__(self, loader):
 		"""
 		@param loader: A class that has a load() that returns two dicts
 			the first being a data dict, the second being a dict of errors.
 		"""
+		UserDict.__init__(self)
 		self._loader = loader
 
 	def load(self):
@@ -33,9 +32,32 @@ class UserConfigKlass(UserDict,object):
 	def __iter__(self):
 		return iter(self.data)
 
-class PackageKeywordsFile(UserConfigKlass):
+class GenericFile(UserDict):
+	"""
+	Inherits from ConfigLoaderKlass, attempts to use all known loaders
+	until it gets <something> in data.  This is probably really slow but is
+	helpful when you really have no idea what you are loading (hint hint the file
+	should perhaps declare  what type it is? ;)
+	"""
+	
+	loaders = [KeyListFileLoader, KeyValuePairFileLoader, ItemFileLoader]
+	
+	def __init__(self, filename):
+		UserDict.__init__(self)
+		self.filename = filename
+	
+	def load(self):
+		for loader in self.loaders:
+			l = loader(self.filename, None)
+			data, errors = l.load()
+			if len(data) and not len(errors):
+				(self.data, self.errors) = (data, errors)
+				return
+
+
+class PackageKeywordsFile(ConfigLoaderKlass):
 	"""
-	Inherits from UserConfigKlass; implements a file-based backend.
+	Inherits from ConfigLoaderKlass; implements a file-based backend.
 	"""
 
 	default_loader = KeyListFileLoader
@@ -44,7 +66,7 @@ class PackageKeywordsFile(UserConfigKlass):
 		super(PackageKeywordsFile, self).__init__(
 			self.default_loader(filename, validator=None))
 	
-class PackageUseFile(UserConfigKlass):
+class PackageUseFile(ConfigLoaderKlass):
 	"""
 	Inherits from PackageUse; implements a file-based backend.  Doesn't handle recursion yet.
 	"""
@@ -54,7 +76,7 @@ class PackageUseFile(UserConfigKlass):
 		super(PackageUseFile, self).__init__(
 			self.default_loader(filename, validator=None))
 	
-class PackageMaskFile(UserConfigKlass):
+class PackageMaskFile(ConfigLoaderKlass):
 	"""
 	A class that implements a file-based package.mask
 	
@@ -72,7 +94,7 @@ class PackageMaskFile(UserConfigKlass):
 		super(PackageMaskFile, self).__init__(
 			self.default_loader(filename, validator=None))
 
-class PortageModulesFile(UserConfigKlass):
+class PortageModulesFile(ConfigLoaderKlass):
 	"""
 	File Class for /etc/portage/modules
 	"""
diff --git a/pym/portage/env/loaders.py b/pym/portage/env/loaders.py
index 3a2d7c257..ae9579a86 100644
--- a/pym/portage/env/loaders.py
+++ b/pym/portage/env/loaders.py
@@ -15,17 +15,19 @@ class LoaderError(Exception):
 		@type error_msg: String
 		"""
 
-		self.resource
+		self.resource = resource
+		self.error_msg = error_msg
 	
 	def __str__(self):
 		return "Failed while loading resource: %s, error was: %s" % (
-			resource, error_msg)
+			self.resource, self.error_msg)
+
 
 def RecursiveFileLoader(filename):
 	"""
-	If filename is of type file, return [filename]
-	else if filename is of type directory, return an array
-	full of files in that directory to process.
+	If filename is of type file, return a generate that yields filename
+	else if filename is of type directory, return a generator that fields
+	files in that directory.
 	
 	Ignore files beginning with . or ending in ~.
 	Prune CVS directories.
@@ -36,36 +38,85 @@ def RecursiveFileLoader(filename):
 	"""
 
 	if os.path.isdir(filename):
-		for root, dirs, files in os.walk(self.fname):
+		for root, dirs, files in os.walk(filename):
 			if 'CVS' in dirs:
 				dirs.remove('CVS')
-			files = filter(files,startswith('.'))
-			files = filter(files,endswith('~'))
-			for file in files:
-				yield file
+			files = filter(files, str.startswith('.'))
+			files = filter(files, str.endswith('~'))
+			for f in files:
+				yield f
 	else:
 		yield filename
 
+
 class DataLoader(object):
 
-	def __init__(self, validator=None):
+	def __init__(self, validator):
 		f = validator
 		if f is None:
 			# if they pass in no validator, just make a fake one
 			# that always returns true
-			class AlwaysTrue(object):
-				def validate(self, key):
-					return True
-			f = AlwaysTrue()
-		self._validator = f
+			def validate(key):
+				return True
+			f = validate
+		self._validate = f
 
 	def load(self):
 		"""
 		Function to do the actual work of a Loader
 		"""
-		pass
+		raise NotImplementedError("Please override in a subclass")
+
+
+class FileLoader(DataLoader):
+	""" Class to access data in files """
+
+	def __init__(self, filename, validator):
+		"""
+			Args:
+				filename : Name of file or directory to open
+				validator : class with validate() method to validate data.
+		"""
+		DataLoader.__init__(self, validator)
+		self.fname = filename
+
+	def load(self):
+		"""
+		Return the {source: {key: value}} pairs from a file
+		Return the {source: [list of errors] from a load
+
+		@param recursive: If set and self.fname is a directory; 
+			load all files in self.fname
+		@type: Boolean
+		@rtype: tuple
+		@returns:
+		Returns (data,errors), both may be empty dicts or populated.
+		"""
+		data = {}
+		errors = {}
+		# I tried to save a nasty lookup on lineparser by doing the lookup
+		# once, which may be expensive due to digging in child classes.
+		func = self.lineParser
+		for fn in RecursiveFileLoader(self.fname):
+			f = open(fn, 'rb')
+			for line_num, line in enumerate(f):
+				func(line, line_num, data, errors)
+		return (data, errors)
+
+	def lineParser(self, line, line_num, data, errors):
+		""" This function parses 1 line at a time
+			Args:
+				line: a string representing 1 line of a file
+				line_num: an integer representing what line we are processing
+				data: a dict that contains the data we have extracted from the file
+				      already
+				errors: a dict representing parse errors.
+			Returns:
+				Nothing (None).  Writes to data and errors
+		"""
+		raise NotImplementedError("Please over-ride this in a child class")
 
-class ItemFileLoader(DataLoader):
+class ItemFileLoader(FileLoader):
 	"""
 	Class to load data from a file full of items one per line
 	
@@ -78,36 +129,31 @@ class ItemFileLoader(DataLoader):
 	Note that due to the data store being a dict, duplicates
 	are removed.
 	"""
-	
-	_recursive = False
 
 	def __init__(self, filename, validator):
-		DataLoader.__init__(self, validator)
-		self.fname = filename
+		FileLoader.__init__(self, filename, validator)
 	
-	def load(self):
-		data = {}
-		errors = {}
-		for file in RecursiveFileLoader(self.fname):
-			f = open(file, 'rb')
-			for line_num, line in enumerate(f):
-				if line.startswith('#'):
-					continue
-				split = line.strip().split()
-				if not len(split):
-					errors.setdefault(self.fname,[]).append(
-					"Malformed data at line: %s, data: %s"
-					% (line_num + 1, split))
-				key = split[0]
-				if not self._validator.validate(key):
-					errors.setdefault(self.fname,[]).append(
-					"Validation failed at line: %s, data %s"
-					% (line_num + 1, split))
-					continue
-				data[key] = None
-		return (data, errors)
-
-class KeyListFileLoader(DataLoader):
+	def lineParser(self, line, line_num, data, errors):
+		line = line.strip()
+		if line.startswith('#'): # Skip commented lines
+			return
+		if not len(line): # skip empty lines
+			return
+		split = line.split()
+		if not len(split):
+			errors.setdefault(self.fname, []).append(
+				"Malformed data at line: %s, data: %s"
+				% (line_num + 1, line))
+			return
+		key = split[0]
+		if not self._validate(key):
+			errors.setdefault(self.fname, []).append(
+				"Validation failed at line: %s, data %s"
+				% (line_num + 1, key))
+			return
+		data[key] = None
+
+class KeyListFileLoader(FileLoader):
 	"""
 	Class to load data from a file full of key [list] tuples
 	
@@ -116,40 +162,36 @@ class KeyListFileLoader(DataLoader):
 	{'key':['foo1','foo2','foo3']}
 	"""
 
-	_recursive = False
-
 	def __init__(self, filename, validator):
-		DataLoader.__init__(self, validator)
-		self.fname = filename
-
-	def load(self):
-		data = {}
-		errors = {}
-		for file in RecursiveFileLoader(self.fname):
-			f = open(file, 'rb')
-			for line_num, line in enumerate(f):
-				if line.startswith('#'):
-					continue
-				split = line.strip().split()
-				if len(split) < 2:
-					errors.setdefault(self.fname,[]).append(
-					"Malformed data at line: %s, data: %s"
-					% (line_num + 1, split))
-					continue
-				key = split[0]
-				value = split[1:]
-				if not self._validator.validate(key):
-					errors.setdefault(self.fname,[]).append(
-					"Validation failed at line: %s, data %s"
-					% (line_num + 1, split))
-					continue
-				if key in data:
-					data[key].append(value)
-				else:
-					data[key] = value
-		return (data, errors)
-
-class KeyValuePairFileLoader(DataLoader):
+		FileLoader.__init__(self, filename, validator)
+
+
+	def lineParser(self, line, line_num, data, errors):
+		line = line.strip()
+		if line.startswith('#'): # Skip commented lines
+			return
+		if not len(line): # skip empty lines
+			return
+		split = line.split()
+		if len(split) < 2:
+			errors.setdefault(self.fname, []).append(
+				"Malformed data at line: %s, data: %s"
+				% (line_num + 1, line))
+			return
+		key = split[0]
+		value = split[1:]
+		if not self._validate(key):
+			errors.setdefault(self.fname, []).append(
+				"Validation failed at line: %s, data %s"
+				% (line_num + 1, key))
+			return
+		if key in data:
+			data[key].append(value)
+		else:
+			data[key] = value
+
+
+class KeyValuePairFileLoader(FileLoader):
 	"""
 	Class to load data from a file full of key=value pairs
 	
@@ -160,47 +202,36 @@ class KeyValuePairFileLoader(DataLoader):
 	 'foo':'bar'}
 	"""
 
-	_recursive = False
-
 	def __init__(self, filename, validator):
-		DataLoader.__init__(self, validator)
-		self.fname = filename
+		FileLoader.__init__(self, filename, validator)
+
+
+	def lineParser(self, line, line_num, data, errors):
+		line = line.strip()
+		if line.startswith('#'): # skip commented lines
+			return
+		if not len(line): # skip empty lines
+			return
+		split = line.split('=')
+		if len(split) < 2:
+			errors.setdefault(self.fname, []).append(
+				"Malformed data at line: %s, data %s"
+				% (line_num + 1, line))
+			return
+		key = split[0]
+		value = split[1:]
+		if not key:
+			errors.setdefault(self.fname, []).append(
+				"Malformed key at line: %s, key %s"
+				% (line_num + 1, key))
+			return
+		if not self._validate(key):
+			errors.setdefault(self.fname, []).append(
+				"Validation failed at line: %s, data %s"
+				% (line_num + 1, key))
+			return
+		if key in data:
+			data[key].append(value)
+		else:
+			data[key] = value
 
-	def load(self):
-		"""
-		Return the {source: {key: value}} pairs from a file
-		Return the {source: [list of errors] from a load
-
-		@param recursive: If set and self.fname is a directory; 
-			load all files in self.fname
-		@type: Boolean
-		@rtype: tuple
-		@returns:
-		Returns (data,errors), both may be empty dicts or populated.
-		"""
-
-		DataLoader.load(self)
-		data = {}
-		errors = {}
-		for file in RecursiveFileLoader(self.fname):
-			f = open(file, 'rb')
-			for line_num, line in enumerate(f):
-				if line.startswith('#'):
-					continue
-				split = line.strip().split('=')
-				if len(split) < 2:
-					errors.setdefault(self.fname,[]).append(
-					"Malformed data at line: %s, data %s"
-					% (line_num + 1, split))
-				key = split[0]
-				value = split[1:]
-				if not self._validator.validate(key):
-					errors.setdefault(self.fname,[]).append(
-					"Validation failed at line: %s, data %s"
-					% (line_num + 1, split))
-					continue
-				if key in data:
-					data[key].append(value)
-				else:
-					data[key] = value
-		return (data, errors)
diff --git a/pym/portage/env/validators.py b/pym/portage/env/validators.py
new file mode 100644
index 000000000..fb29b8d29
--- /dev/null
+++ b/pym/portage/env/validators.py
@@ -0,0 +1,21 @@
+# validators.py Portage File Loader Code
+# Copyright 2007 Gentoo Foundation
+# $Id$
+
+from portage.dep import isvalidatom
+
+ValidAtomValidator = isvalidatom
+
+def PackagesFileValidator(atom):
+	""" This function mutates atoms that begin with - or *
+	    It then checks to see if that atom is valid, and if
+	    so returns True, else it returns False.
+	    
+	    Args:
+		atom: a string representing an atom such as sys-apps/portage-2.1
+	"""
+	if atom.startswith("*") or atom.startswith("-"):
+		atom = atom[1:]
+	if not isvalidatom(atom):
+		return False
+	return True
diff --git a/pym/portage/getbinpkg.py b/pym/portage/getbinpkg.py
index 3e566735d..1d2e5bcb6 100644
--- a/pym/portage/getbinpkg.py
+++ b/pym/portage/getbinpkg.py
@@ -4,7 +4,14 @@
 # $Id: getbinpkg.py 3483 2006-06-10 21:40:40Z genone $
 
 from portage.output import red, yellow, green
-import htmllib,HTMLParser,formatter,sys,os,portage.xpak,time,tempfile,base64,urllib2
+import portage.xpak
+import HTMLParser
+import sys
+import os
+import time
+import tempfile
+import base64
+import urllib2
 from portage.const import CACHE_PATH
 
 try:
@@ -14,16 +21,12 @@ except ImportError:
 
 try:
 	import ftplib
-except SystemExit, e:
-	raise
-except Exception, e:
+except ImportError, e:
 	sys.stderr.write(red("!!! CANNOT IMPORT FTPLIB: ")+str(e)+"\n")
 
 try:
 	import httplib
-except SystemExit, e:
-	raise
-except Exception, e:
+except ImportError, e:
 	sys.stderr.write(red("!!! CANNOT IMPORT HTTPLIB: ")+str(e)+"\n")
 
 def make_metadata_dict(data):
@@ -267,11 +270,11 @@ def match_in_array(array, prefix="", suffix="", match_both=1, allow_overlap=0):
 
 		if not allow_overlap: # Not allow to overlap prefix and suffix
 			if len(x) >= (len(prefix)+len(suffix)):
-				y = x[len(prefix):]
+				pass
 			else:
 				continue          # Too short to match.
 		else:
-			y = x               # Do whatever... We're overlapping.
+			pass                      # Do whatever... We're overlapping.
 		
 		if suffix and (len(x) >= len(suffix)) and (x[-len(suffix):] == suffix):
 			myarray.append(x)   # It matches
@@ -457,8 +460,6 @@ def dir_get_metadata(baseurl, conn=None, chunk_size=3000, verbose=1, usingcache=
 
 	conn,protocol,address,params,headers = create_conn(baseurl, conn)
 
-	filedict = {}
-
 	try:
 		metadatafile = open(CACHE_PATH+"/remote_metadata.pickle")
 		metadata = cPickle.load(metadatafile)
@@ -485,7 +486,6 @@ def dir_get_metadata(baseurl, conn=None, chunk_size=3000, verbose=1, usingcache=
 	# Determine if our metadata file is current.
 	metalist.sort()
 	metalist.reverse() # makes the order new-to-old.
-	havecache=0
 	for mfile in metalist:
 		if usingcache and \
 		   ((metadata[baseurl]["indexname"] != mfile) or \
@@ -651,7 +651,6 @@ class PackageIndex(object):
 		cpv_all = self.packages.keys()
 		cpv_all.sort()
 		if self.modified:
-			import time
 			self.header["TIMESTAMP"] = str(long(time.time()))
 			self.header["PACKAGES"] = str(len(cpv_all))
 		keys = self.header.keys()
diff --git a/pym/portage/glsa.py b/pym/portage/glsa.py
index d8053a820..5e2d5f081 100644
--- a/pym/portage/glsa.py
+++ b/pym/portage/glsa.py
@@ -5,8 +5,6 @@
 import os
 import sys
 import urllib
-import time
-import codecs
 import re
 import xml.dom.minidom
 
@@ -535,7 +533,6 @@ class Glsa:
 		@rtype:		Boolean
 		@returns:	True if the system is affected, False if not
 		"""
-		vList = []
 		rValue = False
 		for k in self.packages.keys():
 			pkg = self.packages[k]
diff --git a/pym/portage/news.py b/pym/portage/news.py
index d8df8c294..702a377a5 100644
--- a/pym/portage/news.py
+++ b/pym/portage/news.py
@@ -72,7 +72,6 @@ class NewsManager(object):
 
 		path = os.path.join(self.portdb.getRepositoryPath(repoid), self.NEWS_PATH)
 
-		repo_path = self.portdb.getRepositoryPath(repoid)
 		# Skip reading news for repoid if the news dir does not exist.  Requested by
 		# NightMorph :)
 		if not os.path.exists(path):
@@ -83,7 +82,7 @@ class NewsManager(object):
 			try:
 				filename = os.path.join(path, itemid, itemid + "." + self.LANGUAGE_ID + ".txt")
 				item = NewsItem(filename, itemid, timestamp)
-			except (TypeError, ValueError), e:
+			except (TypeError, ValueError):
 				continue
 			if item.isRelevant(profile=self._profile_path,
 				config=self.config, vardb=self.vdb):
diff --git a/pym/portage/output.py b/pym/portage/output.py
index 26a5c4f08..46d4b97e8 100644
--- a/pym/portage/output.py
+++ b/pym/portage/output.py
@@ -4,7 +4,12 @@
 
 __docformat__ = "epytext"
 
-import commands, errno, os, re, shlex, sys, time
+import commands
+import errno
+import os
+import re
+import shlex
+import sys
 from portage.const import COLOR_MAP_FILE
 from portage.util import writemsg
 from portage.exception import PortageException, ParseError, PermissionDenied, FileNotFound
@@ -170,7 +175,6 @@ def parse_color_map(onerror=None):
 	try:
 		s = shlex.shlex(open(myfile))
 		s.wordchars = s.wordchars + ";" # for ansi codes
-		d = {}
 		while True:
 			k, o, v = s.get_token(), s.get_token(), s.get_token()
 			if k is s.eof:
@@ -220,10 +224,11 @@ def parse_color_map(onerror=None):
 
 try:
 	parse_color_map(onerror=lambda e: writemsg("%s\n" % str(e), noiselevel=-1))
-except FileNotFound, e:
+except FileNotFound:
 	pass
 except PortageException, e:
 	writemsg("%s\n" % str(e))
+	del e
 
 def nc_len(mystr):
 	tmp = re.sub(esc_seq + "^m]+m", "", mystr);
diff --git a/pym/portage/process.py b/pym/portage/process.py
index c3f5d4df4..fad38770e 100644
--- a/pym/portage/process.py
+++ b/pym/portage/process.py
@@ -4,8 +4,10 @@
 # $Id$
 
 
-import os, atexit, signal, sys
-import portage.data
+import os
+import atexit
+import signal
+import sys
 
 from portage.util import dump_traceback
 from portage.const import BASH_BINARY, SANDBOX_BINARY
diff --git a/pym/portage/sets/__init__.py b/pym/portage/sets/__init__.py
index 3e1fbdb2e..96c4be8a5 100644
--- a/pym/portage/sets/__init__.py
+++ b/pym/portage/sets/__init__.py
@@ -4,165 +4,10 @@
 
 import os
 
-from portage.const import PRIVATE_PATH, USER_CONFIG_PATH
-from portage.exception import InvalidAtom
-from portage.dep import isvalidatom, match_from_list, best_match_to_list, dep_getkey, use_reduce, paren_reduce
-from portage import flatten
-
 OPERATIONS = ["merge", "unmerge"]
 DEFAULT_SETS = ["world", "system", "everything", "security"] \
 	+["package_"+x for x in ["mask", "unmask", "use", "keywords"]]
-
-class PackageSet(object):
-	# Set this to operations that are supported by your subclass. While 
-	# technically there is no difference between "merge" and "unmerge" regarding
-	# package sets, the latter doesn't make sense for some sets like "system"
-	# or "security" and therefore isn't supported by them.
-	_operations = ["merge"]
-	_atommap = {}
-	description = "generic package set"
-	
-	def __init__(self, name):
-		self.name = name
-		self._atoms = set()
-		self._atommap = {}
-		self._loaded = False
-		self._loading = False
-
-	def __contains__(self, atom):
-		return atom in self.getAtoms()
-	
-	def __iter__(self):
-		for x in self.getAtoms():
-			yield x
-	
-	def supportsOperation(self, op):
-		if not op in OPERATIONS:
-			raise ValueError(op)
-		return op in self._operations
-	
-	def getAtoms(self):
-		if not (self._loaded or self._loading):
-			self._loading = True
-			self.load()
-			self._loaded = True
-			self._loading = False
-		return self._atoms
-
-	def _setAtoms(self, atoms):
-		atoms = map(str.strip, atoms)
-		for a in atoms[:]:
-			if a == "":
-				atoms.remove(a)
-			elif not isvalidatom(a):
-				raise InvalidAtom(a)
-		self._atoms = set(atoms)
-		self._updateAtomMap()
-
-	def load(self):
-		# This method must be overwritten by subclasses
-		# Editable sets should use the value of self._mtime to determine if they
-		# need to reload themselves
-		raise NotImplementedError()
-
-	def containsCPV(self, cpv):
-		for a in self.getAtoms():
-			if match_from_list(a, [cpv]):
-				return True
-		return False
-	
-	def getMetadata(self, key):
-		if hasattr(self, key.lower()):
-			return getattr(self, key.lower())
-		else:
-			return ""
-	
-	def _updateAtomMap(self, atoms=None):
-		"""Update self._atommap for specific atoms or all atoms."""
-		if not atoms:
-			self._atommap.clear()
-			atoms = self._atoms
-		for a in atoms:
-			cp = dep_getkey(a)
-			self._atommap.setdefault(cp, set())
-			self._atommap[cp].add(a)
-	
-	# Not sure if this one should really be in PackageSet
-	def findAtomForPackage(self, cpv, metadata):
-		"""Return the best match for a given package from the arguments, or
-		None if there are no matches.  This matches virtual arguments against
-		the PROVIDE metadata.  This can raise an InvalidDependString exception
-		if an error occurs while parsing PROVIDE."""
-		cpv_slot = "%s:%s" % (cpv, metadata["SLOT"])
-		cp = dep_getkey(cpv)
-		self.getAtoms() # make sure the atoms are loaded
-		atoms = self._atommap.get(cp)
-		if atoms:
-			best_match = best_match_to_list(cpv_slot, atoms)
-			if best_match:
-				return best_match
-		if not metadata["PROVIDE"]:
-			return None
-		provides = flatten(use_reduce(paren_reduce(metadata["PROVIDE"]),
-								uselist=metadata["USE"].split()))
-		for provide in provides:
-			provided_cp = dep_getkey(provide)
-			atoms = self._atommap.get(provided_cp)
-			if atoms:
-				atoms = list(atoms)
-				transformed_atoms = [atom.replace(provided_cp, cp) for atom in atoms]
-				best_match = best_match_to_list(cpv_slot, transformed_atoms)
-				if best_match:
-					return atoms[transformed_atoms.index(best_match)]
-		return None
-
-class EditablePackageSet(PackageSet):
-
-	def update(self, atoms):
-		self.getAtoms()
-		self._atoms.update(atoms)
-		self._updateAtomMap(atoms=atoms)
-		self.write()
-	
-	def add(self, atom):
-		self.update([atom])
-
-	def replace(self, atoms):
-		self._setAtoms(atoms)
-		self.write()
-
-	def remove(self, atom):
-		self.getAtoms()
-		self._atoms.discard(atom)
-		self._updateAtomMap()
-		self.write()
-
-	def removePackageAtoms(self, cp):
-		for a in list(self.getAtoms()):
-			if dep_getkey(a) == cp:
-				self.remove(a)
-		self.write()
-
-	def write(self):
-		# This method must be overwritten in subclasses that should be editable
-		raise NotImplementedError()
-
-
-class InternalPackageSet(EditablePackageSet):
-	def __init__(self, initial_atoms=None):
-		super(InternalPackageSet, self).__init__("")
-		if initial_atoms != None:
-			self.update(initial_atoms)
-
-	def clear(self):
-		self._atoms.clear()
-		self._updateAtomMap()
-	
-	def load(self):
-		pass
-
-	def write(self):
-		pass
+del x
 
 def make_default_sets(configroot, root, profile_paths, settings=None, 
 		vdbapi=None, portdbapi=None):
@@ -170,6 +15,7 @@ def make_default_sets(configroot, root, profile_paths, settings=None,
 	from portage.sets.profiles import PackagesSystemSet
 	from portage.sets.security import NewAffectedSet
 	from portage.sets.dbapi import EverythingSet
+	from portage.const import PRIVATE_PATH, USER_CONFIG_PATH
 	
 	rValue = set()
 	worldset = StaticFileSet("world", os.path.join(root, PRIVATE_PATH, "world"))
@@ -193,6 +39,7 @@ def make_default_sets(configroot, root, profile_paths, settings=None,
 
 def make_extra_static_sets(configroot):
 	from portage.sets.files import StaticFileSet
+	from portage.const import PRIVATE_PATH, USER_CONFIG_PATH
 	
 	rValue = set()
 	mydir = os.path.join(configroot, USER_CONFIG_PATH.lstrip(os.sep), "sets")
@@ -203,7 +50,7 @@ def make_extra_static_sets(configroot):
 	for myname in mysets:
 		if myname in DEFAULT_SETS:
 			continue
-		rValue.add(StaticFileSet(fname, os.path.join(mydir, myname)))
+		rValue.add(StaticFileSet(myname, os.path.join(mydir, myname)))
 	return rValue
 
 def make_category_sets(portdbapi, settings, only_visible=True):
@@ -224,7 +71,7 @@ if __name__ == "__main__":
 		for s in sys.argv[1:]:
 			if s.startswith("category_"):
 				c = s[9:]
-				l.add(CategorySet("category_%s" % c, c, portdbapi, only_visible=only_visible))
+				l.add(CategorySet("category_%s" % c, c, portage.db['/']['porttree'].dbapi, only_visible=False))
 			elif os.path.exists(s):
 				l.add(StaticFileSet(os.path.basename(s), s))
 			elif s != "*":
diff --git a/pym/portage/sets/dbapi.py b/pym/portage/sets/dbapi.py
index 1b2520ef6..13c0fd530 100644
--- a/pym/portage/sets/dbapi.py
+++ b/pym/portage/sets/dbapi.py
@@ -3,8 +3,162 @@
 # $Id$
 
 from portage.versions import catsplit
+from portage import flatten
+from portage.dep import isvalidatom, match_from_list, \
+     best_match_to_list, dep_getkey, use_reduce, paren_reduce
+from portage.exception import InvalidAtom
+from portage.sets import OPERATIONS
 
-from portage.sets import PackageSet
+class PackageSet(object):
+	# Set this to operations that are supported by your subclass. While 
+	# technically there is no difference between "merge" and "unmerge" regarding
+	# package sets, the latter doesn't make sense for some sets like "system"
+	# or "security" and therefore isn't supported by them.
+	_operations = ["merge"]
+	_atommap = {}
+	description = "generic package set"
+	
+	def __init__(self, name):
+		self.name = name
+		self._atoms = set()
+		self._atommap = {}
+		self._loaded = False
+		self._loading = False
+
+	def __contains__(self, atom):
+		return atom in self.getAtoms()
+	
+	def __iter__(self):
+		for x in self.getAtoms():
+			yield x
+	
+	def supportsOperation(self, op):
+		if not op in OPERATIONS:
+			raise ValueError(op)
+		return op in self._operations
+	
+	def getAtoms(self):
+		if not (self._loaded or self._loading):
+			self._loading = True
+			self.load()
+			self._loaded = True
+			self._loading = False
+		return self._atoms
+
+	def _setAtoms(self, atoms):
+		atoms = map(str.strip, atoms)
+		for a in atoms[:]:
+			if a == "":
+				atoms.remove(a)
+			elif not isvalidatom(a):
+				raise InvalidAtom(a)
+		self._atoms = set(atoms)
+		self._updateAtomMap()
+
+	def load(self):
+		# This method must be overwritten by subclasses
+		# Editable sets should use the value of self._mtime to determine if they
+		# need to reload themselves
+		raise NotImplementedError()
+
+	def containsCPV(self, cpv):
+		for a in self.getAtoms():
+			if match_from_list(a, [cpv]):
+				return True
+		return False
+	
+	def getMetadata(self, key):
+		if hasattr(self, key.lower()):
+			return getattr(self, key.lower())
+		else:
+			return ""
+	
+	def _updateAtomMap(self, atoms=None):
+		"""Update self._atommap for specific atoms or all atoms."""
+		if not atoms:
+			self._atommap.clear()
+			atoms = self._atoms
+		for a in atoms:
+			cp = dep_getkey(a)
+			self._atommap.setdefault(cp, set())
+			self._atommap[cp].add(a)
+	
+	# Not sure if this one should really be in PackageSet
+	def findAtomForPackage(self, cpv, metadata):
+		"""Return the best match for a given package from the arguments, or
+		None if there are no matches.  This matches virtual arguments against
+		the PROVIDE metadata.  This can raise an InvalidDependString exception
+		if an error occurs while parsing PROVIDE."""
+		cpv_slot = "%s:%s" % (cpv, metadata["SLOT"])
+		cp = dep_getkey(cpv)
+		self.getAtoms() # make sure the atoms are loaded
+		atoms = self._atommap.get(cp)
+		if atoms:
+			best_match = best_match_to_list(cpv_slot, atoms)
+			if best_match:
+				return best_match
+		if not metadata["PROVIDE"]:
+			return None
+		provides = flatten(use_reduce(paren_reduce(metadata["PROVIDE"]),
+								uselist=metadata["USE"].split()))
+		for provide in provides:
+			provided_cp = dep_getkey(provide)
+			atoms = self._atommap.get(provided_cp)
+			if atoms:
+				atoms = list(atoms)
+				transformed_atoms = [atom.replace(provided_cp, cp) for atom in atoms]
+				best_match = best_match_to_list(cpv_slot, transformed_atoms)
+				if best_match:
+					return atoms[transformed_atoms.index(best_match)]
+		return None
+
+class EditablePackageSet(PackageSet):
+
+	def update(self, atoms):
+		self.getAtoms()
+		self._atoms.update(atoms)
+		self._updateAtomMap(atoms=atoms)
+		self.write()
+	
+	def add(self, atom):
+		self.update([atom])
+
+	def replace(self, atoms):
+		self._setAtoms(atoms)
+		self.write()
+
+	def remove(self, atom):
+		self.getAtoms()
+		self._atoms.discard(atom)
+		self._updateAtomMap()
+		self.write()
+
+	def removePackageAtoms(self, cp):
+		for a in list(self.getAtoms()):
+			if dep_getkey(a) == cp:
+				self.remove(a)
+		self.write()
+
+	def write(self):
+		# This method must be overwritten in subclasses that should be editable
+		raise NotImplementedError()
+
+
+class InternalPackageSet(EditablePackageSet):
+	def __init__(self, initial_atoms=None):
+		super(InternalPackageSet, self).__init__("")
+		if initial_atoms != None:
+			self.update(initial_atoms)
+
+	def clear(self):
+		self._atoms.clear()
+		self._updateAtomMap()
+	
+	def load(self):
+		pass
+
+	def write(self):
+		pass
 
 class EverythingSet(PackageSet):
 	_operations = ["merge", "unmerge"]
diff --git a/pym/portage/sets/files.py b/pym/portage/sets/files.py
index 99d30d830..f5884eea7 100644
--- a/pym/portage/sets/files.py
+++ b/pym/portage/sets/files.py
@@ -2,24 +2,31 @@
 # Distributed under the terms of the GNU General Public License v2
 # $Id$
 
-from portage.util import grabfile, grabfile_package, grabdict_package, write_atomic, ensure_dirs
+import os
+
+from portage.util import grabfile, write_atomic, ensure_dirs
 from portage.const import PRIVATE_PATH
 from portage.locks import lockfile, unlockfile
 from portage import portage_gid
-import os
-
-from portage.sets import PackageSet, EditablePackageSet
+from portage.sets.dbapi import PackageSet, EditablePackageSet
+from portage.env.config import ConfigLoaderKlass, GenericFile
+from portage.env.loaders import ItemFileLoader, KeyListFileLoader
+from portage.env.validators import PackagesFileValidator, ValidAtomValidator
 
 class StaticFileSet(EditablePackageSet):
 	_operations = ["merge", "unmerge"]
 	
-	def __init__(self, name, filename):
+	def __init__(self, name, filename, loader=None):
 		super(StaticFileSet, self).__init__(name)
 		self._filename = filename
 		self._mtime = None
 		self.description = "Package set loaded from file %s" % self._filename
+		if loader is None:
+			self.loader = ConfigLoaderKlass(ItemFileLoader(filename=self._filename,
+				validator=PackagesFileValidator))
 		metadata = grabfile(self._filename + ".metadata")
 		key = None
+		value = []
 		for line in metadata:
 			line = line.strip()
 			if len(line) == 0 and key != None:
@@ -45,17 +52,23 @@ class StaticFileSet(EditablePackageSet):
 		except (OSError, IOError):
 			mtime = None
 		if (not self._loaded or self._mtime != mtime):
-			self._setAtoms(grabfile_package(self._filename, recursive=True))
+			self.loader.load()
+			self._setAtoms(self.loader.keys())
 			self._mtime = mtime
 	
 class ConfigFileSet(PackageSet):
-	def __init__(self, name, filename):
+	def __init__(self, name, filename, loader=None, validator=None):
 		super(ConfigFileSet, self).__init__(name)
 		self._filename = filename
 		self.description = "Package set generated from %s" % self._filename
+		if loader is None:
+			self.loader = GenericFile(filename=self._filename)
+		else:
+			self.loader = loader(filename, validator)
 
 	def load(self):
-		self._setAtoms(grabdict_package(self._filename, recursive=True).keys())
+		self.loader.load()
+		self._setAtoms(self.loader.keys())
 
 class WorldSet(StaticFileSet):
 	description = "Set of packages that were directly installed by the user"
diff --git a/pym/portage/sets/profiles.py b/pym/portage/sets/profiles.py
index 55b7942f5..4e369e5e2 100644
--- a/pym/portage/sets/profiles.py
+++ b/pym/portage/sets/profiles.py
@@ -4,8 +4,7 @@
 
 import os
 from portage.util import grabfile_package, stack_lists
-
-from portage.sets import PackageSet
+from portage.sets.dbapi import PackageSet
 
 class PackagesSystemSet(PackageSet):
 	_operations = ["merge"]
diff --git a/pym/portage/sets/security.py b/pym/portage/sets/security.py
index 3bc9cbca4..d1efa4df7 100644
--- a/pym/portage/sets/security.py
+++ b/pym/portage/sets/security.py
@@ -2,12 +2,11 @@
 # Distributed under the terms of the GNU General Public License v2
 # $Id$
 
+import os
 import portage.glsa as glsa
-from portage.util import grabfile
+from portage.util import grabfile, write_atomic
 from portage.const import CACHE_PATH
-import os
-
-from portage.sets import PackageSet
+from portage.sets.dbapi import PackageSet
 
 class SecuritySet(PackageSet):
 	_operations = ["merge"]
diff --git a/pym/portage/sets/shell.py b/pym/portage/sets/shell.py
index 2383dba81..5c316c1e4 100644
--- a/pym/portage/sets/shell.py
+++ b/pym/portage/sets/shell.py
@@ -2,11 +2,25 @@
 # Distributed under the terms of the GNU General Public License v2
 # $Id$
 
-import subprocess, os
+import subprocess
+import os
 
-from portage.sets import PackageSet
+from portage.sets.dbapi import PackageSet
 
 class CommandOutputSet(PackageSet):
+	"""This class creates a PackageSet from the output of a shell command.
+	   The shell command should produce one atom per line, that is:
+
+	   >>> atom1
+	       atom2
+	       ...
+	       atomN
+
+	   Args:
+	     name: A string that identifies the set.
+	     command: A string or sequence identifying the command to run
+	     (see the subprocess.Popen documentaion for the format)
+	"""
 	_operations = ["merge", "unmerge"]
 
 	def __init__(self, name, command):
diff --git a/pym/portage/tests/__init__.py b/pym/portage/tests/__init__.py
index e3702e08e..2ade6d402 100644
--- a/pym/portage/tests/__init__.py
+++ b/pym/portage/tests/__init__.py
@@ -3,17 +3,19 @@
 # Distributed under the terms of the GNU General Public License v2
 # $Id$
 
-import os, sys, time, unittest
-import portage.tests
+import os
+import sys
+import time
+import unittest
 
 def main():
 	testDirs = ["bin", "dep", "ebuild",
-		"env/config", "util", "versions", "xpak"]
+		"env/config", "util", "versions", "xpak", "sets/shell"]
 	suite = unittest.TestSuite()
 	basedir = os.path.dirname(__file__)
 	for mydir in testDirs:
 		suite.addTests(getTests(os.path.join(basedir, mydir), basedir) )
-	return portage.tests.TextTestRunner(verbosity=2).run(suite)
+	return TextTestRunner(verbosity=2).run(suite)
 
 def my_import(name):
 	mod = __import__(name)
@@ -22,7 +24,7 @@ def my_import(name):
 		mod = getattr(mod, comp)
 	return mod
 
-def getTests( path, base_path ):
+def getTests(path, base_path):
 	"""
 
 	path is the path to a given subdir ( 'portage/' for example)
@@ -30,18 +32,17 @@ def getTests( path, base_path ):
 	to import
 
 	"""
-	import os
-	files = os.listdir( path )
-	files = [ f[:-3] for f in files if f.startswith("test_") and f.endswith(".py") ]
+	files = os.listdir(path)
+	files = [ f[:-3] for f in files if f.startswith("test") and f.endswith(".py") ]
 	parent_path = path[len(base_path)+1:]
-	parent_module = ".".join(("portage","tests", parent_path))
-	parent_module = parent_module.replace('/','.')
+	parent_module = ".".join(("portage", "tests", parent_path))
+	parent_module = parent_module.replace('/', '.')
 	result = []
 	for mymodule in files:
 		# Make the trailing / a . for module importing
 		modname = ".".join((parent_module, mymodule))
 		mod = my_import(modname)
-		result.append( unittest.TestLoader().loadTestsFromModule(mod) )
+		result.append(unittest.TestLoader().loadTestsFromModule(mod))
 	return result
 
 class TextTestResult(unittest._TextTestResult):
@@ -53,18 +54,18 @@ class TextTestResult(unittest._TextTestResult):
 	by the test runner.
 	"""
 	
-	def __init__( self, stream, descriptions, verbosity ):
+	def __init__(self, stream, descriptions, verbosity):
 		unittest._TextTestResult.__init__( self, stream, descriptions, verbosity )
 		self.todoed = []
 
-	def addTodo( self, test, info ):
+	def addTodo(self, test, info):
 		self.todoed.append((test,info))
 		if self.showAll:
 			self.stream.writeln("TODO")
 		elif self.dots:
 			self.stream.write(".")
 	
-	def printErrors( self ):
+	def printErrors(self):
 		if self.dots or self.showAll:
 			self.stream.writeln()
 			self.printErrorList('ERROR', self.errors)
@@ -90,7 +91,7 @@ class TestCase(unittest.TestCase):
 	def defaultTestResult(self):
 		return TextTestResult()
 
-	def run( self, result=None ):
+	def run(self, result=None):
 		if result is None: result = self.defaultTestResult()
 		result.startTest(self)
 		testMethod = getattr(self, self._testMethodName)
@@ -134,7 +135,7 @@ class TextTestRunner(unittest.TextTestRunner):
 	def _makeResult(self):
 	        return TextTestResult(self.stream, self.descriptions, self.verbosity)
 
-	def run( self, test ):
+	def run(self, test):
 		"""
 		Run the given test case or test suite.
 		"""
diff --git a/pym/portage/tests/env/config/test_PackageMaskFile.py b/pym/portage/tests/env/config/test_PackageMaskFile.py
index e8357f7e6..4000f1807 100644
--- a/pym/portage/tests/env/config/test_PackageMaskFile.py
+++ b/pym/portage/tests/env/config/test_PackageMaskFile.py
@@ -6,12 +6,10 @@
 import os
 
 from portage.env.config import PackageMaskFile
-from portage.tests import TestCase
+from portage.tests import TestCase, test_cps
 from tempfile import mkstemp
 
 class PackageMaskFileTestCase(TestCase):
-
-	atoms = ['sys-apps/portage','dev-util/diffball','not@va1id@t0m']
 	
 	def testPackageMaskFile(self):
 		self.BuildFile()
@@ -19,14 +17,14 @@ class PackageMaskFileTestCase(TestCase):
 			f = PackageMaskFile(self.fname)
 			f.load()
 			for atom in f:
-				self.assertTrue(atom in self.atoms)
+				self.assertTrue(atom in test_cps)
 		finally:
 			self.NukeFile()
 	
 	def BuildFile(self):
 		fd, self.fname = mkstemp()
 		f = os.fdopen(fd, 'w')
-		f.write("\n".join(self.atoms))
+		f.write("\n".join(test_cps))
 		f.close()
 	
 	def NukeFile(self):
diff --git a/pym/portage/tests/env/config/test_PortageModulesFile.py b/pym/portage/tests/env/config/test_PortageModulesFile.py
index 03104cff5..e4f9e4252 100644
--- a/pym/portage/tests/env/config/test_PortageModulesFile.py
+++ b/pym/portage/tests/env/config/test_PortageModulesFile.py
@@ -1,34 +1,38 @@
-import os
 
 from portage.tests import TestCase
 from portage.env.config import PortageModulesFile
 from tempfile import mkstemp
 from itertools import izip
+import os
 
 class PortageModulesFileTestCase(TestCase):
 
 	keys = ['foo.bar','baz','bob','extra_key']
-	modules = ['spanky','zmedico','antarus','ricer']
+	invalid_keys = ['',""]
+	modules = ['spanky','zmedico','antarus','ricer','5','6']
 
 	def setUp(self):
 		self.items = {}
-		for k,v in izip(self.keys, self.modules):
+		for k, v in izip(self.keys + self.invalid_keys, 
+				self.modules):
 			self.items[k] = v
 
 	def testPortageModulesFile(self):
 		self.BuildFile()
 		f = PortageModulesFile(self.fname)
-		for k in f:
-			self.assertEqual( f[k], self.items[k] )
+		f.load()
+		for k in self.keys:
+			self.assertEqual(f[k], [self.items[k]])
+		for ik in self.invalid_keys:
+			self.assertEqual(False, ik in f)
 		self.NukeFile()
 
 	def BuildFile(self):
 		fd, self.fname = mkstemp()
 		f = os.fdopen(fd, 'wb')
-		for k,v in self.items.iteritems():
+		for k, v in self.items.iteritems():
 			f.write('%s=%s\n' % (k,v))
 		f.close()
 
 	def NukeFile(self):
-		import os
 		os.unlink(self.fname)
diff --git a/pym/portage/tests/sets/__init__.py b/pym/portage/tests/sets/__init__.py
new file mode 100644
index 000000000..e69de29bb
diff --git a/pym/portage/tests/sets/shell/__init__.py b/pym/portage/tests/sets/shell/__init__.py
new file mode 100644
index 000000000..e69de29bb
diff --git a/pym/portage/tests/sets/shell/testShell.py b/pym/portage/tests/sets/shell/testShell.py
new file mode 100644
index 000000000..938c265e1
--- /dev/null
+++ b/pym/portage/tests/sets/shell/testShell.py
@@ -0,0 +1,28 @@
+# testCommandOututSet.py -- Portage Unit Testing Functionality
+# Copyright 2007 Gentoo Foundation
+# Distributed under the terms of the GNU General Public License v2
+# $Id$
+
+from portage.process import find_binary
+from portage.tests import TestCase, test_cps
+from portage.sets.shell import CommandOutputSet
+
+class CommandOutputSetTestCase(TestCase):
+	"""Simple Test Case for CommandOutputSet"""
+
+	def setUp(self):
+		pass
+
+	def tearDown(self):
+		pass
+
+	def testCommand(self):
+		
+		input = set(test_cps)
+		command = find_binary("echo")
+		command += " -e "
+		for a in input:
+		  command += "\"%s\n\"" % a
+		s = CommandOutputSet('testset', command)
+		atoms = s.getAtoms()
+		self.assertEqual(atoms, input)
diff --git a/pym/portage/util.py b/pym/portage/util.py
index 53b1c86f4..ad6f39e82 100644
--- a/pym/portage/util.py
+++ b/pym/portage/util.py
@@ -2,13 +2,20 @@
 # Distributed under the terms of the GNU General Public License v2
 # $Id: portage_util.py 4603 2006-10-06 03:28:25Z zmedico $
 
+
+import os
+import errno
+import shlex
+import stat
+import string
+import sys
+
 from portage.exception import PortageException, FileNotFound, \
        OperationNotPermitted, PermissionDenied, ReadOnlyFileSystem
 import portage.exception
 from portage.dep import isvalidatom
 from portage.const import EPREFIX
 
-import os, errno, shlex, stat, string, sys
 try:
 	import cPickle
 except ImportError:
@@ -223,6 +230,8 @@ def grabdict(myfilename, juststrings=0, empty=0, recursive=0, incremental=1):
 	return newdict
 
 def grabdict_package(myfilename, juststrings=0, recursive=0):
+	""" Does the same thing as grabdict except it validates keys
+	    with isvalidatom()"""
 	pkgs=grabdict(myfilename, juststrings, empty=1, recursive=recursive)
 	# We need to call keys() here in order to avoid the possibility of
 	# "RuntimeError: dictionary changed size during iteration"
@@ -238,9 +247,7 @@ def grabfile_package(myfilename, compatlevel=0, recursive=0):
 	pkgs=grabfile(myfilename, compatlevel, recursive=recursive)
 	for x in range(len(pkgs)-1, -1, -1):
 		pkg = pkgs[x]
-		if pkg[0] == "-":
-			pkg = pkg[1:]
-		if pkg[0] == "*": # Kill this so we can deal the "packages" file too
+		if pkg[0] in  ["-","*"]: # -atom *pkg for packages file
 			pkg = pkg[1:]
 		if not isvalidatom(pkg):
 			writemsg("--- Invalid atom in %s: %s\n" % (myfilename, pkgs[x]),
diff --git a/pym/portage/xpak.py b/pym/portage/xpak.py
index e3588ab72..c463b8b4c 100644
--- a/pym/portage/xpak.py
+++ b/pym/portage/xpak.py
@@ -126,7 +126,6 @@ def xsplit_mem(mydat):
 	if mydat[-8:]!="XPAKSTOP":
 		return None
 	indexsize=decodeint(mydat[8:12])
-	datasize=decodeint(mydat[12:16])
 	return (mydat[16:indexsize+16], mydat[indexsize+16:-8])
 
 def getindex(infile):
@@ -251,7 +250,7 @@ class tbz2(object):
 		return self.unpackinfo(datadir)
 	def compose(self,datadir,cleanup=0):
 		"""Alias for recompose()."""
-		return recompose(datadir,cleanup)
+		return self.recompose(datadir,cleanup)
 	def recompose(self,datadir,cleanup=0):
 		"""Creates an xpak segment from the datadir provided, truncates the tbz2
 		to the end of regular data if an xpak segment already exists, and adds