Re: [gentoo-portage-dev] [PATCH 1/2] dblink: case insensitive support for bug #524236

[Brian Dolbec <dolsen@gentoo.org>]

On Sun, 16 Nov 2014 02:41:54 -0800
Zac Medico <zmedico@gentoo.org> wrote:

> This adds dblink._contents_contains, _contents_iter, and
> _contents_key methods that provide an interface for contents
> operations with "implicit" case handling.
> 
> X-Gentoo-Bug: 524236
> X-Gentoo-Url: https://bugs.gentoo.org/show_bug.cgi?id=524236
> ---
>  pym/portage/dbapi/vartree.py | 43
> +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43
> insertions(+)
> 
> diff --git a/pym/portage/dbapi/vartree.py
> b/pym/portage/dbapi/vartree.py index 8b06f4c..2d1003f 100644
> --- a/pym/portage/dbapi/vartree.py
> +++ b/pym/portage/dbapi/vartree.py
> @@ -1515,6 +1515,8 @@ class dblink(object):
>  		self.contentscache = None
>  		self._contents_inodes = None
>  		self._contents_basenames = None
> +		self._contents_case_insensitive = None
> +		self._contents_case_reverse_map = None

I like that you've prefixed the new variables with _contents_.

I also see there already are a number of _contents_ prefixed variables
and now newly added functions below.



>  		self._linkmap_broken = False
>  		self._device_path_map = {}
>  		self._hardlink_merge_map = {}
> @@ -1526,6 +1528,15 @@ class dblink(object):
>  		# compliance with RESTRICT=preserve-libs.
>  		self._preserve_libs = "preserve-libs" in
> mysettings.features 
> +		if "case-insensitive-fs" in self.settings.features:
> +			self._contents_key =
> self._contents_key_case_insensitive
> +			self._contents_contains =
> self._contents_contains_case_insensitive
> +			self._contents_iter =
> self._contents_iter_case_insensitive
> +		else:
> +			self._contents_key =
> self._contents_key_case_sensitive
> +			self._contents_contains =
> self._contents_contains_case_sensitive
> +			self._contents_iter =
> self._contents_iter_case_sensitive +
>  	def __hash__(self):
>  		return hash(self._hash_key)
>  
> @@ -1612,6 +1623,8 @@ class dblink(object):
>  		self.contentscache = None
>  		self._contents_inodes = None
>  		self._contents_basenames = None
> +		self._contents_case_insensitive = None
> +		self._contents_case_reverse_map = None
>  
>  	def getcontents(self):
>  		"""
> @@ -1716,6 +1729,36 @@ class dblink(object):
>  		self.contentscache = pkgfiles
>  		return pkgfiles
>  
> +	def _contents_case_insensitive_init(self):
> +		self._contents_case_insensitive = dict(
> +			(k.lower(), v) for k, v in
> self.getcontents().items())
> +		self._contents_case_reverse_map = dict(
> +			(k.lower(), k) for k in self.getcontents())
> +
> +	def _contents_iter_case_insensitive(self):
> +		if self._contents_case_insensitive is None:
> +			self._contents_case_insensitive_init()
> +		return iter(self._contents_case_insensitive)
> +
> +	def _contents_key_case_insensitive(self, key):
> +		if self._contents_case_reverse_map is None:
> +			self._contents_case_insensitive_init()
> +		return self._contents_case_reverse_map[key]
> +
> +	def _contents_contains_case_insensitive(self, key):
> +		if self._contents_case_insensitive is None:
> +			self._contents_case_insensitive_init()
> +		return key.lower() in self._contents_case_insensitive
> +
> +	def _contents_key_case_sensitive(self, key):
> +		return key
> +
> +	def _contents_iter_case_sensitive(self):
> +		return iter(self.getcontents())
> +
> +	def _contents_contains_case_sensitive(self, key):
> +		return key in self.getcontents()
> +
>  	def _prune_plib_registry(self, unmerge=False,
>  		needed=None, preserve_paths=None):
>  		# remove preserved libraries that don't have any
> consumers left


These truly have little interaction with the rest of the dblink class.
I would like to see them moved to an independent class.  With that move
the names could be shortened without the _contents prefix since they
would be Contents class referenced.  It also looks like there should be
additional code from the dblink class moved into this new class.
Primarily the getcontents(), but that is a much larger refactor.  That
would also require the cache, re's and probably some others.  Then all
the references to these functions in the vardbapi to the class
instance instead.

On that note, I propose we put this new code in a new class (avoiding
adding to the bloat), pass in the getcontents() pointer along with some
others needed. In later commits do the major re-factor moving more
contents specific handling code into the new class.  Bare in mind I
have not looked that extensively in dblink and vardbapi, but it does
look doable. 

Are you up for the challenge? ;)  
I think it would certainly make the code more maintainable, less
daunting and more clearly defined.  We could even split vartree.py into
some smaller files or sub-pkg.  I dislike multi-thousand lines of code
files and classes.  It makes it much more difficult to move around and
keep track of interactions between the code (complexity) and to learn
everything it does.

-- 
Brian Dolbec <dolsen>