Signed-off-by: Pierre Neidhardt --- src/common/util-common.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/common/util-common.h b/src/common/util-common.h index a2093be..373db19 100644 --- a/src/common/util-common.h +++ b/src/common/util-common.h @@ -34,6 +34,8 @@ char *safe_fgets(char *s, int size, FILE *stream); char *strndup(const char *s, size_t n); #endif +#define ARRAYSIZE(a) (sizeof (a) / sizeof (a[0])) + #endif /* _PM_UTIL_COMMON_H */ /* vim: set noet: */ -- 2.6.1

On Tue, Oct 20, 2015 at 03:30:34PM +0200, Pierre Neidhardt wrote:

Signed-off-by: Pierre Neidhardt --- src/pacman/package.c | 173 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 141 insertions(+), 32 deletions(-)

diff --git a/src/pacman/package.c b/src/pacman/package.c index 6b86599..617ce8d 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -25,6 +25,7 @@ #include #include #include +#include

#include #include @@ -36,6 +37,107 @@

#define CLBUF_SIZE 4096

+/* The term "title" refers to the first field of each line in the package + * information displayed by pacman. Titles are stored in the `titles` array and + * referenced by the following indices. + */ +enum { + T_ARCHITECTURE = 0, + T_BACKUP_FILES, + T_BUILD_DATE, + T_COMPRESSED_SIZE, + T_CONFLICTS_WITH, + T_DEPENDS_ON, + T_DESCRIPTION, + T_DOWNLOAD_SIZE, + T_GROUPS, + T_INSTALL_DATE, + T_INSTALL_REASON, + T_INSTALL_SCRIPT, + T_INSTALLED_SIZE, + T_LICENSES, + T_MD5_SUM, + T_NAME, + T_OPTIONAL_DEPS, + T_OPTIONAL_FOR, + T_PACKAGER, + T_PROVIDES, + T_REPLACES, + T_REPOSITORY, + T_REQUIRED_BY, + T_SHA_256_SUM, + T_SIGNATURES, + T_URL, + T_VALIDATED_BY, + T_VERSION, + /* the following is a sentinel and should remain in last position */ + TITLE_COUNT

I still believe this should be _T_MAX and not some unique butterfly in this enum, but I'm running out of interest in fighting it. Patch looks fine otherwise.

+}; + +/* As of 2015/10/20, the longest title (all locales considered) was less than 30 + * characters long. We set the title maximum length to 50 to allow for some + * potential growth. + */ +#define TITLE_MAXLEN 50 + +static char titles[TITLE_COUNT][TITLE_MAXLEN * sizeof(wchar_t)]; + +/** Build the `titles` array of localized titles and pad them with spaces so + * that they align with the longest title. Storage for strings is stack + * allocated and naively truncated to TITLE_MAXLEN characters. + */ +static void make_aligned_titles() +{ + int i; + size_t max = 0; + static const wchar_t *title_suffix = L" :"; + static const size_t title_suffix_len = sizeof(title_suffix); + wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + title_suffix_len]; + size_t wlen[ARRAYSIZE(wbuf)]; + char *buf[ARRAYSIZE(wbuf)]; + buf[T_ARCHITECTURE] = _("Architecture"); + buf[T_BACKUP_FILES] = _("Backup Files"); + buf[T_BUILD_DATE] = _("Build Date"); + buf[T_COMPRESSED_SIZE] = _("Compressed Size"); + buf[T_CONFLICTS_WITH] = _("Conflicts With"); + buf[T_DEPENDS_ON] = _("Depends On"); + buf[T_DESCRIPTION] = _("Description"); + buf[T_DOWNLOAD_SIZE] = _("Download Size"); + buf[T_GROUPS] = _("Groups"); + buf[T_INSTALL_DATE] = _("Install Date"); + buf[T_INSTALL_REASON] = _("Install Reason"); + buf[T_INSTALL_SCRIPT] = _("Install Script"); + buf[T_INSTALLED_SIZE] = _("Installed Size"); + buf[T_LICENSES] = _("Licenses"); + buf[T_MD5_SUM] = _("MD5 Sum"); + buf[T_NAME] = _("Name"); + buf[T_OPTIONAL_DEPS] = _("Optional Deps"); + buf[T_OPTIONAL_FOR] = _("Optional For"); + buf[T_PACKAGER] = _("Packager"); + buf[T_PROVIDES] = _("Provides"); + buf[T_REPLACES] = _("Replaces"); + buf[T_REPOSITORY] = _("Repository"); + buf[T_REQUIRED_BY] = _("Required By"); + buf[T_SHA_256_SUM] = _("SHA-256 Sum"); + buf[T_SIGNATURES] = _("Signatures"); + buf[T_URL] = _("URL"); + buf[T_VALIDATED_BY] = _("Validated By"); + buf[T_VERSION] = _("Version"); + + for(i = 0; i < ARRAYSIZE(wbuf); i++) { + wlen[i] = mbstowcs(wbuf[i], buf[i], strlen(buf[i]) + 1); + if(wlen[i] > max) { + max = wlen[i]; + } + } + + for(i = 0; i < ARRAYSIZE(wbuf); i++) { + wmemset(wbuf[i] + wlen[i], L' ', max - wlen[i]); + wmemcpy(wbuf[i] + max, title_suffix, title_suffix_len); + wcstombs(titles[i], wbuf[i], sizeof(wbuf[i])); + } +} + /** Turn a depends list into a text list. * @param deps a list with items of type alpm_depend_t */ @@ -70,7 +172,7 @@ static void optdeplist_display(alpm_pkg_t *pkg, unsigned short cols) } text = alpm_list_add(text, depstring); } - list_display_linebreak(_("Optional Deps :"), text, cols); + list_display_linebreak(titles[T_OPTIONAL_DEPS], text, cols); FREELIST(text); }

