At 13:21 Uhr +0000 14.01.2003, Finlay Dobbie wrote:
>On Tuesday, January 14, 2003, at 02:51 am, Max Horn wrote:
>
>>>I just checked in a patch to Fink CVS which reworks the package
>>>indexing code.
>>
>>It might be good to discuss changes like this *before* applying
>>them, not afterwards...
>
>We can always back out the changes if needs be, and I had discussed
>it briefly on #fink. I was under the impression this was something
>that had been thought about in the past, but not implemented through
>lack of time.
Wrong impression. Implementing it would have been easy enough, e.g.
when I last rewrote parts of the index cache code, but I deliberately
chose not do to it this way.
>
>>BTW, making indention changes and a real change at the same time
>>(as in postinstall.pl) is something one should avoid, as it makes
>>it virtually impossible to determine which changes were actually
>>made, at least when one only eyeballs the commit mails (maybe I can
>>make a cvs diff -w or something, gotta try it).
>
>I detailed the change in the ChangeLog, and it was only minuscule
>(issues a fink index at the end).
It's OK, but still what I said above holds true. It was not meant to
criticize you in particular, it's just a rule all of us (including
me) should follow for checkins (I have forgotten this in another
project in the past myself, hence I am extra careful on this matter:
I know how much grief it can cause when you make changes *and*
rearrange/reindent code at the same time: if that particular checkin
causes a regression, it gets very hard to determine which change was
exactly made, esp. if that change is 1-2 week or even months old).
>
>>Mostly the problem you mentioned yourself, i.e. old packages are
>>never removed. The first problem caused by this is that the index
>>would never shrink, only grow.
>
>Well, the index will shrink if it is remade, as it will be every
>time a new version of the package manager is released. Probably not
>an optimal solution, granted, but I mean, it's hardly a massive file
>anyway.
True. Note that I didn't claim it was a massive problem, either. I
just wanted to list the issues I see with the changes, as you
specifically asked us to list the problems we see with the change.
>>Next, it means that if for some reasons we have to revoke a package
>>from CVS, it is not sufficient anymore to just remove it from CVS
>>again - we also need to get people to issue a "fink reindex".
>>That's the main reason I didn't do it that way.
>
>I asked what the policy was on this, and IIRC it was that we bump
>the revision number anyway, to make sure we get rid of it from
>everyone.
That only works if you don't need to revoke a newer version: let's
say you have foo-1.0, and then put up foo-1.1. Later you notice 1.1
is severely broken, so you need to back it out. Upping the revision
of foo-1.0 won't help in this scenario.
A way to fix this problem would be to add proper epoche support.
>
>>On the other hand is the speedup gained by this. But the problem of
>>old packages sticking around was precisely the reason I did not
>>choose to follow this course of action in the past.
>
>As far as I was aware, this was a wanted feature, and the reason it
>was not implemented was because I hadn't actually gotten around to
>looking at it when I implemented the cache anyway. Since I just
>recently found a pocket of free time, I thought I should probably
>implement it. Evidently I was wrong.
I don't say it was necessarily bad to make this change. The point is,
it was bad to just make it w/o discussing it on fink-devel first. And
to talk with the person(s) who are responsible for that part of code,
in the very least. Which in this case is you and me (I rewrote parts
of the code to work differently).
I never mind small bug fixes to obvious problems, or small tweak, or
spelling fixes, being made directly to CVS. But for feature
additions, or changes to fundamental behavior, I like it to be at
least asked about it. Well, in any case, at least you sent an email
to fink-devel after the fact, though beforehand would have been nicer.
That said, from my POV we don't need to discuss this any longer, the
change was made, and we can now discuss whether to revert it, whether
to keep it as it is, or whether it can be augmented to deal with some
of the known problems.
>
>>There is a reliable way to prune old package information, namely if
>>we would record for each PkgInfo from which file it came. Then if
>>those files are missing, one can remove the PkgInfo from the hash.
>>Of course one has to deal with the possibility of multiple files
>>providing the same PkgVersion (e.g. the same package sitting in
>>both stable and unstable). You can detect removed file by the fact
>>that their parent dir has a changed modification time. Alas, doing
>>that might not be faster, or even slower, and of course involves
>>additional code.
>
>I'd need to think about how to deal with dummy packages in this situation.
Dummy packages (macosx and darwin) should never be part of the cache,
I'd think. Or maybe you meant something different?
>
>>Do you cover the fact that when fink (the package manager) is
>>updated, a reindex *has* to be done (after the update)?
>
>Yes, that's what the change in the postinstall change was for.
Very good.
>>Lastly, something neither the current nor the old version did: when
>>the user adds/removes trees from fink.conf, a reindex should be
>>done.
>
>Uhm, the code always used to check the modified date of fink.conf.
Ah OK, I forgot about this then, thanks for clarifying this.
>
>I'm going to go and back out my changes since everybody is up in arms.
Yay, get down man. No need to feel insulted, but "just discussing
this on CVS" and acting on an "impression" easily lets to
misunderstandings. I didn't request you to back this out, but I
wanted to get this straight and clarified.
Max
--
-----------------------------------------------
Max Horn
Software Developer
|
