[pacman-dev] [PATCH] Add more information in conflicts question

[Dan McGee]

On Sun, Apr 10, 2011 at 5:58 AM, Sebastian Nowicki <sebnow at gmail.com> wrote:
> In addition to the names of the conflicting packages, the origin and
> versions will be displayed to the user.
>
> This introduces an API change in the PM_TRANS_CONV_CONFLICT_PKG
> conversation callback. All three arguments were strings. The first
> two arguments are now pmpkg_t structures (sync and local, respectively).
>
> Fixes FS#12536
> ---
>
> In FS#12536 there was some discussion about changing pmconflict_t to
> contain pmpkg_t structures rather than just strings. This would
> be a much larger change, which I didn't think was worthwhile for
> a cosmetic change.
>
> Even so, this does introduce an API change, and a sneaky one at that.
> The conversation callback had three string arguments, now it has two
> pmpkg_t and a string. Due to these being all void pointers, end-users
> won't get any compiler warnings/errors if they don't update their code!

Any consideration to just building the relevant string in the backend
so we don't need 1) an API change and 2) the awfully ugly code in the
frontend to print what you are printing?

I also notice this fixes the callback case, but the other two cases
touched by 12b55958d8884bd are left the same. Not quite sure what to
make of that though.

> Sample output without the patch applied:
>
> $ sudo pacman -S qemu-kvm
> resolving dependencies...
> looking for inter-conflicts...
> :: qemu-kvm and qemu are in conflict. Remove qemu? [y/N] n
> error: unresolvable package conflicts detected
> error: failed to prepare transaction (conflicting dependencies)
> :: qemu-kvm and qemu are in conflict
>
> Sample output with the patch applied:
>
> $ sudo ./src/pacman/pacman -S qemu-kvm
> resolving dependencies...
> looking for inter-conflicts...
> :: extra/qemu-kvm-0.14.0-1 and qemu-0.14.0-1 are in conflict. Remove qemu? [y/N] n
> error: unresolvable package conflicts detected
> error: failed to prepare transaction (conflicting dependencies)
> :: qemu-kvm and qemu are in conflict
>
> This patch was generated against master (commit d3d18a).
>
>
>  lib/libalpm/sync.c    |    4 ++--
>  src/pacman/callback.c |   31 +++++++++++++++++++++----------
>  2 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
> index 5428e40..4b54969 100644
> --- a/lib/libalpm/sync.c
> +++ b/lib/libalpm/sync.c
> @@ -486,8 +486,8 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync
>                        pmpkg_t *sync = _alpm_pkg_find(trans->add, conflict->package1);
>                        pmpkg_t *local = _alpm_db_get_pkgfromcache(db_local, conflict->package2);
>                        int doremove = 0;
> -                       QUESTION(trans, PM_TRANS_CONV_CONFLICT_PKG, conflict->package1,
> -                                                       conflict->package2, conflict->reason, &doremove);
> +                       QUESTION(trans, PM_TRANS_CONV_CONFLICT_PKG, sync, local,
> +                                                       conflict->reason, &doremove);
>                        if(doremove) {
>                                /* append to the removes list */
>                                _alpm_log(PM_LOG_DEBUG, "electing '%s' for removal\n", conflict->package2);
> diff --git a/src/pacman/callback.c b/src/pacman/callback.c
> index 08c1cf3..b253546 100644
> --- a/src/pacman/callback.c
> +++ b/src/pacman/callback.c
> @@ -266,19 +266,30 @@ void cb_trans_conv(pmtransconv_t event, void *data1, void *data2,
>                                        alpm_pkg_get_name(data2));
>                        break;
>                case PM_TRANS_CONV_CONFLICT_PKG:
> -                       /* data parameters: target package, local package, conflict (strings) */
> +                       /* data parameters: target package, local package (pmpkg_t), conflict (string) */
>                        /* print conflict only if it contains new information */
> -                       if(strcmp(data1, data3) == 0 || strcmp(data2, data3) == 0) {
> -                               *response = noyes(_(":: %s and %s are in conflict. Remove %s?"),
> -                                               (char *)data1,
> -                                               (char *)data2,
> -                                               (char *)data2);
> +                       if(strcmp(alpm_pkg_get_name(data1), data3) == 0
> +                                       || strcmp(alpm_pkg_get_name(data2), data3) == 0) {
> +                               *response = noyes(_(":: %s%s%s-%s and %s-%s are in conflict. Remove %s?"),
> +                                               /* The db will be NULL if the package was read from file. */
> +                                               alpm_pkg_get_db(data1) == NULL ? "" : alpm_db_get_name(alpm_pkg_get_db(data1)),
> +                                               alpm_pkg_get_db(data1) == NULL ? "" : "/",
> +                                               alpm_pkg_get_name(data1),
> +                                               alpm_pkg_get_version(data1),
> +                                               alpm_pkg_get_name(data2),
> +                                               alpm_pkg_get_version(data2),
> +                                               alpm_pkg_get_name(data2));
>                        } else {
> -                               *response = noyes(_(":: %s and %s are in conflict (%s). Remove %s?"),
> -                                               (char *)data1,
> -                                               (char *)data2,
> +                               *response = noyes(_(":: %s%s%s-%s and %s-%s are in conflict (%s). Remove %s?"),
> +                                               /* The db will be NULL if the package was read from file. */
> +                                               alpm_pkg_get_db(data1) == NULL ? "" : alpm_db_get_name(alpm_pkg_get_db(data1)),
> +                                               alpm_pkg_get_db(data1) == NULL ? "" : "/",
> +                                               alpm_pkg_get_name(data1),
> +                                               alpm_pkg_get_version(data1),
> +                                               alpm_pkg_get_name(data2),
> +                                               alpm_pkg_get_version(data2),
This makes me cringe a bit...we should have a helper function for
this, not try to do it inline in a print format string.

>                                                (char *)data3,
> -                                               (char *)data2);
> +                                               alpm_pkg_get_name(data2));
>                        }
>                        break;
>                case PM_TRANS_CONV_REMOVE_PKGS:
> --
> 1.7.4.4