Re: [gentoo-catalyst] [PATCH 1/2] modules/generic_stage_target.py, Create SOURCE_MOUNTS_DEFAULTS

[W. Trevor King]

I shift quoted lines around for easier comparison.

On Thu, Jan 02, 2014 at 10:00:43PM -0800, Brian Dolbec wrote:
> +SOURCE_MOUNTS_DEFAULTS = {
> …
> +	"distdir": "/usr/portage/distfiles",
> +	"portdir": "/usr/portage",
> …
> -				"distdir": self.settings["distdir"],
> -				"portdir": normpath("/".join([
> -					self.settings["snapshot_cache_path"],
> -					self.settings["repo_name"],
> -					])),
> …
> +		# initialize our source mounts
> +		self.mountmap = SOURCE_MOUNTS_DEFAULTS.copy()
> +		# update them from settings
> +		self.mountmap["distdir"] = self.settings["distdir"]
> +		self.mountmap["portdir"] = normpath("/".join([
> +			self.settings["snapshot_cache_path"],
> +			self.settings["repo_name"],
> +			]))

Why create dummy initial values and then blow them away?  Wouldn't:

  SOURCE_MOUNTS_DEFAULTS = {
    …
    'distdir': None,  # initialized from settings
    'portdir': None,  # initialized from settings
    }

make more sense?  We'll blow away this default dict once we're loading
it via ConfigParser, so this doesn't have to be super elegant, but
adding values just to clobber them seems misleading.

> -		if "SNAPCACHE" in self.settings:
> -			self.mounts = ["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> …
> -		else:
> -			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
> …
> +		self.mounts = ["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> …
> +		if "SNAPCACHE" not in self.settings:
> +			self.mounts.remove("portdir")

I'd prefer:

  self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
  if "SNAPCACHE" in self.settings:
      self.mounts.append("portdir")

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy