Dan and Xavier, thanks for the reviews. Sending the changes as replies + they are also up at https://github.com/schuay/pacman-arch/commits/working On 02/25/2011 03:30 PM, Dan McGee wrote:

On Mon, Feb 21, 2011 at 1:02 PM, Jakob Gruber wrote:

...

Signed-off-by: Jakob Gruber --- doc/pacman.8.txt | 43 ++++++++++++++++++++----------------------- 1 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt index 8a94ebc..c309425 100644 --- a/doc/pacman.8.txt +++ b/doc/pacman.8.txt @@ -173,15 +173,15 @@ Transaction Options (apply to '-S', '-R' and '-U') *-p, \--print*:: Only print the targets instead of performing the actual operation (sync, remove or upgrade). Use '\--print-format' to specify how targets are - displayed. The default format string is "%l", which displays url with - '-S', filename with '-U' and pkgname-pkgver with '-R'. + displayed. The default format string is "%l", which displays urls with + '-S', filenames with '-U' and pkgname-pkgver with '-R'. I prefer URLs as the canonical version of this word- is it broken in a lot of places? I think we use URL everywhere on a quick grep.

Fixed.

...

*\--print-format*<'format'>:: Specify a printf-like format to control the output of the '\--print' - operation. The possible are attributes are : %n for pkgname, %v for pkgver, + operation. The possible attributes are: %n for pkgname, %v for pkgver, %l for location, %r for repo and %s for size.

-Upgrade Options (apply to 'S' and 'U')[[UO]] +Upgrade Options (apply to '-S' and '-U')[[UO]] -------------------------------------------- *-f, \--force*:: Bypass file conflict checks and overwrite conflicting files. If the @@ -214,21 +214,20 @@ Upgrade Options (apply to 'S' and 'U')[[UO]] Query Options[[QO]] ------------------- *-c, \--changelog*:: - View the ChangeLog of a package. Not every package will provide one but - it will be shown if available. + View the ChangeLog of a package if it exists.

*-d, \--deps*:: Restrict or filter output to packages installed as dependencies. This - option can be combined with '-t' for listing real orphans- packages that + option can be combined with '-t' for listing real orphans - packages that were installed as dependencies but are no longer required by any installed package. ('-Qdt' is equivalent to the pacman 3.0.X '-Qe' option.)

*-e, \--explicit*:: - Restrict or filter output to packages explicitly installed. This option - can be combined with '-t' to list top-level packages- those packages - that were explicitly installed but are not required by any other - package. ('-Qet' is equivalent to the pacman 2.9.X '-Qe' option.) + Restrict or filter output to explicitly installed packages. This option + can be combined with '-t' to list explicitly installed packages which + are not required by any other package. ('-Qet' is equivalent to the + pacman 2.9.X '-Qe' option.) I'd say we can even dump the 2.9.X and 3.0.X notices here in these two messages- it has been a while and this will just confuse most users reading up on these options.

Right, done.

...

*-g, \--groups*:: Display all packages that are members of a named group. If a name is not @@ -255,8 +254,8 @@ Query Options[[QO]] and installed with '\--upgrade'.

*-o, \--owns*<'file'>:: - Search for the package that owns file. The path can be relative or - absolute. + Search for the package that owns the specified file. The path can be + relative or absolute. This is really file(s)- not sure if we can adjust things to be clearer about that. The text is easy, the first line opt description might be trickier.

I modified the text and left the oneliner as is.

...

*-p, \--file*:: Signifies that the package supplied on the command line is a file and @@ -274,7 +273,7 @@ Query Options[[QO]] rather than names and versions.

*-s, \--search*<'regexp'>:: - This will search each locally-installed package for names or + This will search each locally installed package for names or Kill the "this will" if you are changing this line anyway. And can you point at a grammar book that says why the hyphen should go away? It seems like this should have a hyphen to me.

Reverted to "locally-installed" and removed "This will".

...

descriptions that match `regexp`. When you include multiple search terms, only packages with descriptions matching ALL of those terms will be returned. @@ -298,9 +297,8 @@ Remove Options[[RO]] with care since it can remove many potentially needed packages.

*-n, \--nosave*:: - Instructs pacman to ignore file backup designations. Normally, when a - file is removed from the system the database is checked to see if the - file should be renamed with a ``.pacsave'' extension. + Delete backup files instead of renaming them with a ``.pacsave'' + extension.

I think you've oversimplified this. Backup files are not made if you never changed the file, and even though the original text here was perhaps a bit unwieldy, it hinted at this. Please confirm this behavior (look at some pactests?) and adjust the text accordingly.

Reverted this change.

...

*-s, \--recursive*:: Remove each target specified including all of their dependencies, provided @@ -310,7 +308,7 @@ Remove Options[[RO]] orphans. If you want to omit condition (B), pass this option twice.

*-u, \--unneeded*:: - Removes the targets that are not required by any other packages. + Removes targets that are not required by any other packages. This is mostly useful when removing a group without using the '-c' option, to avoid breaking any dependencies.

@@ -372,18 +370,17 @@ linkman:pacman.conf[5]. the same operation.

*-w, \--downloadonly*:: - Retrieve all packages from the server, but do not install/upgrade - anything. + Retrieve all packages from the server, but do not install/upgrade anything.

*-y, \--refresh*:: Download a fresh copy of the master package list from the server(s) defined in linkman:pacman.conf[5]. This should typically be used each time you use '\--sysupgrade' or '-u'. Passing two '\--refresh' or '-y' flags - will force a refresh of all package lists even if they are thought to be up + will force a refresh of all package lists even if they appear to be up to date.

*\--needed*:: - Don't reinstall the targets that are already up-to-date. + Don't reinstall the targets that are already up to date.

Handling Config Files[[HCF]] @@ -396,7 +393,7 @@ actual file existing on the filesystem. After comparing these 3 hashes, the follow scenarios can result:

original=X, current=X, new=X:: - All three files are the same, so overwrites are not an issue Install the + All three files are the same, so overwrites are not an issue. Install the new file.

original=X, current=X, new=Y:: -- 1.7.4.1

On 02/25/2011 03:51 PM, Dan McGee wrote:

On Mon, Feb 21, 2011 at 1:02 PM, Jakob Gruber wrote:

...

char *size_to_human_string(off_t bytes); char *size_to_human_string_short(off_t bytes);

Scale to the first unit with which the value will be less or equal to 2048. size_to_human_string_short uses short unit labels ("K","M",...) instead of the default ("KB", "MB"), uses only one decimal digit and has no space between the value and the label.

For example, size_to_human_string: "1.31 MB", short: "1.3M".

char *size_to_human_string_mb(off_t bytes); char *size_to_human_string_kb(off_t bytes);

Convert to MB and KB respectively and format identically to size_to_human_string.

