On Mon, Jan 23, 2006 at 07:44:44PM +0100, Marius Mauch wrote: > On Mon, 23 Jan 2006 03:47:11 -0800 > Brian Harring <[EMAIL PROTECTED]> wrote: > > On Mon, Jan 23, 2006 at 11:16:03AM +0100, Marius Mauch wrote: > > > On Wed, 11 Jan 2006 12:39:03 -0800 > > > Brian Harring <[EMAIL PROTECTED]> wrote: > > > > > > > Regex you've got there allows for pulling the wrong text- recall, > > > > ebd originally was doing grep based filtering (regex). Had to > > > > rewrite that in a major hurry since bash syntax (specifically > > > > here ops) forces you to track state/constructs rather then just a > > > > regex... > > > > > > Not really an issue in this case. First the code bails out if more > > > than one match is found, so unless the metadata assignment is NOT > > > found by it we don't get the wrong info. > > > Also a mismatch in this special is so > > > extremely unlikely that honestly I don't really care about it, > > > especially as this is a one time conversion (might be different if > > > I'd have added the on the fly extraction). > > > > Re-read that statement. It's a one time conversion- meaning we > > better get it right the first time, else the user's data is > > effectively corrupted. Forcing a full regen from the saved > > environment is not a solution for fixing past corruptions either. > > > > If it were on the fly extraction, I wouldn't care quite as much- but > > the fact this is an untracked change to the users data means we *do* > > need to cover corner cases. > > Can't follow your thinking here. As said, the code won't corrupt any > data, at worst it will tell the user that it couldn't extract some keys > (and even that only if there would be such a stupid case which itself > has a chance of like 10^-10 or so, or can you name a single case?).
Yeah, any extension of your code to pick up EAPI is going to false
positive on somewhere around a quarter of my vdb nodes- I use a
debug function from my bashrc for prefix testing (holdover from eapi
testing days). Why? Because the eapi var didn't exist (thus no env
assignment), the only possible match is in the middle of a here op-
debug_dump() {
...
echo <<<HERE >&2
EAPI="${EAPI-unset}"
PF="$PF"
CATEGORY="$CATEGORY"
HERE
...
}
Setting EAPI to unset strikes me as corruption here, because now
portage will *never* do anything with that vdb node- the eapi differs
from what it supports (it's been set to effectively a random value),
thus I have to fix the vdb entry myself if I ever want to get rid of
it.
Guessing the response on that one is "well, you're using the bashrc",
and yes, I am- the vdb code should be *robust*, not choking on a users
debug code.
> So what package contains filter-env then?
Portage- been in cvs/svn for over a year now :)
> > Aside from that, if the code is in debate (as this is), I really
> > don't think it should get slid into svn 2 weeks later effectively
> > unchanged- didn't write that original email just for the hell of it :)
>
> As said, I disagree with your assessment of the situation. If you can
> name a single case where the code "breaks" or filter-env hits the tree
> I might reconsider, but not before.
Well, if you disagreed with the original response, continue the
conversation prior to commiting- otherwise we see a commit, then a
rebuttal a few hours later. Not really how things should go for a
contested piece of code (at least when the only two to weigh in our
flat out opposed on it)- especially if the code's effect is nontrivial
and it hasn't had any actual peer review (only comment was on your algo).
Issues with the code.
1) Scans for metadata that didn't exist at the time of the ebuild's
installation, only possible match is unintended. Not good.
2) your code is breaking upon *all* newlines, regardless if it's
in the middle of a variable assignment. Yes, bash does use $'' for
assignment, but you're relying (hoping really) that the variable has
been filtered by python portage and the ebuild defined var is stomped.
This is a bad idea- description comes to mind. Your code seems to be
aware of this possibility also, since it's doing rather loose quote
filtering.
3) Stop using regex all over the place. split/join is a helluva lot
faster, and collapses that nasty regex that attempts to remove
whitespace down to two lines (a one time translate, and the split/join
to kill whitespace).
4) Code specifically tries to find, and remove variable chars-
this is *really* the wrong thing to do. If you're trying to work
around catching a var reference, either your parsing screwed up, or
you're mangling a DESCRIPTION value that you shouldn't be hosing.
5) hardcoded vdb.
6) Non root aware VDB.
7) Only is aware of environment.bz2- older portage versions differed
in this afaik (I *do* know PORT_ENV_FILE overloading for ebd I had to
address this case already, so it's out there).
8) perms on the new files?
9) general code comments; 'if s != ""', use 'if s:'. Catch the
exceptions for check mode, don't let vdbkeyhandler nuke world file
checking due to a potential exception. Right now, your code will TB
if ran by non-root for a check run rather then stating "got to be
root".
In general, in looking through the parsing here there is a *lot* of
chunks that look questionable- the scan for a var assignment for
example, should *not* be stripping quotes on it's own. The only
benefit of that stripping is allowing for false positives (hardly a
benefit).
Aside from code commentary, the real brass tacks here is that this
code if used is going to be expanded for more then the trivial vars it
handles now- in other words, this problematic parser will wind up
being used months down the line for important vars with intrinsic issues
still unaddressed.
And yes, what I'm pointing at is a corner case- that doesn't mean
we cut corners on the solution *especially* when a proper tool
exists for this already. No tool? I'd be a bit quieter, but the
issues *are* resolved already, you just need to ditch the adhoc regex
attempt (or write your own state aware parser). It's a 20 minutes mod
to fix my initial complaints on this- it might seem pointless, but the
potential is there as is a fix, so address it.
~harring
pgpdU9abIfMhw.pgp
Description: PGP signature
