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

[Andrew Gregory]

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 <morganamilo at gmail.com>
> ---
> 
> 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