Signed-off-by: Jakob Gruber --- src/pacman/callback.c | 46 ++++++------------------ src/pacman/package.c | 20 ++++++----- src/pacman/query.c | 7 ++-- src/pacman/sync.c | 7 ++-- src/pacman/util.c | 94 +++++++++++++++++++++++++++++++++++++++++-------- src/pacman/util.h | 4 ++ 6 files changed, 111 insertions(+), 67 deletions(-)

diff --git a/src/pacman/callback.c b/src/pacman/callback.c index f1e71bb..8ec4c3e 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -484,17 +484,16 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) { int infolen; int filenamelen; - char *fname, *p; + char *fname, *p, *ratestr, *xferedstr; /* used for wide character width determination and printing */ int len, wclen, wcwid, padwid; wchar_t *wcfname;

int totaldownload = 0; off_t xfered, total; - double rate = 0.0, timediff = 0.0, f_xfered = 0.0; + double rate = 0.0, timediff = 0.0; unsigned int eta_h = 0, eta_m = 0, eta_s = 0; int file_percent = 0, total_percent = 0; - char rate_size = 'K', xfered_size = 'K';

if(config->noprogressbar || file_total == -1) { if(file_xfered == 0) { @@ -556,7 +555,7 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) diff_sec = current_time.tv_sec - initial_time.tv_sec; diff_usec = current_time.tv_usec - initial_time.tv_usec; timediff = diff_sec + (diff_usec / 1000000.0); - rate = xfered / (timediff * 1024.0); + rate = xfered / timediff;

/* round elapsed time to the nearest second */ eta_s = (int)(timediff + 0.5); @@ -568,10 +567,10 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) /* return if the calling interval was too short */ return; } - rate = (xfered - xfered_last) / (timediff * 1024.0); + rate = (xfered - xfered_last) / timediff; /* average rate to reduce jumpiness */ rate = (rate + 2 * rate_last) / 3; - eta_s = (total - xfered) / (rate * 1024.0); + eta_s = (total - xfered) / (rate * 1024.0 * 1024.0); rate_last = rate; xfered_last = xfered; } @@ -625,38 +624,15 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total)

}

- /* Awesome formatting for progress bar. We need a mess of Kb->Mb->Gb stuff - * here. We'll use limit of 2048 for each until we get some empirical */ - /* rate_size = 'K'; was set above */ - if(rate> 2048.0) { - rate /= 1024.0; - rate_size = 'M'; - if(rate> 2048.0) { - rate /= 1024.0; - rate_size = 'G'; - /* we should not go higher than this for a few years (9999.9 Gb/s?)*/ - } - } - - f_xfered = xfered / 1024.0; /* convert to K by default */ - /* xfered_size = 'K'; was set above */ - if(f_xfered> 2048.0) { - f_xfered /= 1024.0; - xfered_size = 'M'; - if(f_xfered> 2048.0) { - f_xfered /= 1024.0; - xfered_size = 'G'; - /* I should seriously hope that archlinux packages never break - * the 9999.9GB mark... we'd have more serious problems than the progress - * bar in pacman */ - } - } + ratestr = size_to_human_string_short((off_t)rate); + xferedstr = size_to_human_string_short(xfered);

/* 1 space + filenamelen + 1 space + 7 for size + 1 + 7 for rate + 2 for /s + 1 space + 8 for eta */ - printf(" %ls%-*s %6.1f%c %#6.1f%c/s %02u:%02u:%02u", wcfname, - padwid, "", f_xfered, xfered_size, - rate, rate_size, eta_h, eta_m, eta_s); + printf(" %ls%-*s %7s %7s/s %02u:%02u:%02u", wcfname, + padwid, "", xferedstr, ratestr, eta_h, eta_m, eta_s);

+ free(ratestr); + free(xferedstr); free(fname); free(wcfname);

diff --git a/src/pacman/package.c b/src/pacman/package.c index 77a5ee7..156d257 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -50,7 +50,7 @@ void dump_pkg_full(pmpkg_t *pkg, int level) { const char *reason; time_t bdate, idate; - char bdatestr[50] = "", idatestr[50] = ""; + char bdatestr[50] = "", idatestr[50] = "", *size; I'd prefer these defined on different lines; yes they are the "same" type but it can be confusing. Understood, done.

...

const alpm_list_t *i; alpm_list_t *requiredby = NULL, *depstrings = NULL;

@@ -105,17 +105,19 @@ void dump_pkg_full(pmpkg_t *pkg, int level) } list_display(_("Conflicts With :"), alpm_pkg_get_conflicts(pkg)); list_display(_("Replaces :"), alpm_pkg_get_replaces(pkg)); + + size = size_to_human_string_kb(alpm_pkg_get_size(pkg)); if(level< 0) { - printf(_("Download Size : %6.2f K\n"), - (double)alpm_pkg_get_size(pkg) / 1024.0); - } - if(level == 0) { - printf(_("Compressed Size: %6.2f K\n"), - (double)alpm_pkg_get_size(pkg) / 1024.0); + printf(_("Download Size : %s\n"), size); + } else if(level == 0) { + printf(_("Compressed Size: %s\n"), size); } + free(size); + + size = size_to_human_string_kb(alpm_pkg_get_isize(pkg)); + printf(_("Installed Size : %s\n"), size); + free(size);

- printf(_("Installed Size : %6.2f K\n"), - (double)alpm_pkg_get_isize(pkg) / 1024.0); string_display(_("Packager :"), alpm_pkg_get_packager(pkg)); string_display(_("Architecture :"), alpm_pkg_get_arch(pkg)); string_display(_("Build Date :"), bdatestr); diff --git a/src/pacman/query.c b/src/pacman/query.c index 734875b..8a66927 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -247,10 +247,9 @@ static int query_search(alpm_list_t *targets)

/* print the package size with the output if ShowSize option set */ if(!config->quiet&& config->showsize) { - /* Convert byte size to MB */ - double mbsize = (double)alpm_pkg_get_size(pkg) / (1024.0 * 1024.0); - - printf(" [%.2f MB]", mbsize); + char *size = size_to_human_string_mb(alpm_pkg_get_size(pkg)); + printf(" [%s]", size); + free(size); }

diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 7af1667..21fe4ba 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -349,10 +349,9 @@ static int sync_search(alpm_list_t *syncs, alpm_list_t *targets)

/* print the package size with the output if ShowSize option set */ if(!config->quiet&& config->showsize) { - /* Convert byte size to MB */ - double mbsize = (double)alpm_pkg_get_size(pkg) / (1024.0 * 1024.0); - - printf(" [%.2f MB]", mbsize); + char *size = size_to_human_string_mb(alpm_pkg_get_size(pkg)); + printf(" [%s]", size); + free(size); }

