[pacman-dev] Some comments on Bryan patches (bugs)

[Nagy Gabor]

> 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 
> dependency).

In the old code, resolvedeps got the whole target list, now it gets only
the first (already resolved) part of that list.

Bye