On 10/30/07, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
On 10/26/07, Nagy Gabor <ngaba@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.)
I'm actually just commenting on the variable name itself. Not the code. The variable name is unclear. I didn't really need all the rationalization behind it. I just think that seeing "dep->inducer" gives me no information because the variable name is confusing. Could you change this to be a little more clear and resubmit?
+int _alpm_pkgname_pkg_cmp(const void *pkgname, const void I can repeat myself: we can live without this. But IMHO this is nicer than get
Ok, in this case, could you resubmit without this function, to make things cleaner. We can add this later as another patch. (You might want to take a look at "git commit --amend"