if (!config->quiet) { diff --git a/src/pacman/util.c b/src/pacman/util.c index c08ebb1..b651478 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -501,10 +501,9 @@ void list_display_linebreak(const char *title, const alpm_list_t *list) /* prepare a list of pkgs to display */ void display_targets(const alpm_list_t *pkgs, int install) { - char *str; + char *str, *size; const alpm_list_t *i; off_t isize = 0, dlsize = 0; - double mbisize = 0.0, mbdlsize = 0.0; alpm_list_t *targets = NULL;

if(!pkgs) { @@ -522,10 +521,10 @@ void display_targets(const alpm_list_t *pkgs, int install)

/* print the package size with the output if ShowSize option set */ if(config->showsize) { - double mbsize = (double)alpm_pkg_get_size(pkg) / (1024.0 * 1024.0); - - pm_asprintf(&str, "%s-%s [%.2f MB]", alpm_pkg_get_name(pkg), - alpm_pkg_get_version(pkg), mbsize); + size = size_to_human_string_mb(alpm_pkg_get_size(pkg)); + pm_asprintf(&str, "%s-%s [%s]", alpm_pkg_get_name(pkg), + alpm_pkg_get_version(pkg), size); + free(size); } else { pm_asprintf(&str, "%s-%s", alpm_pkg_get_name(pkg), alpm_pkg_get_version(pkg)); @@ -533,19 +532,19 @@ void display_targets(const alpm_list_t *pkgs, int install) targets = alpm_list_add(targets, str); }

- /* Convert byte sizes to MB */ - mbdlsize = (double)dlsize / (1024.0 * 1024.0); - mbisize = (double)isize / (1024.0 * 1024.0); - if(install) { pm_asprintf(&str, _("Targets (%d):"), alpm_list_count(targets)); list_display(str, targets); free(str); printf("\n");

- printf(_("Total Download Size: %.2f MB\n"), mbdlsize); + size = size_to_human_string_mb(dlsize); + printf(_("Total Download Size: %s\n"), size); + free(size); if(!(config->flags& PM_TRANS_FLAG_DOWNLOADONLY)) { - printf(_("Total Installed Size: %.2f MB\n"), mbisize); + size = size_to_human_string_mb(isize); + printf(_("Total Installed Size: %s\n"), size); + free(size); } } else { pm_asprintf(&str, _("Remove (%d):"), alpm_list_count(targets)); @@ -553,7 +552,9 @@ void display_targets(const alpm_list_t *pkgs, int install) free(str); printf("\n");

- printf(_("Total Removed Size: %.2f MB\n"), mbisize); + size = size_to_human_string_mb(isize); + printf(_("Total Removed Size: %s\n"), size); + free(size);

I see no reason we should lock any of these to MB units- just use the general formatter here.

I tried not locking the size summaries, and as a user I think I'd prefer keeping them as they are. Compare the following: Total Download Size: 884.34 KB Total Installed Size: 4.80 MB vs Total Download Size: 0.86 MB Total Installed Size: 4.80 MB In the latter, it seems clearer that "Installed" is larger than "Download". Here's what it looks like with summaries locked to MB and ShowSize using the general formatter: Remove (1): libjpeg-8.3.0-1 [848.00 KB] Total Removed Size: 0.83 MB Targets (5): libjpeg-turbo-1.1.0-1 [209.01 KB] linux-firmware-20110227-1 [8.23 MB] ncurses-5.8-1 [945.95 KB] ppl-0.11.2-1 [2.74 MB] v4l-utils-0.8.3-1 [232.80 KB] Total Download Size: 12.32 MB Total Installed Size: 58.82 MB What do you think?

...

}

FREELIST(targets); @@ -594,6 +595,70 @@ static char *pkg_get_location(pmpkg_t *pkg) } }

+/** Converts sizes in bytes into strings. + * + * @param bytes the size in bytes + * @param format the format string to use. The first argument is of type + * float and represents the size. The second argument is of type + * char* and represents the unit label. for example, "%.2f %s" + * could result in "1.23 MB" + * @param unit the target unit. can be either one of the short unit + * labels ("B", "K", ...); or NULL, in which case the first + * unit which will bring the value to below a threshold of 2048 + * will be chosen. + * @param long_units whether to use short ("K") or long ("KB") unit labels + * + * @return the size string + */ +static char *size_to_human_string_format(off_t bytes, const char *format, + const char *unit, int long_labels) +{ + char *ret; + static const char *shortlabels[] = {"B", "K", "M", "G", "T", "P"}; + static const char *longlabels[] = {"B", "KB", "MB", "GB", "TB", "PB"}; + const char **labels = long_labels ? longlabels : shortlabels; + const int unitcount = sizeof(shortlabels) / sizeof(shortlabels[0]); + int index; + float val = (float)bytes; + + /* stop conditions: + * 1) a target unit is specified and we are at target unit, + * 2) target unit is NOT specified and value is leq 2048, + * and 3) we are already at the largest unit.s + * note that if an invalid target is specified in 1), + * we convert to the largest unit */ + for(index = 0; index< unitcount - 1; index++) { + if(unit != NULL&& strcmp(shortlabels[index], unit) == 0) break;

I'd make the function param a char (not a "string") to ensure people aren't doing stupid things like passing a long label in by mistake. Then you can just use == here.

Done.

...

+ else if(unit == NULL&& val<= 2048.0) break; + + val /= 1024.0; + } + + pm_asprintf(&ret, format, val, labels[index]); + + return ret; +} + +char *size_to_human_string(off_t bytes) +{ + return size_to_human_string_format(bytes, "%.2f %s", NULL, 1); +} + +char *size_to_human_string_mb(off_t bytes) +{ + return size_to_human_string_format(bytes, "%.2f %s", "M", 1); +} + +char *size_to_human_string_kb(off_t bytes) +{ + return size_to_human_string_format(bytes, "%.2f %s", "K", 1); +} + +char *size_to_human_string_short(off_t bytes) +{ + return size_to_human_string_format(bytes, "%.1f%s", NULL, 0); +} + I think this API has been over-engineered with all these one-liners. We aren't exposing this as a library function or anything, so people can deal with a little complexity in the call themselves. I'd have one function for all of this, and we can then even skip the malloc/free cycle: double value humanize_size(off_t bytes, char max_unit, int long_labels, const char **label)

Logic stays as is in your format function, expect we don't actually print the string- the value is returned and label, if provided, gets a pointer to the correct string. Does this make sense? I'm open for feedback of course, but I see this as 1) less boilerplate methods in util.c 2) no free() necessary by caller.

