[pacman-dev] [Bulk] Re: Simple refactoring in remove.c
K. Piche
kpiche at rogers.com
Tue Apr 15 21:08:21 EDT 2008
On Mon, 2008-04-14 at 17:35 -0500, dpmcgee at gmail.com wrote:
> 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.
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 at archlinux.org
> http://archlinux.org/mailman/listinfo/pacman-dev
--
K. Piche <kpiche at rogers.com>
More information about the pacman-dev
mailing list