Re: Declarative Diversions - Report 3
[ 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
* The code is reinventing its own diversion structs, which are already
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
* 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
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.