Sure, makes sense. I changed the function as you described, but renamed max_unit to target_unit (because that's what it actually is).

...

void print_packages(const alpm_list_t *packages) { const alpm_list_t *i; @@ -638,8 +703,7 @@ void print_packages(const alpm_list_t *packages) /* %s : size */ if(strstr(temp,"%s")) { char *size; - double mbsize = (double)pkg_get_size(pkg) / (1024.0 * 1024.0); - pm_asprintf(&size, "%.2f", mbsize); + size = size_to_human_string_format(pkg_get_size(pkg), "%.2f", "M", 0); string = strreplace(temp, "%s", size); free(size); free(temp); diff --git a/src/pacman/util.h b/src/pacman/util.h index 234a631..ed03c13 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -51,6 +51,10 @@ char *strtoupper(char *str); char *strtrim(char *str); char *strreplace(const char *str, const char *needle, const char *replace); alpm_list_t *strsplit(const char *str, const char splitchar); +char *size_to_human_string(off_t bytes); +char *size_to_human_string_mb(off_t bytes); +char *size_to_human_string_kb(off_t bytes); +char *size_to_human_string_short(off_t bytes); void string_display(const char *title, const char *string); void list_display(const char *title, const alpm_list_t *list); void list_display_linebreak(const char *title, const alpm_list_t *list); -- 1.7.4.1

Printing the exact size seems to make more sense for scripting contexts. Signed-off-by: Jakob Gruber --- src/pacman/util.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/pacman/util.c b/src/pacman/util.c index ba93963..23b95c9 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -676,7 +676,7 @@ void print_packages(const alpm_list_t *packages) /* %s : size */ if(strstr(temp,"%s")) { char *size; - pm_asprintf(&size, "%.2f", humanize_size(pkg_get_size(pkg), 'M', 0, NULL)); + pm_asprintf(&size, "%u", pkg_get_size(pkg)); string = strreplace(temp, "%s", size); free(size); free(temp); -- 1.7.4.1

Row handling is moved to its own function in preparation for verbose package lists.

Signed-off-by: Jakob Gruber ---  src/pacman/util.c |   53 ++++++++++++++++++++++++++++-------------------------  1 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/src/pacman/util.c b/src/pacman/util.c index 8903d61..85d2d8c 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -498,10 +498,26 @@ void list_display_linebreak(const char *title, const alpm_list_t *list)                }        }  } + +/* returns package info as a string */ +static char *create_list_element(pmpkg_t *pkg) +{ +       char *ret, *size; +       const char *pkgstr = config->showsize ? "%s-%s [%s]" : "%s-%s"; + +       size = size_to_human_string_mb(alpm_pkg_get_size(pkg)); +       pm_asprintf(&ret, pkgstr, alpm_pkg_get_name(pkg), alpm_pkg_get_version(pkg), +                       size); +       free(size); + +       return ret; return() +} +  /* prepare a list of pkgs to display */  void display_targets(const alpm_list_t *pkgs, int install)  {        char *str, *size; +       const char *title;        const alpm_list_t *i;        off_t isize = 0, dlsize = 0;        alpm_list_t *targets = NULL; @@ -510,34 +526,24 @@ void display_targets(const alpm_list_t *pkgs, int install)                return;        }

-       printf("\n"); +       title = install ? _("Targets (%d):") : _("Remove (%d):"); +        for(i = pkgs; i; i = alpm_list_next(i)) {                pmpkg_t *pkg = alpm_list_getdata(i);

-               if(install) { -                       dlsize += alpm_pkg_download_size(pkg); -               } +               dlsize += alpm_pkg_download_size(pkg); So we're now adding this up for both install and remove? We should

On Mon, Feb 21, 2011 at 1:02 PM, Jakob Gruber wrote: probably keep it conditional.

               isize += alpm_pkg_get_isize(pkg);

-               /* print the package size with the output if ShowSize option set */ -               if(config->showsize) { -                       size = size_to_human_string_mb(alpm_pkg_get_size(pkg)); -                       pm_asprintf(&str, "%s-%s [%s]", alpm_pkg_get_name(pkg), -                                       alpm_pkg_get_version(pkg), size); -                       free(size); -               } else { -                       pm_asprintf(&str, "%s-%s", alpm_pkg_get_name(pkg), -                                       alpm_pkg_get_version(pkg)); -               } -               targets = alpm_list_add(targets, str); +               targets = alpm_list_add(targets, create_list_element(pkg));        }

-       if(install) { -               pm_asprintf(&str, _("Targets (%d):"), alpm_list_count(targets)); -               list_display(str, targets); -               free(str); -               printf("\n"); +       pm_asprintf(&str, title, alpm_list_count(targets)); + +       printf("\n"); +       list_display(str, targets); +       printf("\n");

+       if(install) {                size = size_to_human_string_mb(dlsize);                printf(_("Total Download Size:    %s\n"), size);                free(size); @@ -547,16 +553,13 @@ void display_targets(const alpm_list_t *pkgs, int install)                        free(size);                }        } else { -               pm_asprintf(&str, _("Remove (%d):"), alpm_list_count(targets)); -               list_display(str, targets); -               free(str); -               printf("\n"); -                size = size_to_human_string_mb(isize);                printf(_("Total Removed Size:   %s\n"), size);                free(size);        }

+out: +       free(str);        FREELIST(targets);  }

-- 1.7.4.1

Row handling is moved to its own function in preparation for verbose package lists. Signed-off-by: Jakob Gruber --- src/pacman/util.c | 48 ++++++++++++++++++++++++++---------------------- 1 files changed, 26 insertions(+), 22 deletions(-) diff --git a/src/pacman/util.c b/src/pacman/util.c index 23b95c9..ba64a77 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -498,11 +498,27 @@ void list_display_linebreak(const char *title, const alpm_list_t *list) } } } + +/* returns package info as a string */ +static char *create_list_element(pmpkg_t *pkg) +{ + char *ret; + const char *label; + double size; + const char *pkgstr = config->showsize ? "%s-%s [%.2f %s]" : "%s-%s"; + + size = humanize_size(alpm_pkg_get_size(pkg), 0, 1, &label); + pm_asprintf(&ret, pkgstr, alpm_pkg_get_name(pkg), alpm_pkg_get_version(pkg), + size, label); + + return(ret); +} + /* prepare a list of pkgs to display */ void display_targets(const alpm_list_t *pkgs, int install) { char *str; - const char *label; + const char *title, *label; double size; const alpm_list_t *i; off_t isize = 0, dlsize = 0; @@ -512,7 +528,6 @@ void display_targets(const alpm_list_t *pkgs, int install) return; } - printf("\n"); for(i = pkgs; i; i = alpm_list_next(i)) { pmpkg_t *pkg = alpm_list_getdata(i); @@ -521,24 +536,17 @@ void display_targets(const alpm_list_t *pkgs, int install) } isize += alpm_pkg_get_isize(pkg); - /* print the package size with the output if ShowSize option set */ - if(config->showsize) { - size = humanize_size(alpm_pkg_get_size(pkg), 0, 1, &label); - pm_asprintf(&str, "%s-%s [%.2f %s]", alpm_pkg_get_name(pkg), - alpm_pkg_get_version(pkg), size, label); - } else { - pm_asprintf(&str, "%s-%s", alpm_pkg_get_name(pkg), - alpm_pkg_get_version(pkg)); - } - targets = alpm_list_add(targets, str); + targets = alpm_list_add(targets, create_list_element(pkg)); } - if(install) { - pm_asprintf(&str, _("Targets (%d):"), alpm_list_count(targets)); - list_display(str, targets); - free(str); - printf("\n"); + title = install ? _("Targets (%d):") : _("Remove (%d):"); + pm_asprintf(&str, title, alpm_list_count(pkgs)); + + printf("\n"); + list_display(str, targets); + printf("\n"); + if(install) { size = humanize_size(dlsize, 'M', 1, &label); printf(_("Total Download Size: %.2f %s\n"), size, label); if(!(config->flags & PM_TRANS_FLAG_DOWNLOADONLY)) { @@ -546,15 +554,11 @@ void display_targets(const alpm_list_t *pkgs, int install) printf(_("Total Installed Size: %.2f %s\n"), size, label); } } else { - pm_asprintf(&str, _("Remove (%d):"), alpm_list_count(targets)); - list_display(str, targets); - free(str); - printf("\n"); - size = humanize_size(isize, 'M', 1, &label); printf(_("Total Removed Size: %.2f %s\n"), size, label); } + free(str); FREELIST(targets); } -- 1.7.4.1

