[pacman-dev] Simple refactoring in remove.c
From 01b83715ef5e5dab3cb3eb5e2a3a660aaad5fb62 Mon Sep 17 00:00:00 2001 From: K. Piche <kevin@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@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) +{ + 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) +{ + 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); + } } } -- 1.5.4.5 -- K. Piche <kpiche@rogers.com>
From 01b83715ef5e5dab3cb3eb5e2a3a660aaad5fb62 Mon Sep 17 00:00:00 2001 From: K. Piche <kevin@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@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.
Sweet! A refactoring patch, haven't seen these in a while. On 4/9/08, K. Piche <kpiche@rogers.com> wrote: 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
On Mon, 2008-04-14 at 17:35 -0500, dpmcgee@gmail.com wrote:
Sweet! A refactoring patch, haven't seen these in a while.
On 4/9/08, K. Piche <kpiche@rogers.com> wrote:
From 01b83715ef5e5dab3cb3eb5e2a3a660aaad5fb62 Mon Sep 17 00:00:00 2001 From: K. Piche <kevin@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@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.
Too true. I've always had a problem with Aurelian's code regarding whitespace and no comments. It's easier to read if it's not jammed together. We could probably correct this later with indent or some other formatter.
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.
Didn't think of that: static is fine since they're only called from within this file.
+{ + 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.
Will do for 1 & 2. Thanks.
With that said, don't worry- your work is great here. Just wanted to make my life a bit easier. :)
-Dan
_______________________________________________ pacman-dev mailing list pacman-dev@archlinux.org http://archlinux.org/mailman/listinfo/pacman-dev -- K. Piche <kpiche@rogers.com>
participants (2)
-
dpmcgee＠gmail.com
-
K. Piche