[pacman-dev] [PATCH 2/3] Enabled a new prompt to ask the user if they'd like to remove
Dan McGee
dpmcgee at gmail.com
Wed Feb 18 22:11:37 EST 2009
On Mon, Jan 26, 2009 at 6:48 AM, Bryan Ischo
<bji-keyword-pacman.3644cb at www.ischo.com> wrote:
I think your subject got cut off. Git limits you to 76 or so
characters, so maybe shorten it up and add something to the body?
Anyway, I realized I never really reviewed this guy (really sorry
about that), and there are a few minor issues with it...
> From: Bryan Ischo <bryan at ischo.com>
>
> Signed-off-by: Bryan Ischo <bryan at ischo.com>
> ---
> lib/libalpm/alpm.h | 3 ++-
> lib/libalpm/deps.c | 2 +-
> lib/libalpm/sync.c | 23 ++++++++++++++++++-----
> pactest/tests/provision020.py | 2 +-
> pactest/tests/provision022.py | 2 +-
> pactest/tests/sync1008.py | 2 +-
> pactest/tests/sync300.py | 2 +-
> src/pacman/callback.c | 25 +++++++++++++++++++++++++
> 8 files changed, 50 insertions(+), 11 deletions(-)
>
> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> index 7b7ca4e..3836d60 100644
> --- a/lib/libalpm/alpm.h
> +++ b/lib/libalpm/alpm.h
> @@ -371,7 +371,8 @@ typedef enum _pmtransconv_t {
> PM_TRANS_CONV_REPLACE_PKG = 0x02,
> PM_TRANS_CONV_CONFLICT_PKG = 0x04,
> PM_TRANS_CONV_CORRUPTED_PKG = 0x08,
> - PM_TRANS_CONV_LOCAL_NEWER = 0x10
> + PM_TRANS_CONV_LOCAL_NEWER = 0x10,
> + PM_TRANS_CONV_REMOVE_PKGS = 0x20,
> } pmtransconv_t;
>
> /* Transaction Progress */
> diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c
> index da21087..8bec4c0 100644
> --- a/lib/libalpm/deps.c
> +++ b/lib/libalpm/deps.c
> @@ -609,7 +609,7 @@ int _alpm_resolvedeps(pmdb_t *local, alpm_list_t *dbs_sync, pmpkg_t *pkg,
> if(!spkg) {
> pm_errno = PM_ERR_UNSATISFIED_DEPS;
> char *missdepstring = alpm_dep_compute_string(missdep);
> - _alpm_log(PM_LOG_ERROR, _("cannot resolve \"%s\", a dependency of \"%s\"\n"),
> + _alpm_log(PM_LOG_WARNING, _("cannot resolve \"%s\", a dependency of \"%s\"\n"),
> missdepstring, tpkg->name);
> free(missdepstring);
> if(data) {
> diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
> index 5e5ca92..ea903f4 100644
> --- a/lib/libalpm/sync.c
> +++ b/lib/libalpm/sync.c
> @@ -442,12 +442,25 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync
> dependencies not already on the list */
> }
>
> - /* If there were unresolvable top-level packages, fail the
> - transaction. */
> + /* If there were unresolvable top-level packages, prompt the user to
> + see if they'd like to ignore them rather than failing the sync */
> if(unresolvable != NULL) {
> - /* pm_errno is set by resolvedeps */
> - ret = -1;
> - goto cleanup;
> + int remove_unresolvable = 0;
> + QUESTION(handle->trans, PM_TRANS_CONV_REMOVE_PKGS, unresolvable, NULL, NULL, &remove_unresolvable);
You can wrap this if you want, ignore my earlier weird recommendations.
> + if (remove_unresolvable) {
> + /* User wants to remove the unresolvable packages from the
> + transaction, so simply drop the unresolvable list. The
> + packages will be removed from the actual transaction when
> + the transaction packages are replaced with a
> + dependency-reordered list below */
> + alpm_list_free(unresolvable);
> + unresolvable = NULL;
> + }
> + else {
> + /* pm_errno is set by resolvedeps */
> + ret = -1;
> + goto cleanup;
> + }
> }
>
> /* Add all packages which were "pulled" (i.e. weren't already in the
> diff --git a/pactest/tests/provision020.py b/pactest/tests/provision020.py
> index 7cb0a01..c9c0ac3 100644
> --- a/pactest/tests/provision020.py
> +++ b/pactest/tests/provision020.py
> @@ -10,6 +10,6 @@
>
> self.args = "-S %s" % p.name
>
> -self.addrule("PACMAN_RETCODE=1")
> +self.addrule("PACMAN_RETCODE=0")
I think I understand why all these changed from 1 to 0, but isn't it a
bit odd that some of these "success" codes now result from pacman
doing nothing in the --noconfirm case? I guess a little explanation
for me might be in order, it might just be me that is confused.
Putting a note in the commit message might be helpful, e.g.
"The return codes for several pacman commands are now true because <reason>..."
> self.addrule("!PKG_EXIST=pkg1")
> self.addrule("PKG_EXIST=pkg2")
> diff --git a/pactest/tests/provision022.py b/pactest/tests/provision022.py
> index 4883d42..190a8b6 100644
> --- a/pactest/tests/provision022.py
> +++ b/pactest/tests/provision022.py
> @@ -10,6 +10,6 @@
>
> self.args = "-S %s" % p.name
>
> -self.addrule("PACMAN_RETCODE=1")
> +self.addrule("PACMAN_RETCODE=0")
> self.addrule("!PKG_EXIST=pkg1")
> self.addrule("PKG_EXIST=pkg2")
> diff --git a/pactest/tests/sync1008.py b/pactest/tests/sync1008.py
> index a606459..90c61df 100644
> --- a/pactest/tests/sync1008.py
> +++ b/pactest/tests/sync1008.py
> @@ -14,6 +14,6 @@
>
> self.args = "-S pkg"
>
> -self.addrule("PACMAN_RETCODE=1")
> +self.addrule("PACMAN_RETCODE=0")
> self.addrule("!PKG_EXIST=pkg")
> self.addrule("!PKG_EXIST=cpkg")
> diff --git a/pactest/tests/sync300.py b/pactest/tests/sync300.py
> index 31b520a..36d6758 100644
> --- a/pactest/tests/sync300.py
> +++ b/pactest/tests/sync300.py
> @@ -9,6 +9,6 @@
>
> self.args = "-S %s" % sp1.name
>
> -self.addrule("PACMAN_RETCODE=1")
> +self.addrule("PACMAN_RETCODE=0")
> self.addrule("!PKG_EXIST=pkg1")
> self.addrule("!PKG_EXIST=pkg2")
> diff --git a/src/pacman/callback.c b/src/pacman/callback.c
> index 6e7930c..1e2bff3 100644
> --- a/src/pacman/callback.c
> +++ b/src/pacman/callback.c
> @@ -270,6 +270,31 @@ void cb_trans_conv(pmtransconv_t event, void *data1, void *data2,
> (char *)data2,
> (char *)data2);
> break;
> + case PM_TRANS_CONV_REMOVE_PKGS:
> + {
> + /* Allocate a buffer big enough to hold all of the
> + package names */
> + char *packagenames;
> + alpm_list_t *unresolved = (alpm_list_t *) data1;
> + alpm_list_t *i;
> + int len = 1, /* for trailing \0 */ where = 0, count = 0;
> + for (i = unresolved; i; i = i->next) {
> + count += 1;
> + len += 3 /* for \t, comma, and \n */ +
> + strlen(alpm_pkg_get_name(i->data));
> + }
> + packagenames = (char *) malloc(len);
> + for (i = unresolved; i; i = i->next) {
> + where += snprintf(&(packagenames[where]), len - where, "\t%s%s\n",
You have trailing whitespace here. See my suggestion about enabling
the pre-commit hook.
> + alpm_pkg_get_name(i->data), (i->next) ? "," : "");
> + }
I really don't like reinventing the wheel here. I realize that
list_display() in util.c only takes an alpm_list_t of strings right
now, so that doesn't perfectly fit the bill, but I would rather we use
that if possible. What about something like the following (psuedo-C):
namelist = NULL;
for (i = unresolved; i; i = i->next) {
namelist = alpm_list_add(namelist, alpm_pkg_get_name(i->data);
}
....
list_display(" ", namelist);
....
alpm_list_free(namelist);
> + *response = yesno(_(":: the following package%s cannot be upgraded due to unresolvable "
> + "dependencies:\n%s\nDo you want to skip %s package%s for this upgrade?"),
> + (count > 1) ? "s" : "", packagenames, (count > 1) ? "these" : "this",
> + (count > 1) ? "s" : "");
These tricks are cool until translators are involved. You can't do
this in an i18n application- you are going to have to switch on the
whole word, or something. I'd rather just put the string "package(s)"
in there.
> + free(packagenames);
> + }
> + break;
> case PM_TRANS_CONV_LOCAL_NEWER:
> if(!config->op_s_downloadonly) {
> *response = yesno(_(":: %s-%s: local version is newer. Upgrade anyway?"),
> --
> 1.6.1
If you can't get to these changes, then I will sometime as I feel I
owe you one for being so late on reviewing these.
-Dan
More information about the pacman-dev
mailing list