On Mon, Feb 21, 2011 at 1:02 PM, Jakob Gruber wrote:

table_display takes a list of lists of strings (representing the table cells) and displays them formatted as a table. The exact format depends on the longest string in each column.

Signed-off-by: Jakob Gruber ---  src/pacman/util.c |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++  src/pacman/util.h |    1 +  2 files changed, 109 insertions(+), 0 deletions(-)

Noticing it now, but please read the coding style document. We always use {}, even on one line conditionals. We also format all return statements as return(value). Yes, this one is suspect, but consistency is more important than anything.

diff --git a/src/pacman/util.c b/src/pacman/util.c index 85d2d8c..8f7b5e5 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -435,6 +435,114 @@ void string_display(const char *title, const char *string)        printf("\n");  }

+static void table_print_line(const alpm_list_t *line, +                                                                                                                       const alpm_list_t *formats) +{ +       const alpm_list_t *curformat = formats; +       const alpm_list_t *curcell = line; + +       while (curcell && curformat) { +               printf(alpm_list_getdata(curformat), alpm_list_getdata(curcell)); +               curcell = alpm_list_next(curcell); +               curformat = alpm_list_next(curformat); +       } + +       printf("\n"); +} + +/* creates a format string by checking max cell lengths in rows */ +static alpm_list_t *table_create_format(const alpm_list_t *rows) +{ +       alpm_list_t *format, *curformat; +       const alpm_list_t *currow, *curcell; +       char *str, *formatstr; +       const int padding = 2; +       int cols, neededcols = 0; + +       /* initial format list element */ +       format = alpm_list_add(NULL, NULL);

What's going on here? We always have a NULL first item?

+ +       /* go through each cell and store the longest string for each +        * column in the format list */ +       currow = rows; +       while(currow != NULL) { You've wrote a for() loop without the for here and the last line of the block. + +               /* starting on new row, reset format pointer to first column */ +               curformat = format; + +               /* loop through all row cells and update format pointer if necessary */ Which isn't really a format pointer at all, but a pointer to actual data?

+               curcell = alpm_list_getdata(currow); +               while(curcell != NULL) { +                       if(curformat == NULL) { +                               curformat = alpm_list_last(alpm_list_add(format, NULL)); This makes assumptions. I doubt we'll ever break it, but alpm_list_add always needs to be reassigned back to format, in this case, before doing anything else on it.

+                       } + +                       str = alpm_list_getdata(curcell); +                       if(curformat->data == NULL || +                                       strlen(str) > strlen(alpm_list_getdata(curformat))) +                               curformat->data = str; + +                       curformat = alpm_list_next(curformat); +                       curcell = alpm_list_next(curcell); +               } + +               currow = alpm_list_next(currow); +       } + +       /* now use the column width info to generate format strings */ +       curformat = format; +       while(curformat != NULL) { +               cols = strlen(curformat->data) + padding; +               neededcols += cols; + +               str = (alpm_list_next(curformat) != NULL) ? "%%-%ds" : "%%%ds"; Magic here- everything else seems generic, but why is this last column special?

+               pm_asprintf(&formatstr, str, cols); + +               curformat->data = formatstr; +               curformat = alpm_list_next(curformat); +       } I had a real hard time following all this, but that might just be the morning brain not working well. Your reuse of curformat->data as first a immutable string holder and later a alloced format string holder is particularly hard to wrap the brain around until you see it. I'd highly recommend not mixing the two for clarity.

+ +       /* return NULL if terminal is not wide enough */ +       if(neededcols > getcols()) { +               FREELIST(format); +               return NULL; +       } Hmm. Is this likely to happen? Seems odd to then not have anything

input: a list of rows, each with a list of cols (cells)? output: a list of formats for each col? printed at all. I think we should truncate strings as necessary, just like we do in the callback functions.

+ +       return format; +} + +/** Displays the list in table format + * + * @param title the tables title + * @param rows  the rows to display as a list of lists of strings. the outer + *                                                     list represents the rows, the inner list the cells (= columns) + * + * @return -1 if not enough terminal cols available, else 0 + */ +int table_display(const char *title, const alpm_list_t *rows) +{ +       alpm_list_t *i, *formats; + +       if(rows == NULL) +               return 0; + +       formats = table_create_format(rows); +       if(formats == NULL) { +               return -1; +       } + +       if(title != NULL) { +               printf("%s\n\n", title); +       } + +       for(i = alpm_list_first(rows); i; i = alpm_list_next(i)) { +               table_print_line(alpm_list_getdata(i), formats); +       } + +       FREELIST(formats); +       return 0; +} +  void list_display(const char *title, const alpm_list_t *list)  {        const alpm_list_t *i; diff --git a/src/pacman/util.h b/src/pacman/util.h index ed03c13..494becf 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -56,6 +56,7 @@ char *size_to_human_string_mb(off_t bytes);  char *size_to_human_string_kb(off_t bytes);  char *size_to_human_string_short(off_t bytes);  void string_display(const char *title, const char *string); +int table_display(const char *title, const alpm_list_t *rows);  void list_display(const char *title, const alpm_list_t *list);  void list_display_linebreak(const char *title, const alpm_list_t *list);  void display_targets(const alpm_list_t *pkgs, int install); -- 1.7.4.1

table_display takes a list of lists of strings (representing the table cells) and displays them formatted as a table. The exact format depends on the longest string in each column. Signed-off-by: Jakob Gruber --- src/pacman/util.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/pacman/util.h | 1 + 2 files changed, 111 insertions(+), 0 deletions(-) diff --git a/src/pacman/util.c b/src/pacman/util.c index ba64a77..65656ff 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -435,6 +435,116 @@ void string_display(const char *title, const char *string) printf("\n"); } +static void table_print_line(const alpm_list_t *line, + const alpm_list_t *formats) +{ + const alpm_list_t *curformat = formats; + const alpm_list_t *curcell = line; + + while(curcell && curformat) { + printf(alpm_list_getdata(curformat), alpm_list_getdata(curcell)); + curcell = alpm_list_next(curcell); + curformat = alpm_list_next(curformat); + } + + printf("\n"); +} + +/* creates format strings by checking max cell lengths in cols */ +static alpm_list_t *table_create_format(const alpm_list_t *header, + const alpm_list_t *rows) +{ + alpm_list_t *longest_str, *longest_strs = NULL; + alpm_list_t *formats = NULL; + const alpm_list_t *i, *row, *cell; + char *str, *formatstr; + const int padding = 2; + int colwidth, totalwidth = 0; + int curcol = 0; + + /* header determines column count and initial values of longest_strs */ + for(i = header; i; i = alpm_list_next(i)) { + longest_strs = alpm_list_add(longest_strs, alpm_list_getdata(i)); + } + + /* now find the longest string in each column */ + for(longest_str = longest_strs; longest_str; + longest_str = alpm_list_next(longest_str), curcol++) { + for(i = rows; i; i = alpm_list_next(i)) { + row = alpm_list_getdata(i); + cell = alpm_list_nth(row, curcol); + str = alpm_list_getdata(cell); + + if(strlen(str) > strlen(alpm_list_getdata(longest_str))) { + longest_str->data = str; + } + } + } + + /* now use the column width info to generate format strings */ + for(i = longest_strs; i; i = alpm_list_next(i)) { + colwidth = strlen(alpm_list_getdata(i)) + padding; + totalwidth += colwidth; + + /* right align the last column for a cleaner table display */ + str = (alpm_list_next(i) != NULL) ? "%%-%ds" : "%%%ds"; + pm_asprintf(&formatstr, str, colwidth); + + formats = alpm_list_add(formats, formatstr); + } + + alpm_list_free(longest_strs); + + /* return NULL if terminal is not wide enough */ + if(totalwidth > getcols()) { + fprintf(stderr, _("insufficient columns available for table display\n")); + FREELIST(formats); + return(NULL); + } + + return(formats); +} + +/** Displays the list in table format + * + * @param title the tables title + * @param header the column headers. column count is determined by the nr + * of headers + * @param rows the rows to display as a list of lists of strings. the outer + * list represents the rows, the inner list the cells (= columns) + * + * @return -1 if not enough terminal cols available, else 0 + */ +int table_display(const char *title, const alpm_list_t *header, + const alpm_list_t *rows) +{ + const alpm_list_t *i; + alpm_list_t *formats; + + if(rows == NULL || header == NULL) { + return(0); + } + + formats = table_create_format(header, rows); + if(formats == NULL) { + return(-1); + } + + if(title != NULL) { + printf("%s\n\n", title); + } + + table_print_line(header, formats); + printf("\n"); + + for(i = rows; i; i = alpm_list_next(i)) { + table_print_line(alpm_list_getdata(i), formats); + } + + FREELIST(formats); + return(0); +} + void list_display(const char *title, const alpm_list_t *list) { const alpm_list_t *i; diff --git a/src/pacman/util.h b/src/pacman/util.h index 6a20ba4..d8ae7d8 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -53,6 +53,7 @@ char *strreplace(const char *str, const char *needle, const char *replace); alpm_list_t *strsplit(const char *str, const char splitchar); void string_display(const char *title, const char *string); double humanize_size(off_t bytes, const char target_unit, int long_labels, const char **label); +int table_display(const char *title, const alpm_list_t *header, const alpm_list_t *rows); void list_display(const char *title, const alpm_list_t *list); void list_display_linebreak(const char *title, const alpm_list_t *list); void display_targets(const alpm_list_t *pkgs, int install); -- 1.7.4.1

On Mon, Feb 21, 2011 at 1:02 PM, Jakob Gruber wrote:

If enabled, displays package lists for upgrade, sync and remove operations formatted as a table. Falls back to default list display if insufficient terminal columns are available.

Example output (-Su):

Targets (25):

Pkg Name          New Version           Old Version          Size

asciidoc          8.6.4-1               8.6.3-1           0.15 MB chromium          9.0.597.94-2          9.0.597.94-1     17.80 MB ... wine              1.3.14-1              1.3.13-2         24.67 MB

Total Download Size:    158.41 MB Total Installed Size:   693.05 MB

Example output (-S, some targets already installed):

kdeedu-kturtle     4.6.0-1                      0.22 MB kdeedu-kwordquiz   4.6.0-1                      1.06 MB kdeedu-marble      4.6.0-1      4.6.0-1        14.95 MB

Example output (-R):

Remove (15):

Pkg Name                 Old Version       Size

kdeutils-sweeper         4.6.0-1        0.12 MB kdeutils-superkaramba    4.6.0-1        1.08 MB kdeutils-printer-applet  4.6.0-1        0.16 MB kdeutils-kwallet         4.6.0-1        0.81 MB

Probably just omitted but I assume "Total Removed Size" is there somewhere?

Signed-off-by: Jakob Gruber ---  doc/pacman.conf.5.txt |    5 +++  etc/pacman.conf.in    |    1 +  src/pacman/conf.h     |    1 +  src/pacman/pacman.c   |    3 ++  src/pacman/util.c     |   95 ++++++++++++++++++++++++++++++++++++++++++++++--  5 files changed, 101 insertions(+), 4 deletions(-)

diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt index cb4c589..063d305 100644 --- a/doc/pacman.conf.5.txt +++ b/doc/pacman.conf.5.txt @@ -168,6 +168,11 @@ Options        Performs an approximate check for adequate available disk space before        installing packages.

+*VerbosePkgLists*:: +       Displays name, version and size of target packages formatted +       as a table. during which operations? You explained it in the commit message just not here.

+ +  Repository Sections  -------------------  Each repository section defines a section name and at least one location where diff --git a/etc/pacman.conf.in b/etc/pacman.conf.in index 1105db9..bf5925f 100644 --- a/etc/pacman.conf.in +++ b/etc/pacman.conf.in @@ -34,6 +34,7 @@ Architecture = auto  #UseDelta  #TotalDownload  #CheckSpace +#VerbosePkgLists

 #  # REPOSITORIES diff --git a/src/pacman/conf.h b/src/pacman/conf.h index 92c379f..f440090 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -70,6 +70,7 @@ typedef struct __config_t {        /* conf file options */        unsigned short chomp; /* I Love Candy! */        unsigned short showsize; /* show individual package sizes */ +       unsigned short verbosepkglists; /* format target pkg lists as table */ Part of me is tempted to say we should kill showsize now. I know it shows up in -Qs and -Ss, but is that even useful? Opinions?

       /* When downloading, display the amount downloaded, rate, ETA, and percent         * downloaded of the total download list */        unsigned short totaldownload; diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 984bd1b..4cf14bb 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -967,6 +967,9 @@ static int _parse_options(char *key, char *value)                } else if(strcmp(key, "ShowSize") == 0) {                        config->showsize = 1;                        pm_printf(PM_LOG_DEBUG, "config: showsize\n"); +               } else if(strcmp(key, "VerbosePkgLists") == 0) { +                       config->verbosepkglists = 1; +                       pm_printf(PM_LOG_DEBUG, "config: verbosepkglists\n");                } else if(strcmp(key, "UseDelta") == 0) {                        alpm_option_set_usedelta(1);                        pm_printf(PM_LOG_DEBUG, "config: usedelta\n"); diff --git a/src/pacman/util.c b/src/pacman/util.c index 8f7b5e5..71100dd 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -607,6 +607,62 @@ void list_display_linebreak(const char *title, const alpm_list_t *list)        }  }

