[pacman-dev] -Si is laconic
Hi! When I made the previously posted speed tests, I noticed that -Si hides lots of info. Is there any special reason for this (we are not proud of our packagers for example;-) or we just forgot to update dump_pkg_sync in pacman/package.c? I think that dump_pkg_sync should be merged to dump_pkg_full (with a new is_local input parameter for example) to keep -Qi and -Si in sync. And this is a good time (just before the 3.1 release) to review -Qi too. I would do this, but I'm not sure that you would like an "ngaba-English" special edition of pacman :-) Bye ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/
Laconic? Heh. On Nov 12, 2007 6:14 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
When I made the previously posted speed tests, I noticed that -Si hides lots of info. Is there any special reason for this (we are not proud of our packagers for example;-) or we just forgot to update dump_pkg_sync in pacman/package.c? I think that dump_pkg_sync should be merged to dump_pkg_full (with a new is_local input parameter for example) to keep -Qi and -Si in sync. And this is a good time (just before the 3.1 release) to review -Qi too.
This is a good point, but also keep in mind that sync DBs and local DBs do not contain the exact same information.
I would do this, but I'm not sure that you would like an "ngaba-English" special edition of pacman :-)
This should be simple enough if you're just combining the two dump_pkg_* functions. A patch would be nice
Hi! Patch attached. Note: I renamed "License" to "Licenses" and added "Download Size". Bye ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/
OK. Now it is really attached. ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/
On 11/13/2007 04:50 AM, Nagy Gabor wrote:
OK. Now it is really attached.
I just wanted to point out the following patch on my working branch: <http://code.toofishes.net/gitweb.cgi?p=pacman.git;a=commitdiff;h=5e12d3dec99e7a506683cf625fa4344f57df0b77;hp=a0c908dd0da4a00cc98a46407534da67d4aee8a8> Because your patch does a lot more with this code than mine does, hopefully you can incorporate the fixes into your patch as well.
From 45b6b4a84a923f1170275ca0cba683f9602a0095 Mon Sep 17 00:00:00 2001 From: Nagy Gabor <ngaba@bibl.u-szeged.hu> Date: Tue, 13 Nov 2007 08:59:26 +0100 Subject: [PATCH] Merge dump_pkg_sync to dump_pkg_full in pacman/package.c dump_pkg_sync just prints the reponame then calls dump_pkg_full for package info From now on we don't print false values (e.g. Installed Size 0 K), we just skip those lines
Signed-off-by: Nagy Gabor <ngaba@bibl.u-szeged.hu> --- src/pacman/package.c | 117 +++++++++++++++++++++++++------------------------- src/pacman/sync.c | 5 +- 2 files changed, 61 insertions(+), 61 deletions(-)
diff --git a/src/pacman/package.c b/src/pacman/package.c index 2e2eec9..33a346b 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -35,72 +35,95 @@ #include "package.h" #include "util.h"
-/* Display the content of an installed package +/* Display the content of a package * - * level: <1 - omits N/A info for file query (-Qp) - * 1 - normal level - * >1 - extra information (backup files) + * level: 0 - package is not installed + * 1 - package is installed, normal level + * >1 - package is installed, extra information (backup files) */ void dump_pkg_full(pmpkg_t *pkg, int level) { - const char *reason, *descheader; - time_t bdate, idate; - char bdatestr[50], idatestr[50]; + const char *reason=NULL, *descheader, *md5sum=NULL; + const char *tmp; + time_t date; + char bdatestr[50]="", idatestr[50] = "";
if(pkg == NULL) { return; }
/* set variables here, do all output below */ - bdate = alpm_pkg_get_builddate(pkg); - strftime(bdatestr, 50, "%c", localtime(&bdate)); - idate = alpm_pkg_get_installdate(pkg); - strftime(idatestr, 50, "%c", localtime(&idate)); - - switch((long)alpm_pkg_get_reason(pkg)) { - case PM_PKG_REASON_EXPLICIT: - reason = _("Explicitly installed"); - break; - case PM_PKG_REASON_DEPEND: - reason = _("Installed as a dependency for another package"); - break; - default: - reason = _("Unknown"); - break; + date = alpm_pkg_get_builddate(pkg); + if(date) { + strftime(bdatestr, 50, "%c", localtime(&date)); + } + if(level) { + date = alpm_pkg_get_installdate(pkg); + if(date) { + strftime(idatestr, 50, "%c", localtime(&date)); + } + switch((long)alpm_pkg_get_reason(pkg)) { + case PM_PKG_REASON_EXPLICIT: + reason = _("Explicitly installed"); + break; + case PM_PKG_REASON_DEPEND: + reason = _("Installed as a dependency for another package"); + break; + default: + reason = _("Unknown"); + break; + } + } else { + md5sum = alpm_pkg_get_md5sum(pkg); } - descheader = _("Description : ");
/* actual output */ printf(_("Name : %s\n"), (char *)alpm_pkg_get_name(pkg)); printf(_("Version : %s\n"), (char *)alpm_pkg_get_version(pkg)); - printf(_("URL : %s\n"), (char *)alpm_pkg_get_url(pkg)); - list_display(_("License :"), alpm_pkg_get_licenses(pkg)); + tmp = (char *)alpm_pkg_get_url(pkg); + if(*tmp) printf(_("URL : %s\n"), tmp);
Two things: 1. All the other variables were done above, so should this one. 2. I'm confused why we need this cast. 3. We don't do one line if statements anywhere in the code, so don't do it here.
+ list_display(_("Licenses :"), alpm_pkg_get_licenses(pkg)); Good fix.
list_display(_("Groups :"), alpm_pkg_get_groups(pkg)); list_display(_("Provides :"), alpm_pkg_get_provides(pkg)); list_display(_("Depends On :"), alpm_pkg_get_depends(pkg)); list_display(_("Optional Deps :"), alpm_pkg_get_optdepends(pkg)); /* Only applicable if installed */ - if(level > 0) { + if(level) { I'd prefer to keep it explicit just because this isn't a binary value. I know this works, but no need to be cryptic.
list_display(_("Required By :"), alpm_pkg_get_requiredby(pkg)); } list_display(_("Conflicts With :"), alpm_pkg_get_conflicts(pkg)); list_display(_("Replaces :"), alpm_pkg_get_replaces(pkg)); - printf(_("Installed Size : %6.2f K\n"), (float)alpm_pkg_get_size(pkg) / 1024.0); - printf(_("Packager : %s\n"), (char *)alpm_pkg_get_packager(pkg)); - printf(_("Architecture : %s\n"), (char *)alpm_pkg_get_arch(pkg)); - printf(_("Build Date : %s\n"), bdatestr); - if(level > 0) { - printf(_("Install Date : %s\n"), idatestr); + /* %SIZE%, %CSIZE%, %ISIZE% */
Yeah, it sucks that we have all sorts of brokenness here. Look at my patch for a cleaned up approach (you can always use get_isize for installed size). And do we show package size anywhere anymore? Some people might find that interesting and this would be the place to show it, although it only applies for non-installed packages.
+ if(level) { + unsigned long temp = alpm_pkg_get_size(pkg); + if(temp) printf(_("Installed Size : %6.2f K\n"), (float)temp / 1024.0); + } else { + unsigned long temp = alpm_pkg_get_size(pkg); + if(temp) printf(_("Download Size : %6.2f K\n"), (float)temp / 1024.0); + temp = alpm_pkg_get_isize(pkg); + if(temp) printf(_("Installed Size : %6.2f K\n"), (float)temp / 1024.0); + } + tmp = (char *)alpm_pkg_get_packager(pkg); + if(*tmp) printf(_("Packager : %s\n"), tmp); + tmp = (char *)alpm_pkg_get_arch(pkg); + if(*tmp) printf(_("Architecture : %s\n"), tmp); Once again, I do not understand why you need to remove the const pointer. This is not safe. I'd rather have 10 different variable names up above than these casts.
+ if(*bdatestr) printf(_("Build Date : %s\n"), bdatestr); + if(level) { + if(*idatestr) printf(_("Install Date : %s\n"), idatestr); printf(_("Install Reason : %s\n"), reason); If install date is NULL, then install reason shouldn't be printed either. And bdatestr/idatestr are never null- you declared it as such above.
+ printf(_("Install Script : %s\n"), + alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No")); Install script should be printed whether installed or not (although sync repos probably don't have this information, but queried package files do).
} - printf(_("Install Script : %s\n"), - alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No"));
/* printed using a variable to make i18n safe */ printf("%s", descheader); indentprint(alpm_pkg_get_desc(pkg), mbstowcs(NULL, descheader, 0)); printf("\n"); + + if(md5sum != NULL && md5sum[0] != '\0') { + printf(_("MD5 Sum : %s"), md5sum); + }
/* Print additional package info if info flag passed more than once */ if(level > 1) { @@ -115,35 +138,11 @@ void dump_pkg_full(pmpkg_t *pkg, int level) */ void dump_pkg_sync(pmpkg_t *pkg, const char *treename) { - const char *descheader, *md5sum; if(pkg == NULL) { return; } - - descheader = _("Description : "); - - md5sum = alpm_pkg_get_md5sum(pkg); - printf(_("Repository : %s\n"), treename); - printf(_("Name : %s\n"), (char *)alpm_pkg_get_name(pkg)); - printf(_("Version : %s\n"), (char *)alpm_pkg_get_version(pkg)); - list_display(_("Groups :"), alpm_pkg_get_groups(pkg)); - list_display(_("Provides :"), alpm_pkg_get_provides(pkg)); - list_display(_("Depends On :"), alpm_pkg_get_depends(pkg)); - list_display(_("Conflicts With :"), alpm_pkg_get_conflicts(pkg)); - list_display(_("Replaces :"), alpm_pkg_get_replaces(pkg)); - printf(_("Download Size : %6.2f K\n"), (float)alpm_pkg_get_size(pkg) / 1024.0); - printf(_("Installed Size : %6.2f K\n"), (float)alpm_pkg_get_isize(pkg) / 1024.0); - - /* printed using a variable to make i18n safe */ - printf("%s", descheader); - indentprint(alpm_pkg_get_desc(pkg), mbstowcs(NULL, descheader, 0)); - printf("\n"); - - if (md5sum != NULL && md5sum[0] != '\0') { - printf(_("MD5 Sum : %s"), md5sum); - } - printf("\n"); + dump_pkg_full(pkg, 0); }
/* Display list of backup files and their modification states diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 5f1072f..9b2aedc 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -295,7 +295,7 @@ static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) pmpkg_t *pkg = alpm_list_getdata(k);
if(strcmp(alpm_pkg_get_name(pkg), pkgstr) == 0) { - dump_pkg_sync(pkg, alpm_db_get_name(db)); + dump_pkg_sync(pkg, repo); printf("\n"); foundpkg = 1; break; @@ -332,9 +332,10 @@ static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) } else { for(i = syncs; i; i = alpm_list_next(i)) { pmdb_t *db = alpm_list_getdata(i); + const char *dbname = alpm_db_get_name(db);
for(j = alpm_db_getpkgcache(db); j; j = alpm_list_next(j)) { - dump_pkg_sync(alpm_list_getdata(j), alpm_db_get_name(db)); + dump_pkg_sync(alpm_list_getdata(j), dbname); printf("\n"); } } -- 1.5.3.5
A -Qip operation and -Si option may differ enough that it is worth keeping them separate and adding an additional level, but it is your code. :) -Dan
On 11/13/2007 04:50 AM, Nagy Gabor wrote:
OK. Now it is really attached.
I just wanted to point out the following patch on my working branch:
Because your patch does a lot more with this code than mine does, hopefully you can incorporate the fixes into your patch as well.
From 45b6b4a84a923f1170275ca0cba683f9602a0095 Mon Sep 17 00:00:00 2001 From: Nagy Gabor <ngaba@bibl.u-szeged.hu> Date: Tue, 13 Nov 2007 08:59:26 +0100 Subject: [PATCH] Merge dump_pkg_sync to dump_pkg_full in pacman/package.c dump_pkg_sync just prints the reponame then calls dump_pkg_full for package info From now on we don't print false values (e.g. Installed Size 0 K), we just skip those lines
Signed-off-by: Nagy Gabor <ngaba@bibl.u-szeged.hu> --- src/pacman/package.c | 117 +++++++++++++++++++++++++------------------------- src/pacman/sync.c | 5 +- 2 files changed, 61 insertions(+), 61 deletions(-)
diff --git a/src/pacman/package.c b/src/pacman/package.c index 2e2eec9..33a346b 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -35,72 +35,95 @@ #include "package.h" #include "util.h"
-/* Display the content of an installed package +/* Display the content of a package * - * level: <1 - omits N/A info for file query (-Qp) - * 1 - normal level - * >1 - extra information (backup files) + * level: 0 - package is not installed + * 1 - package is installed, normal level + * >1 - package is installed, extra information (backup files) */ void dump_pkg_full(pmpkg_t *pkg, int level) { - const char *reason, *descheader; - time_t bdate, idate; - char bdatestr[50], idatestr[50]; + const char *reason=NULL, *descheader, *md5sum=NULL; + const char *tmp; + time_t date; + char bdatestr[50]="", idatestr[50] = "";
if(pkg == NULL) { return; }
/* set variables here, do all output below */ - bdate = alpm_pkg_get_builddate(pkg); - strftime(bdatestr, 50, "%c", localtime(&bdate)); - idate = alpm_pkg_get_installdate(pkg); - strftime(idatestr, 50, "%c", localtime(&idate)); - - switch((long)alpm_pkg_get_reason(pkg)) { - case PM_PKG_REASON_EXPLICIT: - reason = _("Explicitly installed"); - break; - case PM_PKG_REASON_DEPEND: - reason = _("Installed as a dependency for another package"); - break; - default: - reason = _("Unknown"); - break; + date = alpm_pkg_get_builddate(pkg); + if(date) { + strftime(bdatestr, 50, "%c", localtime(&date)); + } + if(level) { + date = alpm_pkg_get_installdate(pkg); + if(date) { + strftime(idatestr, 50, "%c", localtime(&date)); + } + switch((long)alpm_pkg_get_reason(pkg)) { + case PM_PKG_REASON_EXPLICIT: + reason = _("Explicitly installed"); + break; + case PM_PKG_REASON_DEPEND: + reason = _("Installed as a dependency for another package"); + break; + default: + reason = _("Unknown"); + break; + } + } else { + md5sum = alpm_pkg_get_md5sum(pkg); } - descheader = _("Description : ");
/* actual output */ printf(_("Name : %s\n"), (char *)alpm_pkg_get_name(pkg)); printf(_("Version : %s\n"), (char *)alpm_pkg_get_version(pkg)); - printf(_("URL : %s\n"), (char *)alpm_pkg_get_url(pkg)); - list_display(_("License :"), alpm_pkg_get_licenses(pkg)); + tmp = (char *)alpm_pkg_get_url(pkg); + if(*tmp) printf(_("URL : %s\n"), tmp);
Two things: 1. All the other variables were done above, so should this one. 2. I'm confused why we need this cast.
Some sync packages have URL field filled in, some others have not. In these cases alpm_pkg_get_url() points to "". So you get 'URL: ' printed, which is not nice. First I wanted change this to 'URL: unknown' but I didn't like that (why should we print useless infos?). tmp is used to aviod calling alpm_pkg_get_url twice (wow, what a speed-up ;-)
3. We don't do one line if statements anywhere in the code, so don't do it here. Oh, I see.
+ list_display(_("Licenses :"), alpm_pkg_get_licenses(pkg)); Good fix.
list_display(_("Groups :"), alpm_pkg_get_groups(pkg)); list_display(_("Provides :"), alpm_pkg_get_provides(pkg)); list_display(_("Depends On :"), alpm_pkg_get_depends(pkg)); list_display(_("Optional Deps :"), alpm_pkg_get_optdepends(pkg)); /* Only applicable if installed */ - if(level > 0) { + if(level) { I'd prefer to keep it explicit just because this isn't a binary value. I know this works, but no need to be cryptic.
list_display(_("Required By :"), alpm_pkg_get_requiredby(pkg)); } list_display(_("Conflicts With :"), alpm_pkg_get_conflicts(pkg)); list_display(_("Replaces :"), alpm_pkg_get_replaces(pkg)); - printf(_("Installed Size : %6.2f K\n"), (float)alpm_pkg_get_size(pkg) /
1024.0);
- printf(_("Packager : %s\n"), (char *)alpm_pkg_get_packager(pkg)); - printf(_("Architecture : %s\n"), (char *)alpm_pkg_get_arch(pkg)); - printf(_("Build Date : %s\n"), bdatestr); - if(level > 0) { - printf(_("Install Date : %s\n"), idatestr); + /* %SIZE%, %CSIZE%, %ISIZE% */ Yeah, it sucks that we have all sorts of brokenness here. Look at my patch for a cleaned up approach (you can always use get_isize for installed size). And do we show package size anywhere anymore? Some people might find that interesting and this would be the place to show it, although it only applies for non-installed packages.
+ if(level) { + unsigned long temp = alpm_pkg_get_size(pkg); + if(temp) printf(_("Installed Size : %6.2f K\n"), (float)temp / 1024.0); + } else { + unsigned long temp = alpm_pkg_get_size(pkg); + if(temp) printf(_("Download Size : %6.2f K\n"), (float)temp / 1024.0); + temp = alpm_pkg_get_isize(pkg); + if(temp) printf(_("Installed Size : %6.2f K\n"), (float)temp / 1024.0); + } + tmp = (char *)alpm_pkg_get_packager(pkg); + if(*tmp) printf(_("Packager : %s\n"), tmp); + tmp = (char *)alpm_pkg_get_arch(pkg); + if(*tmp) printf(_("Architecture : %s\n"), tmp); Once again, I do not understand why you need to remove the const pointer. This is not safe. I'd rather have 10 different variable names up above than these casts.
+ if(*bdatestr) printf(_("Build Date : %s\n"), bdatestr); + if(level) { + if(*idatestr) printf(_("Install Date : %s\n"), idatestr); printf(_("Install Reason : %s\n"), reason); If install date is NULL, then install reason shouldn't be printed either. And bdatestr/idatestr are never null- you declared it as such above.
+ printf(_("Install Script : %s\n"), + alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No")); Install script should be printed whether installed or not (although sync repos probably don't have this information, but queried package files do).
} - printf(_("Install Script : %s\n"), - alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No"));
/* printed using a variable to make i18n safe */ printf("%s", descheader); indentprint(alpm_pkg_get_desc(pkg), mbstowcs(NULL, descheader, 0)); printf("\n"); + + if(md5sum != NULL && md5sum[0] != '\0') { + printf(_("MD5 Sum : %s"), md5sum); + }
/* Print additional package info if info flag passed more than once */ if(level > 1) { @@ -115,35 +138,11 @@ void dump_pkg_full(pmpkg_t *pkg, int level) */ void dump_pkg_sync(pmpkg_t *pkg, const char *treename) { - const char *descheader, *md5sum; if(pkg == NULL) { return; } - - descheader = _("Description : "); - - md5sum = alpm_pkg_get_md5sum(pkg); - printf(_("Repository : %s\n"), treename); - printf(_("Name : %s\n"), (char *)alpm_pkg_get_name(pkg)); - printf(_("Version : %s\n"), (char *)alpm_pkg_get_version(pkg)); - list_display(_("Groups :"), alpm_pkg_get_groups(pkg)); - list_display(_("Provides :"), alpm_pkg_get_provides(pkg)); - list_display(_("Depends On :"), alpm_pkg_get_depends(pkg)); - list_display(_("Conflicts With :"), alpm_pkg_get_conflicts(pkg)); - list_display(_("Replaces :"), alpm_pkg_get_replaces(pkg)); - printf(_("Download Size : %6.2f K\n"), (float)alpm_pkg_get_size(pkg) / 1024.0); - printf(_("Installed Size : %6.2f K\n"), (float)alpm_pkg_get_isize(pkg) / 1024.0); - - /* printed using a variable to make i18n safe */ - printf("%s", descheader); - indentprint(alpm_pkg_get_desc(pkg), mbstowcs(NULL, descheader, 0)); - printf("\n"); - - if (md5sum != NULL && md5sum[0] != '\0') { - printf(_("MD5 Sum : %s"), md5sum); - } - printf("\n"); + dump_pkg_full(pkg, 0); }
/* Display list of backup files and their modification states diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 5f1072f..9b2aedc 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -295,7 +295,7 @@ static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) pmpkg_t *pkg = alpm_list_getdata(k);
if(strcmp(alpm_pkg_get_name(pkg), pkgstr) == 0) { - dump_pkg_sync(pkg, alpm_db_get_name(db)); + dump_pkg_sync(pkg, repo); printf("\n"); foundpkg = 1; break; @@ -332,9 +332,10 @@ static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) } else { for(i = syncs; i; i = alpm_list_next(i)) { pmdb_t *db = alpm_list_getdata(i); + const char *dbname = alpm_db_get_name(db);
for(j = alpm_db_getpkgcache(db); j; j = alpm_list_next(j)) { - dump_pkg_sync(alpm_list_getdata(j), alpm_db_get_name(db)); + dump_pkg_sync(alpm_list_getdata(j), dbname); printf("\n"); } } -- 1.5.3.5
A -Qip operation and -Si option may differ enough that it is worth keeping them separate and adding an additional level, but it is your code. :) Oops, I simply forgot about -Qip (to be honest, mentioning -Qp in the old comment misled me; I thought -Qp == -Q(0*i)p with level 0; and I thought that this is broken...) I didn't test it at all :-(. So I will rework this patch soon.
Bye, ngaba ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/
participants (3)
-
Aaron Griffin
-
Dan McGee
-
Nagy Gabor