Re: Declarative Diversions - Report 3

[Guillem Jover <guillem@debian.org>]

Hi!

[ I mentioned this before, but could you try to send mails only in
  plain text? Not in plain text and html? Thanks! ]

On Tue, 2011-07-05 at 01:36:22 +0100, Sam Dunne wrote:
> The next week will be pushing for the release of preliminary code and after
> that it will be bug testing and working on feedback
> If anyone has any further questions let me know :)

I only skimmed over the spec and the code so I've not done a proper
review of those yet, but there's few things I think are clear enough
regardless. Some of these I mentioned on IRC yesterday but you were
not present, so I'll repeat them here:

  * We should reuse parts of the parser from lib/dpkg/parse.c, I've some
    code now to refactor that, but I realized we don't need to untie
    the code from pkginfo because the declarative diversions are tied
    to a specific package anyway so those will serve a purpose, but
    yes from control file stanza parsing.
  * The new code should live under src/ where all the other files db
    related code resides, moving those to libdpkg right now does not
    make sense anyway.
  * All db related allocations should be done using the non-freeing
    malloc (nfmalloc).
  * The code is reinventing its own diversion structs, which are already
    in src/filesdb.h.

Steve and Sam mentioned that it does not make sense to just reuse the
structs from src/filesdb.h just for reuse sake, because they add more
complexity. My point thought was that:

  * Declarative diversions are more or less “built-in calls” to
    dpkg-divert, so the code needs to update the diversions db to
    add and remove entries there, the current code deals with
    ‘struct diversion’, no point in creating more parsers and loaders
    for the db.
  * The unpack code needs to deal with diversions, which are already
    loaded from the diversions db, declarative diversions need to
    update the db so that diverted files on the same dpkg run work.
    This needs to use the existing infrastructure, not something
    parallel.
  * The current struct is not really that complex, and generating it
    is just few lines of code as can be seen in ensure_diversions(),
    the thing I can see making sense though it adding a new struct to
    be used in the current code as a named constructor argument.
  * This kind of duplicatoin it's generally a symptom something is
    wrong, either in the new code or the old one.

Hope this clarifies.

I've pushed some preliminary refactoring to pu/diversions in my repo,
so that you get an idea of what I mean, the key function here is
parse_stanza():

  <http://git.hadrons.org/?p=debian/dpkg.git;a=shortlog;h=pu/diversions>

The code has only been build tested, and there's few things I don't
quite like about it. I'll be away during the weekend starting today,
but I'll clean that up once I come back.

regards,
guillem