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

[Brian Dolbec]

On Thu, 2014-01-02 at 22:20 -0800, W. Trevor King wrote:
> 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.
>

We probably could, they are defaulted in config_defaults.  But at the
time I was just thinking that catalyst.conf or a spec file may not have
the value declared, so was better to give it a default here, just in
case.


> > -		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
>

I didn't do that because I didn't know if the mount order was important,
so erred on the side of the current order.