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

[Brian Dolbec]

On Sun, 16 Nov 2014 17:29:26 -0800
Zac Medico <zmedico@g.o> wrote:

> This adds dblink._contents_contains, _contents_keys, and
> _contents_map_key methods that provide an interface for contents
> operations with "implicit" case handling. The new methods are
> implemented in a separate ContentsCaseSensitivityManager class,
> in order to avoid adding more bloat to vartree.py.
>
> X-Gentoo-Bug: 524236
> X-Gentoo-Url: https://bugs.gentoo.org/show_bug.cgi?id=524236
> ---
>  .../dbapi/_ContentsCaseSensitivityManager.py       | 54
> ++++++++++++++++++++++
> pym/portage/dbapi/vartree.py                       |  8 ++++ 2 files
> changed, 62 insertions(+) create mode 100644
> pym/portage/dbapi/_ContentsCaseSensitivityManager.py
>
> diff --git a/pym/portage/dbapi/_ContentsCaseSensitivityManager.py
> b/pym/portage/dbapi/_ContentsCaseSensitivityManager.py new file mode
> 100644 index 0000000..6b8301d
> --- /dev/null
> +++ b/pym/portage/dbapi/_ContentsCaseSensitivityManager.py
> @@ -0,0 +1,54 @@
> +# Copyright 2014 Gentoo Foundation
> +# Distributed under the terms of the GNU General Public License v2
> +
> +class ContentsCaseSensitivityManager(object):
> +
> +	def __init__(self, db):
> +
> +		self.getcontents = db.getcontents
> +
> +		if "case-insensitive-fs" in db.settings.features:
> +			self.unmap_key =
> self._unmap_key_case_insensitive
> +			self.contains =
> self._contains_case_insensitive
> +			self.keys = self._keys_case_insensitive
> +		else:
> +			self.unmap_key =
> self._unmap_key_case_sensitive
> +			self.contains = self._contains_case_sensitive
> +			self.keys = self._keys_case_sensitive
> +
> +		self._contents_insensitive = None
> +		self._reverse_key_map = None
> +
> +	def clear_cache(self):
> +		self._contents_insensitive = None
> +		self._reverse_key_map = None
> +
> +	def _case_insensitive_init(self):
> +		self._contents_insensitive = dict(
> +			(k.lower(), v) for k, v in
> self.getcontents().items())
> +		self._reverse_key_map = dict(
> +			(k.lower(), k) for k in self.getcontents())
> +
> +	def _keys_case_insensitive(self):
> +		if self._contents_insensitive is None:
> +			self._case_insensitive_init()
> +		return iter(self._contents_insensitive)
> +
> +	def _contains_case_insensitive(self, key):
> +		if self._contents_insensitive is None:
> +			self._case_insensitive_init()
> +		return key.lower() in self._contents_insensitive
> +
> +	def _unmap_key_case_insensitive(self, key):
> +		if self._reverse_key_map is None:
> +			self._case_insensitive_init()
> +		return self._reverse_key_map[key]
> +
> +	def _keys_case_sensitive(self):
> +		return iter(self.getcontents())
> +
> +	def _contains_case_sensitive(self, key):
> +		return key in self.getcontents()
> +
> +	def _unmap_key_case_sensitive(self, key):
> +		return key


Very nice, much better this way than adding to the dblink class
directly.

Just one thing left to do...  Docstrings please.  While normally you
don't NEED them for private functions, in this case they are exported
using the function pointer assignments.  They are named quite well to
indicate their function, but document them anyway please.




> diff --git a/pym/portage/dbapi/vartree.py
> b/pym/portage/dbapi/vartree.py index 8b06f4c..22f41d0 100644
> --- a/pym/portage/dbapi/vartree.py
> +++ b/pym/portage/dbapi/vartree.py
> @@ -69,6 +69,7 @@ from _emerge.EbuildPhase import EbuildPhase
>  from _emerge.emergelog import emergelog
>  from _emerge.MiscFunctionsProcess import MiscFunctionsProcess
>  from _emerge.SpawnProcess import SpawnProcess
> +from ._ContentsCaseSensitivityManager import
> ContentsCaseSensitivityManager
>  import errno
>  import fnmatch
> @@ -1526,6 +1527,12 @@ class dblink(object):
>  		# compliance with RESTRICT=preserve-libs.
>  		self._preserve_libs = "preserve-libs" in
> mysettings.features
> +		manager = ContentsCaseSensitivityManager(self)
> +		self._contents_case_sensitivity_manager = manager
> +		self._contents_unmap_key = manager.unmap_key
> +		self._contents_contains = manager.contains
> +		self._contents_keys = manager.keys
> +

Rather than so many function pointer assignments, why not

 +		self._contents = ContentsCaseSensitivityManager(self)

Then later use it like so...

 +		self._contents.unmap_key(...)
 +		self._contents.contains(...)
 +		self._contents.keys()
 +

vardbapi calls will just get a similar one additional level of
direction.

<snipped from patch V2 2/2>
-								for p in dblink(cpv).getcontents():
+								for p in dblink(cpv)._contents_keys():

to:
-								for p in dblink(cpv).getcontents():
+								for p in dblink(cpv)._contents.keys():

It should be less confusing for future maintenance by having to track all the 
pointer assignments to know what it is actually pointing at.  It is a lot easier to find that _contents is a 
ContentsCaseSensitivityManager class instance.  Then all function calls directly refer to those functions 
or the sensitivity set exported versions of them.


>  	def __hash__(self):
>  		return hash(self._hash_key)
>
> @@ -1612,6 +1619,7 @@ class dblink(object):
>  		self.contentscache = None
>  		self._contents_inodes = None
>  		self._contents_basenames = None
> +		self._contents_case_sensitivity_manager.clear_cache()

again...

+		self._contents.clear_cache()

>
>  	def getcontents(self):
>  		"""



-- 
Brian Dolbec <dolsen>