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.
signature.asc
Description: This is a digitally signed message part
