> > The original version of this code simply ignored any catalog entry that > > we didn't understand. When we downloaded one, we ignored it, and > > omitted it from the catalog. If one happened to sneak into a catalog, > > we just ignored it too. > > Well, kindof, but not really. :)
Sorry, I mis-typed. I should have said my first draft of this bugfix, not the original code. > I'm suggesting that you needn't do all the set arithmetic, and that you > just write known_prefixes into the attribute directly, since it ends up > doing exactly the same work with less and more readable code. Except that we don't want to remove prefixes in the case I gave below running old/new/old clients. I can try to code this up without sets, if you'd like. > > > - wouldn't we want to check if any of the prefixes in the catalog aren't > > > recognized (i.e., "if pfx_set - known_prefixes:")? > > > > This is a good question. If any prefixes in pfx_set aren't in > > known_prefixes, we'll just skip them as most of the code only checks the > > known_prefixes list. If something is in pfx_set that isn't in > > known_prefixes, it's likely that someone ran an old version of the > > client, used a new version, and then used the old version again. I'm > > assuming that in this situation we want the older client to be able to > > get work done. > > Okay. > > > - line 546ff - so I would have just done this first test after the > > > split() on line 551 -- "if cv not in known_prefixes:". But the check > > > should actually be more specific here, because this bit of the code > > > can > > > only handle "C" and "V" types. In fact, adding just that check was > > > pretty much the intent of this bug. > > > > I don't think I understand. In what way should this check be more > > specific? > > On line 551, you split() the entry into a 4-tuple. You're adding the check > that the second element is "pkg", which is good, but you should also be > checking that the first element is either "C" or "V". If it's, say, "Z" > (and "Z" is in known_prefixes), we carry on, and potentially screw up. > > For that matter, assuming a 4-tuple is probably wrong, too. Assume a > 2-tuple, instead, limiting the split to two, which will give you either a > 2- or 3-tuple. The third element of that tuple, if it exists, can be split > further later on. I'm a little lost, since I would have assumed that we would have written code to modify what we're looking for once prefix Z was introduced. I'm not adding new prefixes yet, so it seemed a bit overkill to modify this past explicitly ignoring unknown prefixes. I assumed that since the only prefixes that we know about in this case are C and V and we'll ignore all others, it's still sane to keep this code. Do you disagree? > > > - Similar comment for the hunk at line 600, though here you don't do a > > > check inside the try clause, but you do in the except, which is > > > confusing. > > > > I took a look at this code, and I'm confused by this comment. The check > > against known_prefixes happens outside of the try both here and at line > > 546. The check if pkg == "pkg" is in the try in both cases. > > Sorry I was confusing. The check you make on 614 (in the except) is not > duplicated in the try block. And it's that check which should also be in > the previous similar code, back in the 550s, in both the try and the except > blocks in that case, too. Ah, okay. I can add these checks. Thanks for the clarification. > And I think the isspace or in known_prefixes test isn't useful with > this tighter test. I saw the isspace check as a way to catch garbage lines, and the known_prefixes check as a way to catch unsupported entries. I think that these checks are complimentary since get_matching_fmris probably only wants to look at package frmis in the catalog. Finding an unknown prefix is somewhat different than finding a known prefix of a type that's not applicable to the behavior of get_matching_fmris. > I'm worried that a new catalog type might mean that a client will have to > (or be strongly encouraged to) do something special (like upgrade the > client), but if they're never told, they're never gonna know. The question > is how urgent such action -- and thus such notification -- is, or might be. > It's an open question, I think, so an XXX comment seems appropriate. Oh, okay. I think I misunderstood you. I'm happy to add this. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
