On Sat, 12 Dec 2015 12:44:40 -0800 Zac Medico wrote: > On 12/12/2015 06:50 AM, Michał Górny wrote: > > On Sat, 12 Dec 2015 13:01:04 +0100 > > Alexander Berntsen wrote: > > > >> Friends, > >> > >> As I'm sure you've noticed, Arfrever pushed a bunch of unreviewed > >> stuff. Michał reverted some of them as they were breaking changes in > >> addition to being unreviewed (oh, and of course they don't follow the > >> commit message format -- why would they). There's a bunch left. > >> > >> Do we want to cherry-pick Zac's patches (i.e. revert Arfrever's > >> patches), and have him put them up for review as usual? It's a bit of > >> a mess because of Zac's patches. If we go down this route it would > >> likely be easiest for Zac to revert them, since he'll be the more > >> confident regarding what to keep in the ensuing merge conflicts. > > Yeah, I'd be happy to do that. > > >> Arfrever asked that someone review the patches and keep them if they > >> are OK, and revert only the ones that are problematic. I think this > >> would be OK for now, but obviously not something we want to encourage > >> or allow in the future. > > > > Here's my quick review of the remaining commits (newest first): > > Thanks! > > > 31923f4 portage.package.ebuild.config.config.__init__(): Skip some warnings for Portage Python scripts called in ebuild environment. > > 7853950 Delete support for PORTDIR and PORTDIR_OVERLAY from make.conf and environment. > > Before dropping PORTDIR and PORTDIR_OVERLAY, we need deprecation > warnings for a couple of years in advance. > > > e7d95cb portage.repository.config.RepoConfig: Support location with trailing whitespace by using quoting. > > This one might be okay. Did it really break anything? I don't know. I reverted the whole bunch of unreviewed stuff then, better safe than sorry. Furthermore, it looks suspicious, so I'd rather have that reviewed properly rather than post-factum. > > fb4d1f4 ebuild: Rename some variables. > > > > I'd keep it, doesn't hurt anything and the new names are more readable than 'myrepo' ;-). > > Looks good. I had to revert it because it relies on the previous commit. I'd rather have it reintroduced cleanly rather than having fixes mixed with reverts, or partially broken states. > > 9e104c4 ebuild: Set PORTAGE_REPOSITORIES instead of deprecated PORTDIR_OVERLAY. > > > > Kill it as relying on weirdo PORTAGE_REPOSITORIES invention. > > I think this one is mostly okay, since PORTAGE_REPOSITORIES has been > supported for some time, and it would be nice to eliminate internal > usage for PORTDIR_OVERLAY. > > However, this patch relies on the _read_repo_name method added in > 2cde1f6. When we revert 2cde1f6, we can also change this code to use the > RepoConfig._read_valid_repo_name staticmethod that was removed in 2cde1f6. I have reverted it for this reason. As above, we can reintroduce it when it's fixed to work with the resulting code. > > 2cde1f6 portage.repository.config: Clean reading of repository names and drop support for repositories with missing or invalid names. > > > > I'd revert it, it's a breaking change. > > I think this is too much at once, so we should revert it. > > As a first step in the direction that this patch is going, we could > remove the special case for /usr/local/portage from the repo_name_check > functions, so that people will start getting warnings for a missing > repo_name there. Reverted. We can proceed from here. > > 10cccf7 Support environmental variables overriding parts of configuration of repositories. > > > > Kill this ugly thing with a lot of fire. This is so wrong you can't get > > much worse. > > I don't like this mainly because variables of the form > PORTAGE_REPOSITORY:${repository_name}:${attribute} cannot be exported in > bash: > > $ export foo:bar=baz > bash: export: `foo:bar=baz': not a valid identifier Reverted. > > 39d81c5 portage.package.ebuild.config.config.__getitem__(): Partially drop backward compatibility for nonexistent keys. > > > > Maybe keep it but needs someone smarter than me to review. > > Looks good, except the last hunk seems redundant because "for x in self" > should only yield valid keys: > > @@ -2697,7 +2714,9 @@ class config(object): > for x in self: > if x in environ_filter: > continue > - myvalue = self[x] > + myvalue = self.get(x) > + if myvalue is None: > + continue Could you fix it then, please? I've left all the other unmentioned commits. -- Best regards, Michał Górny