[pacman-dev] libalpm/sync.c: restrict alpm_sync_newversion by USAGE_UPGRADE
diff mbox

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

Commit Message

Morgan Adamiec July 24, 2018, 4:53 p.m. UTC
Commit 106d0fc54 Added the usage option for databases and
alpm_sync_newversion was restricted by USAGE_SEARCH instead of
USAGE_UPGRADE.

USAGE_UPGRADE should be used instead. This means packages only show up
in commands such as `pacman -Qu` if the database Has the Upgrade option.

Signed-off-by: morganamilo <morganamilo@gmail.com>

Comments

Dave Reisner July 24, 2018, 6:48 p.m. UTC | #1
On Tue, Jul 24, 2018 at 05:53:00PM +0100, morganamilo wrote:
> Commit 106d0fc54 Added the usage option for databases and
> alpm_sync_newversion was restricted by USAGE_SEARCH instead of
> USAGE_UPGRADE.

I don't recall exactly what my thinking was when I wrote this patch, but
looking at 'pacman -Qu' output right now, I think I actually I like
seeing potential upgrades, not just "actual" upgrades.

Anyone else have an opinion?

> USAGE_UPGRADE should be used instead. This means packages only show up
> in commands such as `pacman -Qu` if the database Has the Upgrade option.
> 
> Signed-off-by: morganamilo <morganamilo@gmail.com>
> 
> diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
> index 696a5131..23b0ccfa 100644
> --- a/lib/libalpm/sync.c
> +++ b/lib/libalpm/sync.c
> @@ -61,7 +61,7 @@ alpm_pkg_t SYMEXPORT *alpm_sync_newversion(alpm_pkg_t *pkg, alpm_list_t *dbs_syn
>  
>  	for(i = dbs_sync; !spkg && i; i = i->next) {
>  		alpm_db_t *db = i->data;
> -		if(!(db->usage & ALPM_DB_USAGE_SEARCH)) {
> +		if(!(db->usage & ALPM_DB_USAGE_UPGRADE)) {
>  			continue;
>  		}
>  
> -- 
> 2.18.0
Erich Eckner July 24, 2018, 7:23 p.m. UTC | #2
On 24.07.2018 20:48, Dave Reisner wrote:
> On Tue, Jul 24, 2018 at 05:53:00PM +0100, morganamilo wrote:
>> Commit 106d0fc54 Added the usage option for databases and
>> alpm_sync_newversion was restricted by USAGE_SEARCH instead of
>> USAGE_UPGRADE.
> 
> I don't recall exactly what my thinking was when I wrote this patch, but
> looking at 'pacman -Qu' output right now, I think I actually I like
> seeing potential upgrades, not just "actual" upgrades.
> 
> Anyone else have an opinion?

Frankly, I don't really understand what would change - what does "the
database Has the Upgrade option" mean? What is the difference between an
"actual" and a "potential" upgrade?

Please exuse if these are stupid questions - I'm just a user of "-Qu".

regards,
Erich

> 
>> USAGE_UPGRADE should be used instead. This means packages only show up
>> in commands such as `pacman -Qu` if the database Has the Upgrade option.
>>
>> Signed-off-by: morganamilo <morganamilo@gmail.com>
>>
>> diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
>> index 696a5131..23b0ccfa 100644
>> --- a/lib/libalpm/sync.c
>> +++ b/lib/libalpm/sync.c
>> @@ -61,7 +61,7 @@ alpm_pkg_t SYMEXPORT *alpm_sync_newversion(alpm_pkg_t *pkg, alpm_list_t *dbs_syn
>>  
>>  	for(i = dbs_sync; !spkg && i; i = i->next) {
>>  		alpm_db_t *db = i->data;
>> -		if(!(db->usage & ALPM_DB_USAGE_SEARCH)) {
>> +		if(!(db->usage & ALPM_DB_USAGE_UPGRADE)) {
>>  			continue;
>>  		}
>>  
>> -- 
>> 2.18.0
Morgan Adamiec July 24, 2018, 7:31 p.m. UTC | #3
On Tue, 24 Jul 2018 at 20:24, Erich Eckner <arch@eckner.net> wrote:
>
> On 24.07.2018 20:48, Dave Reisner wrote:
> > On Tue, Jul 24, 2018 at 05:53:00PM +0100, morganamilo wrote:
> >> Commit 106d0fc54 Added the usage option for databases and
> >> alpm_sync_newversion was restricted by USAGE_SEARCH instead of
> >> USAGE_UPGRADE.
> >
> > I don't recall exactly what my thinking was when I wrote this patch, but
> > looking at 'pacman -Qu' output right now, I think I actually I like
> > seeing potential upgrades, not just "actual" upgrades.
> >
> > Anyone else have an opinion?
>
> Frankly, I don't really understand what would change - what does "the
> database Has the Upgrade option" mean? What is the difference between an
> "actual" and a "potential" upgrade?
>
> Please exuse if these are stupid questions - I'm just a user of "-Qu".
>
> regards,
> Erich
>
> >
> >> USAGE_UPGRADE should be used instead. This means packages only show up
> >> in commands such as `pacman -Qu` if the database Has the Upgrade option.
> >>
> >> Signed-off-by: morganamilo <morganamilo@gmail.com>
> >>
> >> diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
> >> index 696a5131..23b0ccfa 100644
> >> --- a/lib/libalpm/sync.c
> >> +++ b/lib/libalpm/sync.c
> >> @@ -61,7 +61,7 @@ alpm_pkg_t SYMEXPORT *alpm_sync_newversion(alpm_pkg_t *pkg, alpm_list_t *dbs_syn
> >>
> >>      for(i = dbs_sync; !spkg && i; i = i->next) {
> >>              alpm_db_t *db = i->data;
> >> -            if(!(db->usage & ALPM_DB_USAGE_SEARCH)) {
> >> +            if(!(db->usage & ALPM_DB_USAGE_UPGRADE)) {
> >>                      continue;
> >>              }
> >>
> >> --
> >> 2.18.0
>

See the Usage option in pacman.conf (5)

Personally I would like -Qu to show actual upgrades. Mainly for the
checkupdates script. Which many people use create update notifiers.
It's quite annoying to have a notification always saying there's an
update available when in reality it's just a package in a database
that you only have Usage = Sync Search.

On a lower level I would also like to use alpm_sync_newversion so get
pending upgrades. The alternative right now Is starting a transaction
and calling alpm_sync_sysupgrade.
Andrew Gregory July 24, 2018, 7:40 p.m. UTC | #4
On 07/24/18 at 02:48pm, Dave Reisner wrote:
> On Tue, Jul 24, 2018 at 05:53:00PM +0100, morganamilo wrote:
> > Commit 106d0fc54 Added the usage option for databases and
> > alpm_sync_newversion was restricted by USAGE_SEARCH instead of
> > USAGE_UPGRADE.
> 
> I don't recall exactly what my thinking was when I wrote this patch, but
> looking at 'pacman -Qu' output right now, I think I actually I like
> seeing potential upgrades, not just "actual" upgrades.
> 
> Anyone else have an opinion?

Based on how it's used, I'd say it should be SEARCH; it's being used
as a filter for -Q and no upgrade transaction is being performed, or
even prepared.

Really, though, I'd say this is a great example as to why usages
should have been implemented in the front-end or limited to only the
highest-level library functions.  Usage is contextual, how is libalpm
supposed to know how such a basic function is being used?  pacman may
only use it as a filter for -Q, but some other front-end could use it
to actually prepare an upgrade.
Morgan Adamiec July 25, 2018, 1:23 a.m. UTC | #5
On Tue, 24 Jul 2018 at 20:40, Andrew Gregory <andrew.gregory.8@gmail.com> wrote:
> Based on how it's used, I'd say it should be SEARCH; it's being used
> as a filter for -Q and no upgrade transaction is being performed, or
> even prepared.
>
> Really, though, I'd say this is a great example as to why usages
> should have been implemented in the front-end or limited to only the
> highest-level library functions.  Usage is contextual, how is libalpm
> supposed to know how such a basic function is being used?  pacman may
> only use it as a filter for -Q, but some other front-end could use it
> to actually prepare an upgrade.

The thing is pacman with not let you use -s with -u: error: invalid
option: '--search' and '--upgrade' may not be used together. By that
logic you could argue it is not a search at all.
Front ends aside the function is called alpm_sync_newversion, it makes
no mention to searching.

Slightly off topic of the original patch. Playing around more I've
come to find that Upgrade implies Install. Is that an oversight or is
it intended?
As for use case I would like tp pacman -S db/pkg explicitly and have
them upgrade but never touch the repo during a normal pacman -S pkg.
Andrew Gregory July 25, 2018, 1:28 a.m. UTC | #6
On 07/25/18 at 02:23am, Morgan Adamiec wrote:
> On Tue, 24 Jul 2018 at 20:40, Andrew Gregory <andrew.gregory.8@gmail.com> wrote:
> > Based on how it's used, I'd say it should be SEARCH; it's being used
> > as a filter for -Q and no upgrade transaction is being performed, or
> > even prepared.
> >
> > Really, though, I'd say this is a great example as to why usages
> > should have been implemented in the front-end or limited to only the
> > highest-level library functions.  Usage is contextual, how is libalpm
> > supposed to know how such a basic function is being used?  pacman may
> > only use it as a filter for -Q, but some other front-end could use it
> > to actually prepare an upgrade.
> 
> The thing is pacman with not let you use -s with -u: error: invalid
> option: '--search' and '--upgrade' may not be used together. By that
> logic you could argue it is not a search at all.
> Front ends aside the function is called alpm_sync_newversion, it makes
> no mention to searching.

It makes no mention of upgrading either.

> Slightly off topic of the original patch. Playing around more I've
> come to find that Upgrade implies Install. Is that an oversight or is
> it intended?
> As for use case I would like tp pacman -S db/pkg explicitly and have
> them upgrade but never touch the repo during a normal pacman -S pkg.

How is that different from just putting that repo below the one you
want it to use normally?
Morgan Adamiec July 25, 2018, 1:35 a.m. UTC | #7
On Wed, 25 Jul 2018 at 02:28, Andrew Gregory <andrew.gregory.8@gmail.com> wrote:
> It makes no mention of upgrading either

It is called alpm_sync_*newversion*

>How is that different from just putting that repo below the one you
want it to use normally?

pacman -S pkg should return an error. These packages are not in any
other of my repos, so there is nothing to move it under really.

But bedsides that It would just make more sense for Upgrade to not
imply install. Or at least document that it does.
Eli Schwartz July 25, 2018, 3:51 a.m. UTC | #8
On 07/24/2018 02:48 PM, Dave Reisner wrote:
> On Tue, Jul 24, 2018 at 05:53:00PM +0100, morganamilo wrote:
>> Commit 106d0fc54 Added the usage option for databases and
>> alpm_sync_newversion was restricted by USAGE_SEARCH instead of
>> USAGE_UPGRADE.
> 
> I don't recall exactly what my thinking was when I wrote this patch, but
> looking at 'pacman -Qu' output right now, I think I actually I like
> seeing potential upgrades, not just "actual" upgrades.
> 
> Anyone else have an opinion?

I don't really see the utility... a search-only database will not show
the upgrades in pacman -Su (even ignored packages get shown, just as a
warning instead of an active transaction member).

pacman -Qu should therefore (IMHO) only show those Sync packages (with
[ignored] in cases, yes).

When people hear about -Qu, they generally hear about it as "oh, the
thing which is like -Su except it doesn't actually do the thing".

Admittedly the actual thing which doesn't actually do the thing would be
pacman -Sup --print-format '%r/%n %v'

But then we're faced with the classic problem "print-format? no such
thing, we don't support that and you should use expac, HAHAHAHA, oh wait
print-format is actually useful here (surprise!)"

So actually I guess in theory it's completely different, but now I'm not
sure what I would specifically use this behavior for.
Eli Schwartz July 25, 2018, 3:58 a.m. UTC | #9
On 07/24/2018 09:23 PM, Morgan Adamiec wrote:
> On Tue, 24 Jul 2018 at 20:40, Andrew Gregory <andrew.gregory.8@gmail.com> wrote:
>> Based on how it's used, I'd say it should be SEARCH; it's being used
>> as a filter for -Q and no upgrade transaction is being performed, or
>> even prepared.
>>
>> Really, though, I'd say this is a great example as to why usages
>> should have been implemented in the front-end or limited to only the
>> highest-level library functions.  Usage is contextual, how is libalpm
>> supposed to know how such a basic function is being used?  pacman may
>> only use it as a filter for -Q, but some other front-end could use it
>> to actually prepare an upgrade.
> 
> The thing is pacman with not let you use -s with -u: error: invalid
> option: '--search' and '--upgrade' may not be used together. By that
> logic you could argue it is not a search at all.

It has nothing to do with "upgrades", -S means different things
depending on whether you just give it package names, a bunch of inferred
package names via -u, or whatever.

-Us doesn't work either. Nor does -Ssw.

Likewise this low-level function I guess is not called just by pacman
-Su ...

> Front ends aside the function is called alpm_sync_newversion, it makes
> no mention to searching.

It is a function to find packages from sync repositories (in contrast to
the "local" repository) that have new versions. I think it's obvious
this function does not handle the actual syncing...

Anyway. Seems to me the name is a reference to the local/remote nature
of the repo, not its Usage field.

> Slightly off topic of the original patch. Playing around more I've
> come to find that Upgrade implies Install. Is that an oversight or is
> it intended?

It implies this where? Shouldn't anything using it be
checking/specifying both bitmasks?
Allan McRae Aug. 29, 2018, 4:30 a.m. UTC | #10
On 25/07/18 04:48, Dave Reisner wrote:
> On Tue, Jul 24, 2018 at 05:53:00PM +0100, morganamilo wrote:
>> Commit 106d0fc54 Added the usage option for databases and
>> alpm_sync_newversion was restricted by USAGE_SEARCH instead of
>> USAGE_UPGRADE.
> I don't recall exactly what my thinking was when I wrote this patch, but
> looking at 'pacman -Qu' output right now, I think I actually I like
> seeing potential upgrades, not just "actual" upgrades.
> 
> Anyone else have an opinion?
> 

I just spent some time looking into this...  only a month after submission!

My thoughts:

1) Implementing --print-format usage for -Su is the correct way to
determine updates.  (I guess in combination with --no-confirm?)

2) -Qu should label those package updates that are available but won't
be updated by -Su due to database restriction.  We currently do this for
packages in IgnorePkg, so should be an "easy" extension.

Would that be suitable for everyone?

Allan
Eli Schwartz Aug. 29, 2018, 4:59 a.m. UTC | #11
On 8/29/18 12:30 AM, Allan McRae wrote:
> On 25/07/18 04:48, Dave Reisner wrote:
>> On Tue, Jul 24, 2018 at 05:53:00PM +0100, morganamilo wrote:
>>> Commit 106d0fc54 Added the usage option for databases and
>>> alpm_sync_newversion was restricted by USAGE_SEARCH instead of
>>> USAGE_UPGRADE.
>> I don't recall exactly what my thinking was when I wrote this patch, but
>> looking at 'pacman -Qu' output right now, I think I actually I like
>> seeing potential upgrades, not just "actual" upgrades.
>>
>> Anyone else have an opinion?
>>
> 
> I just spent some time looking into this...  only a month after submission!
> 
> My thoughts:
> 
> 1) Implementing --print-format usage for -Su is the correct way to
> determine updates.  (I guess in combination with --no-confirm?)

It works today, though, as I mentioned above. The only "problem" is that
people never remember --print-format exists, mostly because it's not
implemented in all the *other* cases where we'd want to have it.

> 2) -Qu should label those package updates that are available but won't
> be updated by -Su due to database restriction.  We currently do this for
> packages in IgnorePkg, so should be an "easy" extension.
> 
> Would that be suitable for everyone?

That sounds like something very nice to have, yes.
Allan McRae Aug. 29, 2018, 5:03 a.m. UTC | #12
On 29/08/18 14:59, Eli Schwartz wrote:
>> My thoughts:
>>
>> 1) Implementing --print-format usage for -Su is the correct way to
>> determine updates.  (I guess in combination with --no-confirm?)
>
> It works today, though, as I mentioned above. The only "problem" is that
> people never remember --print-format exists, mostly because it's not
> implemented in all the *other* cases where we'd want to have it.
> 

Well...   that is embarrassing!  Seems I forgot it works for -Su too!

Someone should send the pacman-contrib maintainers a patch to use that
instead of -Qu.

A
Morgan Adamiec Aug. 31, 2018, 8:41 a.m. UTC | #13
On Wed, 29 Aug 2018 at 06:03, Allan McRae <allan@archlinux.org> wrote:
> Someone should send the pacman-contrib maintainers a patch to use that
> instead of -Qu.

Sadly you would need some sort of 'currentpkgver' formatter
`--print-format "%n %currentpkgver -> %v"`.

Patch
diff mbox

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
index 696a5131..23b0ccfa 100644
--- a/lib/libalpm/sync.c
+++ b/lib/libalpm/sync.c
@@ -61,7 +61,7 @@  alpm_pkg_t SYMEXPORT *alpm_sync_newversion(alpm_pkg_t *pkg, alpm_list_t *dbs_syn
 
 	for(i = dbs_sync; !spkg && i; i = i->next) {
 		alpm_db_t *db = i->data;
-		if(!(db->usage & ALPM_DB_USAGE_SEARCH)) {
+		if(!(db->usage & ALPM_DB_USAGE_UPGRADE)) {
 			continue;
 		}