Re: [pacman-dev] [PATCH] libalpm: process needed before group selection

On 09/22/18 at 05:24pm, morganamilo wrote:

When --needed is used, up to date packages are now filtered out before showing the group select.

Signed-off-by: morganamilo ---

This patch set is currently incomplete. There is a problem where if every package in the group is already installed you end up with the eror. "error: target not found: groupname". Instead "there is nothing to do" should be produced instead.

I'm unsure of how to solve this so I am submitting this for discussion. Currently my idea is to have alpm_find_group_pkgs gain a new param `int *exists` which the front end can then check instead of the length of the return. Or instead the needed check could just be moved to the front end. Let me know if theres a better way.

I really hate this function. The logic is way too complex and specific for it to fit comfortably in the backend. I'd really prefer the whole thing move to the frontend. Regardless, I'm not concerned about the somewhat erroneous message. That already happens if the entire group is ignored, so that's really an existing problem that can be solved separately. I'd like to see the patch cleaned up a little, but I think this solution is fine for now. If you resubmit, please include a note in the commit message that this fixes FS#22870.

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index b6ae7b72..f1c02417 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -270,6 +270,8 @@ alpm_list_t SYMEXPORT *alpm_find_group_pkgs(alpm_list_t *dbs, for(i = dbs; i; i = i->next) { alpm_db_t *db = i->data; alpm_group_t *grp = alpm_db_get_group(db, name); + alpm_handle_t *handle = db->handle; + alpm_trans_t *trans = handle->trans;

If you're going to save db->handle to a variable, go ahead and replace the other db->handle references in the function.

if(!grp) { continue; @@ -277,10 +279,26 @@ alpm_list_t SYMEXPORT *alpm_find_group_pkgs(alpm_list_t *dbs,

for(j = grp->packages; j; j = j->next) { alpm_pkg_t *pkg = j->data; + alpm_pkg_t *local = _alpm_db_get_pkgfromcache(handle->db_local, pkg->name);

if(alpm_pkg_find(ignorelist, pkg->name)) { continue; } + if(local) { + const char *localpkgname = local->name; + const char *localpkgver = local->version; + int cmp = _alpm_pkg_compare_versions(pkg, local);

These variables are unnecessary. They are each used once, don't add any descriptive detail, and the two strings have names longer than the struct member they reference.

+ + if(cmp == 0) { + if(trans != NULL && trans->flags & ALPM_TRANS_FLAG_NEEDED) {

Of the three conditions, this will be the fastest to check, so it should be the outermost. The other two can then be combined to avoid unnecessary indentation: if(trans != NULL && trans->flags & ALPM_TRANS_FLAG_NEEDED) { alpm_pkg_t *local = _alpm_db_get_pkgfromcache(handle->db_local, pkg->name); if(local && _alpm_pkg_compare_versions(pkg, local) == 0) {

+ /* with the NEEDED flag, packages up to date are not reinstalled */ + _alpm_log(handle, ALPM_LOG_WARNING, _("%s-%s is up to date -- skipping\n"), + localpkgname, localpkgver); + ignorelist = alpm_list_add(ignorelist, pkg); + continue; + } + } + } if(alpm_pkg_should_ignore(db->handle, pkg)) { alpm_question_install_ignorepkg_t question = { .type = ALPM_QUESTION_INSTALL_IGNOREPKG, -- 2.19.0