@@ -91,6 +193,13 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) const char *label, *reason; alpm_list_t *validation = NULL, *requiredby = NULL, *optionalfor = NULL;

+ /* make aligned titles once only */ + static int need_alignment = 1; + if(need_alignment) { + need_alignment = 0; + make_aligned_titles(); + } + from = alpm_pkg_get_origin(pkg);

/* set variables here, do all output below */ @@ -124,7 +233,7 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) validation = alpm_list_add(validation, _("MD5 Sum")); } if(v & ALPM_PKG_VALIDATION_SHA256SUM) { - validation = alpm_list_add(validation, _("SHA256 Sum")); + validation = alpm_list_add(validation, _("SHA-256 Sum")); } if(v & ALPM_PKG_VALIDATION_SIGNATURE) { validation = alpm_list_add(validation, _("Signature")); @@ -144,33 +253,33 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra)

/* actual output */ if(from == ALPM_PKG_FROM_SYNCDB) { - string_display(_("Repository :"), + string_display(titles[T_REPOSITORY], alpm_db_get_name(alpm_pkg_get_db(pkg)), cols); } - string_display(_("Name :"), alpm_pkg_get_name(pkg), cols); - string_display(_("Version :"), alpm_pkg_get_version(pkg), cols); - string_display(_("Description :"), alpm_pkg_get_desc(pkg), cols); - string_display(_("Architecture :"), alpm_pkg_get_arch(pkg), cols); - string_display(_("URL :"), alpm_pkg_get_url(pkg), cols); - list_display(_("Licenses :"), alpm_pkg_get_licenses(pkg), cols); - list_display(_("Groups :"), alpm_pkg_get_groups(pkg), cols); - deplist_display(_("Provides :"), alpm_pkg_get_provides(pkg), cols); - deplist_display(_("Depends On :"), alpm_pkg_get_depends(pkg), cols); + string_display(titles[T_NAME], alpm_pkg_get_name(pkg), cols); + string_display(titles[T_VERSION], alpm_pkg_get_version(pkg), cols); + string_display(titles[T_DESCRIPTION], alpm_pkg_get_desc(pkg), cols); + string_display(titles[T_ARCHITECTURE], alpm_pkg_get_arch(pkg), cols); + string_display(titles[T_URL], alpm_pkg_get_url(pkg), cols); + list_display(titles[T_LICENSES], alpm_pkg_get_licenses(pkg), cols); + list_display(titles[T_GROUPS], alpm_pkg_get_groups(pkg), cols); + deplist_display(titles[T_PROVIDES], alpm_pkg_get_provides(pkg), cols); + deplist_display(titles[T_DEPENDS_ON], alpm_pkg_get_depends(pkg), cols); optdeplist_display(pkg, cols);

if(extra || from == ALPM_PKG_FROM_LOCALDB) { - list_display(_("Required By :"), requiredby, cols); - list_display(_("Optional For :"), optionalfor, cols); + list_display(titles[T_REQUIRED_BY], requiredby, cols); + list_display(titles[T_OPTIONAL_FOR], optionalfor, cols); } - deplist_display(_("Conflicts With :"), alpm_pkg_get_conflicts(pkg), cols); - deplist_display(_("Replaces :"), alpm_pkg_get_replaces(pkg), cols); + deplist_display(titles[T_CONFLICTS_WITH], alpm_pkg_get_conflicts(pkg), cols); + deplist_display(titles[T_REPLACES], alpm_pkg_get_replaces(pkg), cols);

size = humanize_size(alpm_pkg_get_size(pkg), '\0', 2, &label); if(from == ALPM_PKG_FROM_SYNCDB) { - printf("%s%s%s %6.2f %s\n", config->colstr.title, _("Download Size :"), + printf("%s%s%s %6.2f %s\n", config->colstr.title, titles[T_DOWNLOAD_SIZE], config->colstr.nocolor, size, label); } else if(from == ALPM_PKG_FROM_FILE) { - printf("%s%s%s %6.2f %s\n", config->colstr.title, _("Compressed Size:"), + printf("%s%s%s %6.2f %s\n", config->colstr.title, titles[T_COMPRESSED_SIZE], config->colstr.nocolor, size, label); } else { /* autodetect size for "Installed Size" */ @@ -178,17 +287,17 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) }

size = humanize_size(alpm_pkg_get_isize(pkg), label[0], 2, &label); - printf("%s%s%s %6.2f %s\n", config->colstr.title, _("Installed Size :"), + printf("%s%s%s %6.2f %s\n", config->colstr.title, titles[T_INSTALLED_SIZE], config->colstr.nocolor, size, label);

- string_display(_("Packager :"), alpm_pkg_get_packager(pkg), cols); - string_display(_("Build Date :"), bdatestr, cols); + string_display(titles[T_PACKAGER], alpm_pkg_get_packager(pkg), cols); + string_display(titles[T_BUILD_DATE], bdatestr, cols); if(from == ALPM_PKG_FROM_LOCALDB) { - string_display(_("Install Date :"), idatestr, cols); - string_display(_("Install Reason :"), reason, cols); + string_display(titles[T_INSTALL_DATE], idatestr, cols); + string_display(titles[T_INSTALL_REASON], reason, cols); } if(from == ALPM_PKG_FROM_FILE || from == ALPM_PKG_FROM_LOCALDB) { - string_display(_("Install Script :"), + string_display(titles[T_INSTALL_SCRIPT], alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No"), cols); }

@@ -206,27 +315,27 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) keys = alpm_list_add(keys, _("None")); }

- string_display(_("MD5 Sum :"), alpm_pkg_get_md5sum(pkg), cols); - string_display(_("SHA-256 Sum :"), alpm_pkg_get_sha256sum(pkg), cols); - list_display(_("Signatures :"), keys, cols); + string_display(titles[T_MD5_SUM], alpm_pkg_get_md5sum(pkg), cols); + string_display(titles[T_SHA_256_SUM], alpm_pkg_get_sha256sum(pkg), cols); + list_display(titles[T_SIGNATURES], keys, cols);

if(base64_sig) { FREELIST(keys); } } else { - list_display(_("Validated By :"), validation, cols); + list_display(titles[T_VALIDATED_BY], validation, cols); }

if(from == ALPM_PKG_FROM_FILE) { alpm_siglist_t siglist; int err = alpm_pkg_check_pgp_signature(pkg, &siglist); if(err && alpm_errno(config->handle) == ALPM_ERR_SIG_MISSING) { - string_display(_("Signatures :"), _("None"), cols); + string_display(titles[T_SIGNATURES], _("None"), cols); } else if(err) { - string_display(_("Signatures :"), + string_display(titles[T_SIGNATURES], alpm_strerror(alpm_errno(config->handle)), cols); } else { - signature_display(_("Signatures :"), &siglist, cols); + signature_display(titles[T_SIGNATURES], &siglist, cols); } alpm_siglist_cleanup(&siglist); } @@ -290,7 +399,7 @@ void dump_pkg_backups(alpm_pkg_t *pkg) { alpm_list_t *i; const char *root = alpm_option_get_root(config->handle); - printf("%s%s%s", config->colstr.title, _("Backup Files:\n"), + printf("%s%s\n%s", config->colstr.title, titles[T_BACKUP_FILES], config->colstr.nocolor); if(alpm_pkg_get_backup(pkg)) { /* package has backup files, so print them */ -- 2.6.1

On 21/10/15 17:15, Pierre Neidhardt wrote:

On 15-10-21 14:49:21, Allan McRae wrote:

...

On 20/10/15 23:30, Pierre Neidhardt wrote:

...

Signed-off-by: Pierre Neidhardt --- src/common/util-common.h | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/src/common/util-common.h b/src/common/util-common.h index a2093be..373db19 100644 --- a/src/common/util-common.h +++ b/src/common/util-common.h @@ -34,6 +34,8 @@ char *safe_fgets(char *s, int size, FILE *stream); char *strndup(const char *s, size_t n); #endif

+#define ARRAYSIZE(a) (sizeof (a) / sizeof (a[0])) + #endif /* _PM_UTIL_COMMON_H */

This is needed nowhere in libalpm, so put this in src/pacman/util.c at the top of the file. And you might as well use it in the three places that could use it in pacman while you are at it.

Allan

How do you define the appropriate spots where it can be used? I believe alpm could use the macro in many places as well. For instance, both pacman and alpm have numerous PATH_MAX referencing the size of an array (beside its initialization of course).

PATH_MAX is used all over the place for storing paths. They usually get filled with a snprintf result and then have the length stored. I don't see a use for this define there.

I endorse the use of ARRAYSIZE, but I must say it is quite inconsistent with what has been done until now. Suggestions:

* We replace every static array size reference with a call to the macro. Lots of work and error prone.

I can see use doing the size of calculation in three places: $ git grep sizeof | grep "\[" src/pacman/pacman.c: for(i = 0; i < sizeof(signals) / sizeof(signals[0]); i++) { src/pacman/sync.c: for(j = 0; j < sizeof(glob_skips) / sizeof(glob_skips[0]); j++) { src/pacman/util.c: static const int unitcount = sizeof(labels) / sizeof(labels[0]); I think you can replace those without too many errors creeping in.

* Or we only use it in future code. * Or we don't use it at all and I remove it from my patch.

On 21/10/15 17:55, Pierre Neidhardt wrote:

On 15-10-21 17:28:11, Allan McRae wrote:

...

On 21/10/15 17:15, Pierre Neidhardt wrote:

...

On 15-10-21 14:49:21, Allan McRae wrote:

...

On 20/10/15 23:30, Pierre Neidhardt wrote:

...

Signed-off-by: Pierre Neidhardt --- src/common/util-common.h | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/src/common/util-common.h b/src/common/util-common.h index a2093be..373db19 100644 --- a/src/common/util-common.h +++ b/src/common/util-common.h @@ -34,6 +34,8 @@ char *safe_fgets(char *s, int size, FILE *stream); char *strndup(const char *s, size_t n); #endif

+#define ARRAYSIZE(a) (sizeof (a) / sizeof (a[0])) + #endif /* _PM_UTIL_COMMON_H */

This is needed nowhere in libalpm, so put this in src/pacman/util.c at the top of the file. And you might as well use it in the three places that could use it in pacman while you are at it.

Allan

How do you define the appropriate spots where it can be used? I believe alpm could use the macro in many places as well. For instance, both pacman and alpm have numerous PATH_MAX referencing the size of an array (beside its initialization of course).

PATH_MAX is used all over the place for storing paths. They usually get filled with a snprintf result and then have the length stored. I don't see a use for this define there.

PATH_MAX is not a great example as it is fairly idiomatic. But you can use ARRAYSIZE for any string manipulation, e.g.:

snprintf(s, ARRAYSIZE(s), ...

This way you can choose to restrict the explicit 'sizeof' calls to types. As was originally suggested in HACKING.

It's a simple matter of style consistency, and since this was not done before I don't think we should change it.

So we are both saying it should not get used in string manipulation? Which is good because I would not accept changes to that...

...

...

I endorse the use of ARRAYSIZE, but I must say it is quite inconsistent with what has been done until now. Suggestions:

* We replace every static array size reference with a call to the macro. Lots of work and error prone.

I can see use doing the size of calculation in three places:

$ git grep sizeof | grep "\[" src/pacman/pacman.c: for(i = 0; i < sizeof(signals) / sizeof(signals[0]); i++) { src/pacman/sync.c: for(j = 0; j < sizeof(glob_skips) / sizeof(glob_skips[0]); j++) { src/pacman/util.c: static const int unitcount = sizeof(labels) / sizeof(labels[0]);

Wrong regex:

$ git grep "sizeof *([^)]*) */" lib/libalpm/pkghash.c: loopsize = sizeof(prime_list) / sizeof(*prime_list); src/pacman/pacman.c: for(i = 0; i < sizeof(signals) / sizeof(signals[0]); i++) { src/pacman/sync.c: for(j = 0; j < sizeof(glob_skips) / sizeof(glob_skips[0]); j++) { src/pacman/util.c: static const int unitcount = sizeof(labels) / sizeof(labels[0]);

So yes, alpm has *one* use for it! :D

OK - keep it in util-common then. Will you submit a patch with those changed to use ARRAYSIZE? Or should I just patchcount++ myself? A