+/* creates a header row for use with table_display */ +static alpm_list_t *create_verbose_header(int install) +{ +       alpm_list_t *res = NULL; +       char *str; + +       pm_asprintf(&str, "%s", _("Pkg Name")); Are we that constrained for size here? "Package Name" would be good; remember also that these are translated strings and other locales could very well have longer strings. +       res = alpm_list_add(res, str); +       if(install) { +               pm_asprintf(&str, "%s", _("New Version")); +               res = alpm_list_add(res, str); +       } +       pm_asprintf(&str, "%s", _("Old Version")); +       res = alpm_list_add(res, str); +       pm_asprintf(&str, "%s", _("Size")); +       res = alpm_list_add(res, str); + +       return res; +} So your magic NULL, NULL first row is now slightly making sense from the other patch, but I think you are still not accounting for header size > max row sizes?

+ +/** Generates a table row with package info * + * + * @param pkg  the package + * @param install      if value is zero format for remove operation, + *                                                             otherwise for installation + * + * @return a list of strings representing the current row + */ +static alpm_list_t *create_verbose_row(pmpkg_t *pkg, int install) +{ +       char *str; +       alpm_list_t *ret = NULL; +       pmdb_t *ldb = alpm_option_get_localdb(); + +       /* a row consists of the package name, */ +       pm_asprintf(&str,"%s", alpm_pkg_get_name(pkg)); +       ret = alpm_list_add(ret, str); + +       /* (new version,) old version */ +       pm_asprintf(&str,"%s", alpm_pkg_get_version(pkg)); +       ret = alpm_list_add(ret, str); + +       if(install) { +               pmpkg_t *oldpkg = alpm_db_get_pkg(ldb, alpm_pkg_get_name(pkg)); +               pm_asprintf(&str, "%s", +                               oldpkg != NULL ? alpm_pkg_get_version(oldpkg) : ""); +               ret = alpm_list_add(ret, str); +       } + +       /* and size */ +       str = size_to_human_string_mb(alpm_pkg_get_size(pkg)); +       ret = alpm_list_add(ret, str); + +       return ret; +} +  /* returns package info as a string */  static char *create_list_element(pmpkg_t *pkg)  { @@ -628,7 +684,7 @@ void display_targets(const alpm_list_t *pkgs, int install)        const char *title;        const alpm_list_t *i;        off_t isize = 0, dlsize = 0; -       alpm_list_t *targets = NULL; +       alpm_list_t *j, *lp, *targets = NULL;

       if(!pkgs) {                return; @@ -636,19 +692,40 @@ void display_targets(const alpm_list_t *pkgs, int install)

       title = install ? _("Targets (%d):") : _("Remove (%d):");

+       if(config->verbosepkglists) { +               /* create table header and add an empty line after it */ +               targets = alpm_list_add(targets, create_verbose_header(install)); +               targets = alpm_list_add(targets, NULL); +       } + +       /* gather pkg infos */        for(i = pkgs; i; i = alpm_list_next(i)) {                pmpkg_t *pkg = alpm_list_getdata(i);

               dlsize += alpm_pkg_download_size(pkg);                isize += alpm_pkg_get_isize(pkg);

-               targets = alpm_list_add(targets, create_list_element(pkg)); +               if(config->verbosepkglists) { +                       targets = alpm_list_add(targets, create_verbose_row(pkg, install)); +               } else { +                       targets = alpm_list_add(targets, create_list_element(pkg)); +               }        }

+       /* print to screen */        pm_asprintf(&str, title, alpm_list_count(targets));

       printf("\n"); -       list_display(str, targets); +       if(config->verbosepkglists) { +               if(table_display(str, targets) != 0) { +                       printf(_("Insufficient columns available, using default list display...\n")); +                       config->verbosepkglists = 0; +                       display_targets(pkgs, install); +                       goto out; +               } +       } else { +               list_display(str, targets); +       }        printf("\n");

       if(install) { @@ -667,8 +744,18 @@ void display_targets(const alpm_list_t *pkgs, int install)        }

 out: +       /* cleanup */ +       if(config->verbosepkglists) { +               /* targets is a list of lists of strings, free inner lists here */ +               for(j = alpm_list_first(targets); j; j = alpm_list_next(j)) { +                       lp = alpm_list_getdata(j); +                       FREELIST(lp); +               } +               alpm_list_free(targets); +       } else { +               FREELIST(targets); +       }        free(str); -       FREELIST(targets);  }

 static off_t pkg_get_size(pmpkg_t *pkg) -- 1.7.4.1

On 02/25/2011 04:30 PM, Dan McGee wrote:

On Mon, Feb 21, 2011 at 1:02 PM, Jakob Gruber wrote:

...

If enabled, displays package lists for upgrade, sync and remove operations formatted as a table. Falls back to default list display if insufficient terminal columns are available.

Example output (-Su):

Targets (25):

Pkg Name New Version Old Version Size

asciidoc 8.6.4-1 8.6.3-1 0.15 MB chromium 9.0.597.94-2 9.0.597.94-1 17.80 MB ... wine 1.3.14-1 1.3.13-2 24.67 MB

Total Download Size: 158.41 MB Total Installed Size: 693.05 MB

Example output (-S, some targets already installed):

kdeedu-kturtle 4.6.0-1 0.22 MB kdeedu-kwordquiz 4.6.0-1 1.06 MB kdeedu-marble 4.6.0-1 4.6.0-1 14.95 MB

Example output (-R):

Remove (15):

Pkg Name Old Version Size

kdeutils-sweeper 4.6.0-1 0.12 MB kdeutils-superkaramba 4.6.0-1 1.08 MB kdeutils-printer-applet 4.6.0-1 0.16 MB kdeutils-kwallet 4.6.0-1 0.81 MB Probably just omitted but I assume "Total Removed Size" is there somewhere?

Indeed. Chose a better example for the commit message this time.

...

Signed-off-by: Jakob Gruber --- doc/pacman.conf.5.txt | 5 +++ etc/pacman.conf.in | 1 + src/pacman/conf.h | 1 + src/pacman/pacman.c | 3 ++ src/pacman/util.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 101 insertions(+), 4 deletions(-)

diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt index cb4c589..063d305 100644 --- a/doc/pacman.conf.5.txt +++ b/doc/pacman.conf.5.txt @@ -168,6 +168,11 @@ Options Performs an approximate check for adequate available disk space before installing packages.

+*VerbosePkgLists*:: + Displays name, version and size of target packages formatted + as a table. during which operations? You explained it in the commit message just not here.

Added.

...

+ + Repository Sections ------------------- Each repository section defines a section name and at least one location where diff --git a/etc/pacman.conf.in b/etc/pacman.conf.in index 1105db9..bf5925f 100644 --- a/etc/pacman.conf.in +++ b/etc/pacman.conf.in @@ -34,6 +34,7 @@ Architecture = auto #UseDelta #TotalDownload #CheckSpace +#VerbosePkgLists

# # REPOSITORIES diff --git a/src/pacman/conf.h b/src/pacman/conf.h index 92c379f..f440090 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -70,6 +70,7 @@ typedef struct __config_t { /* conf file options */ unsigned short chomp; /* I Love Candy! */ unsigned short showsize; /* show individual package sizes */ + unsigned short verbosepkglists; /* format target pkg lists as table */ Part of me is tempted to say we should kill showsize now. I know it shows up in -Qs and -Ss, but is that even useful? Opinions?

...

/* When downloading, display the amount downloaded, rate, ETA, and percent * downloaded of the total download list */ unsigned short totaldownload; diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 984bd1b..4cf14bb 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -967,6 +967,9 @@ static int _parse_options(char *key, char *value) } else if(strcmp(key, "ShowSize") == 0) { config->showsize = 1; pm_printf(PM_LOG_DEBUG, "config: showsize\n"); + } else if(strcmp(key, "VerbosePkgLists") == 0) { + config->verbosepkglists = 1; + pm_printf(PM_LOG_DEBUG, "config: verbosepkglists\n"); } else if(strcmp(key, "UseDelta") == 0) { alpm_option_set_usedelta(1); pm_printf(PM_LOG_DEBUG, "config: usedelta\n"); diff --git a/src/pacman/util.c b/src/pacman/util.c index 8f7b5e5..71100dd 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -607,6 +607,62 @@ void list_display_linebreak(const char *title, const alpm_list_t *list) } }

