[pacman-dev] [PATCH] Verbose package list now show if packages are explicit or dependencies

[Andrew Gregory]

On 05/10/14 at 04:48pm, Guillaume Bouchard wrote:
> When verbose package is activated, the table is displayed two times,
> once for explicit packages and a second time for dependencies.
> 
> This helps understanding the upgrade process. You can focus on on
> explicit package and gives less attention to dependencies.
> 
> Design choices:
> 
> -- If a table does not fit in terminal width, it fallbacks to traditional
> compact display, but this fallback can happen on none, one or both
> tables
> 
> -- The header name stay short, "Package" and "Dependancies". This is to
> avoid too long column name, but i think it is enough explicit as is.
> ---
>  src/pacman/util.c | 59 +++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 46 insertions(+), 13 deletions(-)

I'm not a fan of having two independent tables, especially when one
might be printed as a table and the other a list, but I do like the
idea of making it easy to pick out the packages I actually care about.
The patch needs a few fixes, but I'll give the overall change a +1
unless somebody has a better way to provide the information.

> diff --git a/src/pacman/util.c b/src/pacman/util.c
> index d42e27b..864b7a3 100644
> --- a/src/pacman/util.c
> +++ b/src/pacman/util.c
> @@ -774,12 +774,12 @@ void signature_display(const char *title, alpm_siglist_t *siglist,
>  }
>  
>  /* creates a header row for use with table_display */
> -static alpm_list_t *create_verbose_header(size_t count)
> +static alpm_list_t *create_verbose_header(size_t count, int is_explicit)
>  {
>  	alpm_list_t *ret = NULL;
>  
>  	char *header;
> -	pm_asprintf(&header, "%s (%zd)", _("Package"), count);
> +	pm_asprintf(&header, "%s (%zd)", (is_explicit ? _("Package") : _("Dependency")), count);

"Package" and "Dependency" are a little ambiguous.  Dependency could
refer to either packages installed as dependencies or packages
currently required by other packages.  If we can't find a clearer way
to describe what each list is, I think a note should be added to the
pacman.conf man page to clarify.

>  	add_table_cell(&ret, header, CELL_TITLE | CELL_FREE);
>  	add_table_cell(&ret, _("Old Version"), CELL_TITLE);
> @@ -846,7 +846,11 @@ static void _display_targets(alpm_list_t *targets, int verbose)
>  	char *str;
>  	off_t isize = 0, rsize = 0, dlsize = 0;
>  	unsigned short cols;
> -	alpm_list_t *i, *names = NULL, *header = NULL, *rows = NULL;
> +	alpm_list_t *i,
> +				*names = NULL, *names_explicit = NULL, *names_deps = NULL,
> +				*header_explicit = NULL, *header_deps = NULL,
> +				*rows = NULL, *explicit = NULL, *deps = NULL;
> +	size_t count_explicit = 0, count_deps = 0;
>  
>  	if(!targets) {
>  		return;
> @@ -870,10 +874,6 @@ static void _display_targets(alpm_list_t *targets, int verbose)
>  	for(i = targets; i; i = alpm_list_next(i)) {
>  		pm_target_t *target = i->data;
>  
> -		if(verbose) {
> -			rows = alpm_list_add(rows, create_verbose_row(target));
> -		}
> -
>  		if(target->install) {
>  			pm_asprintf(&str, "%s-%s", alpm_pkg_get_name(target->install),
>  					alpm_pkg_get_version(target->install));
> @@ -885,27 +885,60 @@ static void _display_targets(alpm_list_t *targets, int verbose)
>  					alpm_pkg_get_version(target->remove), _("removal"));
>  		}
>  		names = alpm_list_add(names, str);
> +
> +		if(verbose) {
> +			// Check if package is installed as a explicit or as a dependency
> +			// and store it in associated lists
> +			if(alpm_pkg_get_reason(target->remove ? target->remove : target->install) == ALPM_PKG_REASON_DEPEND) {
> +					deps = alpm_list_add(deps, create_verbose_row(target));
> +					names_explicit = alpm_list_add(names_explicit, str);
> +					count_deps += 1;
> +			} else {
> +					explicit = alpm_list_add(explicit, create_verbose_row(target));
> +					names_deps = alpm_list_add(names_deps, str);
> +					count_explicit += 1;
> +			}
> +		}
> +
>  	}
>  
>  	/* print to screen */
> -	pm_asprintf(&str, "%s (%zd)", _("Packages"), alpm_list_count(targets));
>  	printf("\n");
>  
>  	cols = getcols(fileno(stdout));
>  	if(verbose) {
> -		header = create_verbose_header(alpm_list_count(targets));
> -		if(table_display(header, rows, cols) != 0) {
> +		header_explicit = create_verbose_header(count_explicit, 1);
> +		if(table_display(header_explicit, explicit, cols) != 0) {
> +			/* fallback to list display if table wouldn't fit */
> +			pm_asprintf(&str, "%s (%zd)", _("Packages"), count_explicit);
> +			list_display(str, names_explicit, cols);
> +			free(str);
> +		}
> +		printf("\n");

The trailing newline for each table should only be printed if a table
or list is printed, otherwise you end up with extra newlines when
a section is empty.

> +		header_deps = create_verbose_header(count_deps, 0);
> +		if(table_display(header_deps, deps, cols) != 0) {
>  			/* fallback to list display if table wouldn't fit */
> -			list_display(str, names, cols);
> +			pm_asprintf(&str, "%s (%zd)", _("Dependencies"), count_deps);
> +			list_display(str, names_deps, cols);
> +			free(str);
>  		}
> +
>  	} else {
> +		pm_asprintf(&str, "%s (%zd)", _("Packages"), alpm_list_count(targets));
>  		list_display(str, names, cols);
> +		free(str);
>  	}
>  	printf("\n");
>  
> -	table_free(header, rows);
> +	table_free(header_explicit, explicit);
> +	table_free(header_deps, deps);
> +
>  	FREELIST(names);
> -	free(str);
> +	/* The content of names_explicit and names_deps has allready been free'ed
> +	 * FREELIST(names), so we only free the containers */
> +	free(names_explicit);
> +	free(names_deps);

These need to be alpm_list_free.

> +
>  	rows = NULL;
>  
>  	if(dlsize > 0 || config->op_s_downloadonly) {
> -- 
> 1.9.2