[pacman-dev] [PATCH] New -Ru option

[Nagy Gabor]

> On 10/26/07, Nagy Gabor <ngaba at bibl.u-szeged.hu> wrote:
> > >+                      printf(_("  -u, --unneeded       remove only
> > >unneeded packages (that won't break packages)\n"));
> > Hm. This description may not be correct ("remove unneeded packages only"
> > sounds better for me). But I won't fix it now, because I want to
> > motivate you to fix these "Hunglish" problems (I'm not sure on
> > "target-list" neither...). You have probably realized already, that my
> > English is far from perfect ;-), so I leave these works for you, sorry.
> 
> Hah. "Hunglish"
> 
> Just for the record... both sound a little "forced". I think it's
> actually the word "only" that is making it feel funny (to me). "remove
> unneeded packages" sounds better, but that's just my opinion.
> 
> Regarding the patch... a couple of comments:
> 
> > --- a/lib/libalpm/deps.h
> > +++ b/lib/libalpm/deps.h
> > @@ -40,6 +40,7 @@ struct __pmdepmissing_t {
> >         char target[PKG_NAME_LEN];
> >         pmdeptype_t type;
> >         pmdepend_t depend;
> > +       char inducer[PKG_NAME_LEN]; /* this is used in case of remove
> dependency error only */
> 
> Hrm. I don't know if I like the name "inducer" - could you explain
> what you meant with this so we could maybe use a clearer term?

OK. First of all, this is useful with "pacman -R" only.(*)
If you do a "pacman -R foo", checkdeps will collect the broken dependencies
(after the theoretical commit), for it will return with "bar" requires "prov"
("foo" provides "prov" here, and there is no other installed "prov"-provider). So
1. user get a not really informative error message; that's why I wrote in patch
description: "miss->depend (EDIT: typo, I meant miss->inducer) will be useful in
-R dependency error messages too."
2. -Ru needs this information too to figure out the "needed" packages.
Of course we can live without inducer field (which indicates the
"dependency-broker" package): we just checking the target list again; but that
is simply needless, and using alpm_depcmp and such things in pacman/remove.c is
not nice imho.
Note: I'm not sure on (*), this can be useful with other commands, too:
I hate these type of error messages:
"pacman -S foo"
ERROR: bar requires baz=1.0
(This can happen in the following case: resolvedeps pulls baz, since new version
is needed for foo; but updating baz 1.0 to 1.1 would break the "baz=1.0" depend
of bar <- resolvedeps doesn't resolve this.)

> > +int _alpm_pkgname_pkg_cmp(const void *pkgname, const void *package)
> 
> I don't know if I'm a fan of this function here. Seems a bit
> excessive, BUT if it does have a lot of usages, could you submit this
> a separate (small) patch, just so we can push it in there?

I can repeat myself: we can live without this. But IMHO this is nicer than get
pkg from the list by name, then alpm_list_remove... or create a pseudo package
with only pkgname filled in (we do the "same" thing twice). This can be used in
every place where "removing package from target list" is needed (conflict
resolving, for example).
Or an alternative function: get pkg's alpm_list_node from the list by its name,
and this can be removed by alpm_list_remove_node. But IMHO this is uglier.

> Other than that, it all looks sound. And I do like the -Ru feature, myself.


----------------------------------------------------
SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu
This mail sent through IMP: http://horde.org/imp/