FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > ArchLinux > ArchLinux Pacman Development

 
 
LinkBack Thread Tools
 
Old 04-10-2008, 02:16 AM
"K. Piche"
 
Default 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
",
+ 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
"),
+ 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
",
+ 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
");
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
",
- 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
"),
- 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
",
- 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>


_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
 
Old 04-14-2008, 10:35 PM
 
Default Simple refactoring in remove.c

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.
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
",
> + 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
"),
> + 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
",
> + 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
");
> 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
",
> - 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
"),
> - 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
",
> - 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

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
 
Old 04-16-2008, 01:08 AM
"K. Piche"
 
Default Simple refactoring in remove.c

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
",
> > + 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
"),
> > + 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
",
> > + 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
");
> > 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
",
> > - 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
"),
> > - 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
",
> > - 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>


_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
 

Thread Tools




All times are GMT. The time now is 12:15 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org