[pacman-dev] [PATCH] Merge dump_pkg_sync to dump_pkg_full in pacman/package.c
Nagy Gabor
ngaba at bibl.u-szeged.hu
Tue Nov 13 12:21:59 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.
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/
More information about the pacman-dev
mailing list