+/* creates a header row for use with table_display */ +static alpm_list_t *create_verbose_header(int install) +{ + alpm_list_t *res = NULL; + char *str; + + pm_asprintf(&str, "%s", _("Pkg Name"));

Are we that constrained for size here? "Package Name" would be good; remember also that these are translated strings and other locales could very well have longer strings.

Changed the header for the first column to just "Name".

...

+ res = alpm_list_add(res, str); + if(install) { + pm_asprintf(&str, "%s", _("New Version")); + res = alpm_list_add(res, str); + } + pm_asprintf(&str, "%s", _("Old Version")); + res = alpm_list_add(res, str); + pm_asprintf(&str, "%s", _("Size")); + res = alpm_list_add(res, str); + + return res; +} So your magic NULL, NULL first row is now slightly making sense from the other patch, but I think you are still not accounting for header size> max row sizes?

Simplified the previous patch.

...

+ +/** Generates a table row with package info * + * + * @param pkg the package + * @param install if value is zero format for remove operation, + * otherwise for installation + * + * @return a list of strings representing the current row + */ +static alpm_list_t *create_verbose_row(pmpkg_t *pkg, int install) +{ + char *str; + alpm_list_t *ret = NULL; + pmdb_t *ldb = alpm_option_get_localdb(); + + /* a row consists of the package name, */ + pm_asprintf(&str,"%s", alpm_pkg_get_name(pkg)); + ret = alpm_list_add(ret, str); + + /* (new version,) old version */ + pm_asprintf(&str,"%s", alpm_pkg_get_version(pkg)); + ret = alpm_list_add(ret, str); + + if(install) { + pmpkg_t *oldpkg = alpm_db_get_pkg(ldb, alpm_pkg_get_name(pkg)); + pm_asprintf(&str, "%s", + oldpkg != NULL ? alpm_pkg_get_version(oldpkg) : ""); + ret = alpm_list_add(ret, str); + } + + /* and size */ + str = size_to_human_string_mb(alpm_pkg_get_size(pkg)); + ret = alpm_list_add(ret, str); + + return ret; +} + /* returns package info as a string */ static char *create_list_element(pmpkg_t *pkg) { @@ -628,7 +684,7 @@ void display_targets(const alpm_list_t *pkgs, int install) const char *title; const alpm_list_t *i; off_t isize = 0, dlsize = 0; - alpm_list_t *targets = NULL; + alpm_list_t *j, *lp, *targets = NULL;

if(!pkgs) { return; @@ -636,19 +692,40 @@ void display_targets(const alpm_list_t *pkgs, int install)

title = install ? _("Targets (%d):") : _("Remove (%d):");

+ if(config->verbosepkglists) { + /* create table header and add an empty line after it */ + targets = alpm_list_add(targets, create_verbose_header(install)); + targets = alpm_list_add(targets, NULL); + } + + /* gather pkg infos */ for(i = pkgs; i; i = alpm_list_next(i)) { pmpkg_t *pkg = alpm_list_getdata(i);

dlsize += alpm_pkg_download_size(pkg); isize += alpm_pkg_get_isize(pkg);

- targets = alpm_list_add(targets, create_list_element(pkg)); + if(config->verbosepkglists) { + targets = alpm_list_add(targets, create_verbose_row(pkg, install)); + } else { + targets = alpm_list_add(targets, create_list_element(pkg)); + } }

+ /* print to screen */ pm_asprintf(&str, title, alpm_list_count(targets));

printf("\n"); - list_display(str, targets); + if(config->verbosepkglists) { + if(table_display(str, targets) != 0) { + printf(_("Insufficient columns available, using default list display...\n")); + config->verbosepkglists = 0; + display_targets(pkgs, install); + goto out; + } + } else { + list_display(str, targets); + } printf("\n");

if(install) { @@ -667,8 +744,18 @@ void display_targets(const alpm_list_t *pkgs, int install) }

out: + /* cleanup */ + if(config->verbosepkglists) { + /* targets is a list of lists of strings, free inner lists here */ + for(j = alpm_list_first(targets); j; j = alpm_list_next(j)) { + lp = alpm_list_getdata(j); + FREELIST(lp); + } + alpm_list_free(targets); + } else { + FREELIST(targets); + } free(str); - FREELIST(targets); }

static off_t pkg_get_size(pmpkg_t *pkg) -- 1.7.4.1