[pacman-dev] during -Qu add [ignored] for repos without Usage = Upgrade
diff mbox

Message ID 20180831233556.5343-1-morganamilo@gmail.com
State New
Headers show

Commit Message

Morgan Adamiec Aug. 31, 2018, 11:35 p.m. UTC
Fixes FS#59854

Signed-off-by: morganamilo <morganamilo@gmail.com>
---
 src/pacman/query.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Allan McRae Sept. 19, 2018, 8:17 a.m. UTC | #1
On 1/9/18 9:35 am, morganamilo wrote:
> Fixes FS#59854
> 
> Signed-off-by: morganamilo <morganamilo@gmail.com>
> ---

Looks good.

A

>  src/pacman/query.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/pacman/query.c b/src/pacman/query.c
> index 00c39638..faa06e60 100644
> --- a/src/pacman/query.c
> +++ b/src/pacman/query.c
> @@ -325,10 +325,14 @@ static int display(alpm_pkg_t *pkg)
>  					colstr->version, alpm_pkg_get_version(pkg), colstr->nocolor);
>  
>  			if(config->op_q_upgrade) {
> +				int usage;
>  				alpm_pkg_t *newpkg = alpm_sync_newversion(pkg, alpm_get_syncdbs(config->handle));
> +				alpm_db_t *db = alpm_pkg_get_db(newpkg);
> +				alpm_db_get_usage(db, &usage);
> +
>  				printf(" -> %s%s%s", colstr->version, alpm_pkg_get_version(newpkg), colstr->nocolor);
>  
> -				if(alpm_pkg_should_ignore(config->handle, pkg)) {
> +				if(alpm_pkg_should_ignore(config->handle, pkg) || !(usage & ALPM_DB_USAGE_UPGRADE)) {
>  					printf(" %s", _("[ignored]"));
>  				}
>  			}
>
Allan McRae Sept. 19, 2018, 9 a.m. UTC | #2
On 1/9/18 9:35 am, morganamilo wrote:
> Fixes FS#59854
> 
> Signed-off-by: morganamilo <morganamilo@gmail.com>
> ---

You patch is fine...  But in testing it, I once again come to see how
screwy -Qu is!  We need to go back to your original issue.


-Qu ignores databases with Usage = Sync or Install when calculating the
list of possible package updates, but not databases with Usage = Search.

It gets better...   packages in databases with Usage = Upgrade do not
show up in -Qu.  That is clearly wrong!


What is special about Usage = Search?  I can't see why these packages
should be included in a -Qu output and not those from a database with
Usage = Sync/Install/Upgrade.


Lets look at the relevant function:

/** Check for new version of pkg in sync repos
 * (only the first occurrence is considered in sync)
 */
alpm_pkg_t SYMEXPORT *alpm_sync_newversion(alpm_pkg_t *pkg, alpm_list_t
*dbs_sync)
{

We need to define the sync repos that are searched.  We either look at
all sync repos, or those with Usage = Upgrade/All.  I can't see a
justification for anything else.


Going to "man pacman" for -Qu

-u, --upgrades
   Restrict or filter output to packages that are out-of-date on the
   local system. Only package versions are used to find outdated
   packages; replacements are not checked here. This option works best
   if the sync database is refreshed using -Sy.

The option name implies we should only print those with Usage =
Upgrade/All, but the description suggests we can print all of them.  I'm
inclined to go with the option name, as that is less mutable than the
description.


My plan is to change alpm_sync_newversion to take an additional
parameter to flag whether it should include all databases or just those
available for upgrades.  It could even be make more flexible and just
take the bitmask as a parameter for filtering.

Allan
Morgan Adamiec Sept. 19, 2018, 5:18 p.m. UTC | #3
Yeah -Qu is still a little funky.

> It gets better...   packages in databases with Usage = Upgrade do not
> show up in -Qu.  That is clearly wrong!

I left the requirement on Usage = Search after every one disagreed with
my patch [1] to change it. Although I would prefer it if Search had no
bearing on this.

So with this patch, Search is needed for a package to be listed and
Upgrade is needed for [ignore] to not be added.

> My plan is to change alpm_sync_newversion to take an additional
> parameter to flag whether it should include all databases or just those
> available for upgrades.  It could even be make more flexible and just
> take the bitmask as a parameter for filtering.

Why not just remove the Usage check all together? You need
alpm_sync_newversion to find the packages then later we do the Usage =
Upgrade check to add [ignored] to the end of the line.

Unless you want the packages to not show up at all? That's exactly what
the previous patch [1] did.

[1] https://lists.archlinux.org/pipermail/pacman-dev/2018-July/022723.html
Allan McRae Sept. 19, 2018, 9:27 p.m. UTC | #4
On 20/9/18 3:18 am, Morgan Adamiec wrote:
> Yeah -Qu is still a little funky.
> 
>> It gets better...   packages in databases with Usage = Upgrade do not
>> show up in -Qu.  That is clearly wrong!
> 
> I left the requirement on Usage = Search after every one disagreed with
> my patch [1] to change it. Although I would prefer it if Search had no
> bearing on this.
> 
> So with this patch, Search is needed for a package to be listed and
> Upgrade is needed for [ignore] to not be added.
> 
>> My plan is to change alpm_sync_newversion to take an additional
>> parameter to flag whether it should include all databases or just those
>> available for upgrades.  It could even be make more flexible and just
>> take the bitmask as a parameter for filtering.
> 
> Why not just remove the Usage check all together? You need
> alpm_sync_newversion to find the packages then later we do the Usage =
> Upgrade check to add [ignored] to the end of the line.

My concern is of users of that function other than pacman (if there are
any...).  Adding a parameter at least flags the change in behaviour an
allows other users to keep it the way it was.

> Unless you want the packages to not show up at all? That's exactly what
> the previous patch [1] did.
> 
> [1] https://lists.archlinux.org/pipermail/pacman-dev/2018-July/022723.html
> .
>
Morgan Adamiec Sept. 19, 2018, 9:41 p.m. UTC | #5
On Wed, 19 Sep 2018 at 22:28, Allan McRae <allan@archlinux.org> wrote:
> My concern is of users of that function other than pacman (if there are
> any...).  Adding a parameter at least flags the change in behaviour an
> allows other users to keep it the way it was.

Ah yeah good point, forgot that other frontends are a thing. Your idea
is probably
best then. Thanks for all the feedback.

Patch
diff mbox

diff --git a/src/pacman/query.c b/src/pacman/query.c
index 00c39638..faa06e60 100644
--- a/src/pacman/query.c
+++ b/src/pacman/query.c
@@ -325,10 +325,14 @@  static int display(alpm_pkg_t *pkg)
 					colstr->version, alpm_pkg_get_version(pkg), colstr->nocolor);
 
 			if(config->op_q_upgrade) {
+				int usage;
 				alpm_pkg_t *newpkg = alpm_sync_newversion(pkg, alpm_get_syncdbs(config->handle));
+				alpm_db_t *db = alpm_pkg_get_db(newpkg);
+				alpm_db_get_usage(db, &usage);
+
 				printf(" -> %s%s%s", colstr->version, alpm_pkg_get_version(newpkg), colstr->nocolor);
 
-				if(alpm_pkg_should_ignore(config->handle, pkg)) {
+				if(alpm_pkg_should_ignore(config->handle, pkg) || !(usage & ALPM_DB_USAGE_UPGRADE)) {
 					printf(" %s", _("[ignored]"));
 				}
 			}