On Mon, Jan 26, 2009 at 6:48 AM, Bryan Ischo <[email protected]> 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 <[email protected]> > > Signed-off-by: Bryan Ischo <[email protected]> > --- > 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 _______________________________________________ pacman-dev mailing list [email protected] http://www.archlinux.org/mailman/listinfo/pacman-dev
