[Aptitude-devel] Preview of work refactored from experimental-0.6.9

[Manuel A. Fernandez Montecelo]

2014-02-09 11:27 Daniel Hartwig:
>Hello
>
>I have just pushed two branches, updated for current master.

Thanks for this.


>* dth/remove-libept
>
>Libept is suggested to be removed by its developer.  Aptitude only
>uses it to read debtags database and initiate apt-xapian-index.  Both
>of these tasks are trivial to perform without libept, so this branch
>is moving towards doing that.
>
>Refactored with care from experimental.

Review comments/suggestions:

- the string "/var/lib/apt-xapian-index/index" [1], perhaps should be
   set with a constant?  Or a common place with constant-like, like
   config.h?  Or as these in configure script:
   "--with-package-state-loc" and "--with-lock-loc"?

- in [2], since it's not a reference, returning from function does not
   need to be const, and probably shouldn't be

- perhaps should be handy to check or assert ranges before this (also
   in [2])?  "return tagDB[pkg->ID];"

[1] http://anonscm.debian.org/gitweb/?p=aptitude/aptitude.git;a=commitdiff;h=029963a0f9fce7a541f7b2721b17f8eeb591fd04

[2] http://anonscm.debian.org/gitweb/?p=aptitude/aptitude.git;a=commitdiff;h=e92c39b806520434bcdf509ffbb5072b2f1fb34a
const std::set<tag> aptitude::apt::get_tags(const pkgCache::PkgIterator &pkg)


>The HEAD is not yet
>finalized, and then there are a few more commits to make on top of
>that (multi-arch optimization and reading tags from apt db when
>debtags is not installed).

(Similar questions at bottom, perhaps you can hold your reply to this
part and wait until the end).

It's not very clear to me what's needed in order to integrate this and
to plan releases.

Can we (and would you like to) integrate when "head is finalized", or
only after everything is complete?

I'm OK with anything that you decide, but as a general idea, the
sooner the better.  And also I prefer not to pack too many changes in
new releases.


>* dth/cachefile
>
>This is the most significantly changed part of wip-cmdline.  In the
>older branch there was much duplication of code from libapt for
>constructing CacheSets, due to aptitudes independent cachefile class.
>Here it is derived directly from apts class and hence, less code
>duplication.

Yay \o/


Review comments/suggestions:

- Why the "if (1) { }" in [3]?

- The style in [4] is a bit inconsistent within the file:

   pkgCache::PkgIterator const &Pkg,
   std::string const &pattern)
   const bool was_empty = pci->empty();

   Is this because it was copied from Apt code/interfaces, or some
   other reason?


[3] http://anonscm.debian.org/gitweb/?p=aptitude/aptitude.git;a=commitdiff;h=7d7768d757adf85785538ea250bc33a790555039

[4] http://anonscm.debian.org/gitweb/?p=aptitude/aptitude.git;a=commitdiff;h=1d81bd645c087ea57beb08c9a51dfa5cf802fbc3


>This final commit entangles some unrelated behavioral changes which I
>am pulling out in to separate commits.

Same question as for the other branch: Do you want to integrate the
changes so far in the next releases, before changing the behaviour
with error reporting and the rest?


>* On to 0.7 development
>
>Merging any of this work is sufficient to justify a bump to 0.7.  In
>particular, the tighter error handling and fixups to command line
>handling constitute a noticeable change to the interface.
>
>We can expect final drafts of the complete work by this time next week.

I think that you mean that the work not integrated yet (command line,
errors...) justifies bumping to 0.7.  I agree with that.

Before 0.7 though, I would like to do 0.6.8.5 (and more 0.6.8.x if
needed) with the content of master (and possibly a few more small
fixes), and then (or before) the changes of russian documentation and
multi-arch in the Debian side of the package.  I could have done it
this weekend, but nobody replied and I was busy with BTS instead.

 From my point of view, several or all of the changes that you commited
so far to both branches are OK and do not add any big change,
specially not as seen from the users.  So do you want to include them
already in this release?


Cheers.
--
Manuel