[pacman-dev] Simple refactoring in remove.c
dpmcgee at gmail.com
dpmcgee at gmail.com
Mon Apr 14 18:35:13 EDT 2008
Sweet! A refactoring patch, haven't seen these in a while.
On 4/9/08, K. Piche <kpiche at rogers.com> wrote:
> >From 01b83715ef5e5dab3cb3eb5e2a3a660aaad5fb62 Mon Sep 17 00:00:00 2001
> From: K. Piche <kevin at archlinux.org>
> Date: Fri, 4 Apr 2008 23:02:23 -0400
> Subject: [PATCH] Pulled two loops out of _alpm_remove_prepare and gave
> them their own functions.
>
> Signed-off-by: K. Piche <kevin at archlinux.org>
> ---
> lib/libalpm/remove.c | 108
> +++++++++++++++++++++++++++++++------------------
> 1 files changed, 68 insertions(+), 40 deletions(-)
>
> diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c
> index 3119bf8..5ef32a2 100644
> --- a/lib/libalpm/remove.c
> +++ b/lib/libalpm/remove.c
> @@ -81,6 +81,64 @@ int _alpm_remove_loadtarget(pmtrans_t *trans, pmdb_t
> *db, char *name)
> return(0);
> }
>
> +
> +
> +void _alpm_remove_prepare_cascade(pmtrans_t *trans, pmdb_t *db,
> alpm_list_t *lp)
A few things here, some of which are rather minor points.
1 Obviously you are a fan of whitespace here, but I'd rather stay
consistant with the rest of the code. So for now, I'm going to keep
out the whitespae additions.
2. This can be static void, right? We can also drop the _alpm prefix
if we do this. I've taken the liberty of doing this locally before
pushing it to my tree, so let me know if I'm wrong.
> +{
> + ALPM_LOG_FUNC;
> +
> + while(lp) {
> + alpm_list_t *i;
> + for(i = lp; i; i = i->next) {
> + pmdepmissing_t *miss = (pmdepmissing_t *)i->data;
> + pmpkg_t *info = _alpm_db_get_pkgfromcache(db, miss->target);
> + if(info) {
> + if(!_alpm_pkg_find(alpm_pkg_get_name(info), trans->packages)) {
> + _alpm_log(PM_LOG_DEBUG, "pulling %s in the targets list\n",
> + alpm_pkg_get_name(info));
> + trans->packages = alpm_list_add(trans->packages,
> _alpm_pkg_dup(info));
> + }
> + } else {
> + _alpm_log(PM_LOG_ERROR, _("could not find %s in database --
> skipping\n"),
> + miss->target);
> + }
> + }
> + alpm_list_free_inner(lp, (alpm_list_fn_free)_alpm_depmiss_free);
> + alpm_list_free(lp);
> + lp = alpm_checkdeps(db, 1, trans->packages, NULL);
> + }
> +}
> +
> +
> +
> +void _alpm_remove_prepare_keep_needed(pmtrans_t *trans, pmdb_t *db,
> alpm_list_t *lp)
Whitespace/static void like above.
> +{
> + ALPM_LOG_FUNC;
> +
> + /* Remove needed packages (which break dependencies) from the target
> list */
> + while(lp != NULL) {
> + alpm_list_t *i;
> + for(i = lp; i; i = i->next) {
> + pmdepmissing_t *miss = (pmdepmissing_t *)i->data;
> + void *vpkg;
> + pmpkg_t *pkg;
> + trans->packages = alpm_list_remove(trans->packages,
> miss->causingpkg,
> + _alpm_pkgname_pkg_cmp, &vpkg);
> + pkg = vpkg;
> + if(pkg) {
> + _alpm_log(PM_LOG_WARNING, "removing %s from the target-list\n",
> + alpm_pkg_get_name(pkg));
> + _alpm_pkg_free(pkg);
> + }
> + }
> + alpm_list_free_inner(lp, (alpm_list_fn_free)_alpm_depmiss_free);
> + alpm_list_free(lp);
> + lp = alpm_checkdeps(db, 1, trans->packages, NULL);
> + }
> +}
> +
> +
> +
> int _alpm_remove_prepare(pmtrans_t *trans, pmdb_t *db, alpm_list_t
> **data)
> {
> alpm_list_t *lp;
> @@ -101,49 +159,18 @@ int _alpm_remove_prepare(pmtrans_t *trans, pmdb_t
> *db, alpm_list_t **data)
> _alpm_log(PM_LOG_DEBUG, "looking for unsatisfied dependencies\n");
> lp = alpm_checkdeps(db, 1, trans->packages, NULL);
> if(lp != NULL) {
> +
> if(trans->flags & PM_TRANS_FLAG_CASCADE) {
> - while(lp) {
> - alpm_list_t *i;
> - for(i = lp; i; i = i->next) {
> - pmdepmissing_t *miss = (pmdepmissing_t *)i->data;
> - pmpkg_t *info = _alpm_db_get_pkgfromcache(db, miss->target);
> - if(info) {
> - if(!_alpm_pkg_find(alpm_pkg_get_name(info), trans->packages)) {
> - _alpm_log(PM_LOG_DEBUG, "pulling %s in the targets list\n",
> - alpm_pkg_get_name(info));
> - trans->packages = alpm_list_add(trans->packages,
> _alpm_pkg_dup(info));
> - }
> - } else {
> - _alpm_log(PM_LOG_ERROR, _("could not find %s in database --
> skipping\n"),
> - miss->target);
> - }
> - }
> - alpm_list_free_inner(lp, (alpm_list_fn_free)_alpm_depmiss_free);
> - alpm_list_free(lp);
> - lp = alpm_checkdeps(db, 1, trans->packages, NULL);
> - }
> +
> + _alpm_remove_prepare_cascade(trans, db, lp);
> +
> } else if (trans->flags & PM_TRANS_FLAG_UNNEEDED) {
> - /* Remove needed packages (which break dependencies) from the
> target list */
> - while(lp != NULL) {
> - alpm_list_t *i;
> - for(i = lp; i; i = i->next) {
> - pmdepmissing_t *miss = (pmdepmissing_t *)i->data;
> - void *vpkg;
> - pmpkg_t *pkg;
> - trans->packages = alpm_list_remove(trans->packages,
> miss->causingpkg,
> - _alpm_pkgname_pkg_cmp, &vpkg);
> - pkg = vpkg;
> - if(pkg) {
> - _alpm_log(PM_LOG_WARNING, "removing %s from the target-list\n",
> - alpm_pkg_get_name(pkg));
> - _alpm_pkg_free(pkg);
> - }
> - }
> - alpm_list_free_inner(lp, (alpm_list_fn_free)_alpm_depmiss_free);
> - alpm_list_free(lp);
> - lp = alpm_checkdeps(db, 1, trans->packages, NULL);
> - }
> +
> + /* Remove needed packages (which would break dependencies) from the
> target list */
> + _alpm_remove_prepare_keep_needed(trans, db, lp);
> +
> } else {
> +
> if(data) {
> *data = lp;
> } else {
> @@ -151,6 +178,7 @@ int _alpm_remove_prepare(pmtrans_t *trans, pmdb_t
> *db, alpm_list_t **data)
> alpm_list_free(lp);
> }
> RET_ERR(PM_ERR_UNSATISFIED_DEPS, -1);
> +
> }
> }
> }
I decided to skip this hunk. :)
> --
> 1.5.4.5
Thanks for the work here, it should make the code a bit more digestible.
While on the patching frenzy, you may want to consider two things:
1. Try sending patches using git-send-email & a command line sender
like msmtp. I had to manually touch up the email patches to get them
to apply because evolution wrecks and puts its own line endings in
2. Try to keep the first line of the commit message short and sweet,
and the longer text in the body (seperated by one blank line). This
make the git log and git shortlog output useful and fitting in an 80
column terminal.
With that said, don't worry- your work is great here. Just wanted to
make my life a bit easier. :)
-Dan
More information about the pacman-dev
mailing list