[Aptitude-devel] Review: 1ec363ac (001.1-package^)

[Daniel Burrows]

On Tue, Jul 20, 2010 at 08:10:38PM +0200, Piotr Galiszewski <piotr at galiszewski.pl> was heard to say:
> Ah. I see (looking at pkg_item). I mixed logic of candidate version
> and visible version. It looks like I should rename candidate_version
> to visible_version and use it as previous. Furthermore, pkt_item's
> visible_version uses the the same code as my candidate_version. I have
> only one more questions. If I read it correctly this visible_version
> returns NULL in the same situations as my candidate_version.
>
> In the second version of my patch (the first version without Qt
> dependencies), I used proper logic for is_virtual and
> candidate_version (should have been visible_version). I have changed
> that code after first real testing of this code. My code crashes
> frequently when is_virtual  was false and candidate_version returned
> null. I based that code on pkg_tree and pkg_item, but probably I
> missed some important checks

  You still have get_candidate_version, but it appears to return
something other than the candidate version?  It should just read:

    // ...
    if(!candver.end())
      candidate_ver = version::create(candver);
    else
      candidate_ver = version_ptr();
    // ...

  Also, be aware that there may be some issues with caching the
candidate version due to the way that version selection is handled
(i.e., the candidate version of a package is *not* immutable).  I
don't think you need to handle those in this iteration, though.

> Should visible_version returns something in every situation
> pkg.VersionList().end() returns true?

  If by "true" you mean "false", then I think that seems correct.

  Daniel