[pacman-dev] [PATCH] Merge dump_pkg_sync to dump_pkg_full in pacman/package.c
Dan McGee
dpmcgee at gmail.com
Tue Nov 13 08:30:21 EST 2007
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 at 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 at 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
More information about the pacman-dev
mailing list