[Aptitude-devel] Aptitude History Tracking progress report, Week 6

[Daniel Burrows]

  Hi, Christian.  It's nice to see that you've been making some real
progress.

On Sat, Jul 04, 2009 at 11:32:46AM -0500, Cristian Porras <porrascristian at gmail.com> was heard to say:
> For a) I redefined the database draft that had been around for some
> time to have a database schema like this:
> 
>             History Entries
>            /                      \
> Dpkg Changes         Aptitude State

  The new design looks good at a first glance.  I have one concern: you
use foreign key constraints, but according to the SQLite documentation,
they aren't actually supported (see http://www.sqlite.org/omitted.html).
There's a suggestion about how to get the same effect with triggers
here:

http://www.sqlite.org/cvstrac/wiki?p=ForeignKeyTriggers

> In b) I added command line support, thus having the ability to view
> history of a package as $ aptitude history [PACKAGE ...].

  I notice that the code throws out attempts to view the history of
packages that don't currently exist.  It would probably be better to
move this test so that it only happens if a package has no history: the
user should be able to view the history of packages that no longer exist
in the archive.  And of course turn the error message into something
like "package %s does not exist and has no recorded history".

  I tried running an upgrade with this version of aptitude and then
checking the history of one of the upgraded packages.  "aptitude
history" crashed with a segmentation fault.  I tried running it as root,
just to see whether maybe there was a permissions problem, and it
crashed with a glibc invalid free() error.  When I run the GUI, it
crashes on start-up.  Also, the text history file that aptitude
generated is empty.

  I also got a crash when there was no history database at all, but I
don't know if that is the same problem or something separate.

  I haven't debugged further yet -- are you aware of these problems, or
does it work for you?

> For the GUI, I have based on the code made to view the files and
> folders (filesview), and added a subtab in the packet tab.

  Haven't looked at the code or run this part of the program yet, I'll
write more once I have.  (as noted above, I can't run it at all)

  A few points I noticed while reading the code:

    (1) You use a static variable for storing the history entries --
        I think this should be a non-static member variable.
    (2) I think that do_history_from_list should not clear out the
        history list; it should be a const parameter, and theh caller
        should be responsible for clearing it.  The current convention
        seems to me like it violates the principle of least surprise.
    (3) When writing out the entry list, the code in aptcache.cc updates
        all the entries according to the current package state and
        overwrites the Version field with the first version in the list
        of versions.  This looks to me like it's more or less
        guaranteed to do the wrong thing, but I can't test it because
        of the bugs I noted above.
    (4) I want to plead one more time for some semblance of local
        consistency in indentation.  Stuff like this is jarring and hard
        to read:

	      if(do_dselect && pkg->SelectedState != last_dselect_state)
		{
      PkgIterator pkg_back=pkg;
      aptitude_state state_back=get_ext_state(pkg);

    (5) Once you have everything working, I assume you'll convert the
        text log to a more compact and readable format?  The format I
        specced out in the history_entry header file would be a good
        place to start (but feel free to get stuff working before you
        change formats).

  Daniel