[pacman-dev] Improving -Qi/-Si code

[Allan McRae]

On 29/12/13 04:24, Simon Gomizelj wrote:
> I've been working out-of-tree for a replacement for -Qi / -Si, which can
> be found here: <https://github.com/vodik/alpm-dump> and I'm looking for
> some feedback before I start integrating it into pacman.
> 
> For those not familiar with why I've decided to rewrite this, the
> existing code is rather clumsy and makes some assumptions:
> 
>     string_display(_("Name           :"), alpm_pkg_get_name(pkg), cols);
> 
> The header is "Name", but the padding is embedded into the string. This
> forces translation writers to also put the right paddings into their
> translations, something which they shouldn't be burdened with.
> 
> Further, `string_display` actually needs to know how long "Name           :"
> is so it can do proper line wrapping. The following lines need to be
> indented properly. This ends up being recalculated for every header.
> 
> Instead, if we knew how wide the widest header would be ahead of time,
> we no longer need to embed padding into translations and we no longer
> need to calculate line lengths for each print operation.
> 
> This is what the code in the alpm-dump repository attempts to do.
> 
> The approach I've taken is have the printing function take an array of
> const char *. Each index represents a field, like Name or Version. If
> the index is NULL, its ignored, and if it has a value, it is printed
> along side corresponding data from the package.
> 
> For example:
> 
>     static const char *sync_table[LAST_ENTRY] = {
>         [ENTRY_REPOSITORY]   = "Repository",
>         [ENTRY_NAME]         = "Name",
>         [ENTRY_VERSION]      = "Version",
>         [ENTRY_DESCRIPTION]  = "Description",
>         [ENTRY_ARCHITECTURE] = "Architecture",
>         ...
>     };
> 
> Calculating the max width means iterating across this table once looking
> for the longest field length. Dumping a package is then a matter of
> iterating across the table, printing the stored string and then the
> value that corresponds to the array index.
>

So we have two tables, one for -Si and one for -Qi.  What about -Sii
that adds extra fields?  Will you have to add if(config->info > 1)
conditions?  In the output only, or also in the calculation of field
width?   -Qii adds "Backup Files:" at the end in a a different format,
so we can ignore that...


What are the arrays gaining us?  Removing a few if(from ==
ALPM_PKG_FROM_FILE) tests?  Is see the disadvantages over passing a
first column width to dump_pkg_full are:

1) the field order can easily become inconsistent for -Si and -Qi.
alpm_dump shows this...

2) to add/remove a field, I now need to edit an additional two arrays
and a enum on top of the output function.


I really like the idea of precomupting the first column width, but I am
seeing a decrease in maintainability with the approach involving various
arrays compared to what we currently have.

Can you clarify what we gain over adding:
  site_t dump_pkg_column_width(alpm_pkgfrom_t, int extra)
and adjusting:
  void dump_pkg_full(alpm_pkg_t *pkg, int extra, size_t colwidth)

Allan