[pacman-dev] Some comments on Bryan patches (bugs)
ngaba at bibl.u-szeged.hu
Fri Mar 6 15:24:06 EST 2009
> Nagy Gabor wrote:
> > Hi!
> > I know that I speak too late, but I haven't time/motivation to review
> > this patch nowadays.
> > 1. See alpm/sync.c/sync_prepare(). When user decides to remove
> > unresolvable targets from the list, our code doesn't remove any targets
> > from target->packages, just adds the pulled dependencies. The actual
> > removal is done in the sortbydeps part (sortbydeps is called with list
> > param!). This works, but it is hard to understand (imho), and it is a
> > memleak: some pmsyncpkg_t structs magically disappeared.
> I don't think that it's confusing that it happens this way; the resolve
> operation builds up a new list of packages for the transaction, and that
> list is then used to replace the trans->packages list. This is opposed
> to before where the resolve operation modified trans->packages in place.
> You are right that there is a memory leak there, though; my change
> failed to free up any pmsyncpkg_t structs which were in trans->packages
> but were not in the replacement list. I will submit a small and simple
> patch which fixes this issue shortly.
OK, but please insert a short comment somewhere to make our code more
readable. (The unresolvable targets will be removed later...)
> sync993 does not exist in the pacman sources. If it did, I would have
> seen this problem and addressed it.
That's why I posted it here :) I've just created it to show the problem.
(I like pactests, because we can test our patches easily with them.)
> I will get back to you on this once
> I have figured out what's going on (I suspect that it's a difference in
> the way the new resolve code works, it may stop too short now to figure
> out that there is a 'better' package to choose for resolving the
In the old code, resolvedeps got the whole target list, now it gets only
the first (already resolved) part of that list.
More information about the pacman-dev