[pacman-dev] [PATCH] Add new CleanMethod option.
Dan McGee
dpmcgee at gmail.com
Tue Mar 11 21:25:29 EDT 2008
On Tue, Mar 11, 2008 at 7:59 AM, Chantry Xavier <shiningxc at gmail.com> wrote:
> As it was already mentioned several times, the new -Sc behavior in 3.1 is
> great, but only when the package cache is not shared.
>
> This option has two possible values : KeepInstalled and KeepCurrent
> With KeepCurrent, -Sc will clean packages that are no longer available in
> any sync db, rather than packages that are no longer in the local db. The
> resulting behavior should be better for shared cache.
>
> Ref :
> http://www.archlinux.org/pipermail/pacman-dev/2008-February/011140.html
>
> Signed-off-by: Chantry Xavier <shiningxc at gmail.com>
> ---
> doc/pacman.8.txt | 3 ++
> doc/pacman.conf.5.txt | 9 +++++++
> src/pacman/conf.h | 13 +++++++--
> src/pacman/pacman.c | 12 +++++++++
> src/pacman/sync.c | 64 +++++++++++++++++++++++++++++++++++-------------
> 5 files changed, 80 insertions(+), 21 deletions(-)
>
> diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt
> index 8235875..7c45ce9 100644
> --- a/doc/pacman.8.txt
> +++ b/doc/pacman.8.txt
> @@ -244,6 +244,9 @@ Sync Options[[SO]]
> packages that are no longer installed; use two to remove all packages
> from the cache. In both cases, you will have a yes or no option to
> remove packages and/or unused downloaded databases.
> ++
> +If you use a network shared cache, see the 'CleanMethod' option in
> +linkman:pacman.conf[5].
>
> *-e, \--dependsonly*::
> Install all dependencies of a package, but not the specified package
> diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt
> index e8f7454..8a229f1 100644
> --- a/doc/pacman.conf.5.txt
> +++ b/doc/pacman.conf.5.txt
> @@ -114,6 +114,15 @@ Options
> 'index.php', then you would not want the 'index.html' file to be extracted
> from the 'apache' package.
>
> +*CleanMethod =* KeepInstalled | KeepCurrent::
> + If set to `KeepInstalled` (the default), the '-Sc' operation will clean
> + packages that are no longer installed (not present in the local database).
> + If set to `KeepCurrent`, '-Sc' will clean outdated packages (not present in
> + any sync database).
> + The second behavior is especially useful when the package cache is shared
> + among multiple machines, where the local databases are usually different, but
> + the sync databases used could be the same.
Small English nag- I would just say "the sync databases in use are the
same". Drop "especially" too, its really just a space filler. :)
> *UseSyslog*::
> Log action messages through syslog(). This will insert log entries into
> ``/var/log/messages'' or equivalent.
Off-topic: who actually uses this? Forgot it even existed.
> diff --git a/src/pacman/conf.h b/src/pacman/conf.h
> index d53db08..48ddba1 100644
> --- a/src/pacman/conf.h
> +++ b/src/pacman/conf.h
> @@ -66,9 +66,10 @@ typedef struct __config_t {
> unsigned short chomp; /* I Love Candy! */
> unsigned short usecolor; /* enable colorful output */
> unsigned short showsize; /* show individual package sizes */
> - unsigned short totaldownload; /* When downloading, display the amount
> - downloaded, rate, ETA, and percent
> - downloaded of the total download list */
> + /* When downloading, display the amount downloaded, rate, ETA, and percent
> + * downloaded of the total download list */
> + unsigned short totaldownload;
> + unsigned short cleanmethod; /* select -Sc behavior */
> } config_t;
>
> /* Operations */
> @@ -81,6 +82,12 @@ enum {
> PM_OP_DEPTEST
> };
>
> +/* clean method */
> +enum {
> + PM_CLEAN_KEEPINST = 0, /* default */
> + PM_CLEAN_KEEPCUR
> +};
> +
> /* global config variable */
> extern config_t *config;
>
> diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c
> index f87db27..80f7d2d 100644
> --- a/src/pacman/pacman.c
> +++ b/src/pacman/pacman.c
> @@ -699,6 +699,18 @@ static int _parseconfig(const char *file, const char *givensection,
> } else if (strcmp(key, "XferCommand") == 0 || strcmp(upperkey, "XFERCOMMAND") == 0) {
> alpm_option_set_xfercommand(ptr);
> pm_printf(PM_LOG_DEBUG, "config: xfercommand: %s\n", ptr);
> + } else if (strcmp(key, "CleanMethod") == 0 || strcmp(upperkey, "CLEANMETHOD") == 0) {
> + char *upperptr = strtoupper(strdup(ptr));
> + if (strcmp(ptr, "KeepInstalled") == 0 || strcmp(upperptr, "KEEPINSTALLED") == 0) {
> + config->cleanmethod = PM_CLEAN_KEEPINST;
> + } else if (strcmp(ptr, "KeepCurrent") == 0 || strcmp(upperptr, "KEEPCURRENT") == 0) {
> + config->cleanmethod = PM_CLEAN_KEEPCUR;
> + } else {
> + pm_printf(PM_LOG_ERROR, _("invalid value for 'CleanMethod' : '%s'\n"), ptr);
> + return(1);
> + }
> + pm_printf(PM_LOG_DEBUG, "config: cleanmethod: %s\n", ptr);
> + free(upperptr);
I see why wanting to get rid of case-insensitivity would be nice.
> } else {
> pm_printf(PM_LOG_ERROR, _("config file %s, line %d: directive '%s' not recognized.\n"),
> file, linenum, key);
> diff --git a/src/pacman/sync.c b/src/pacman/sync.c
> index f2d8f4b..ffa6b61 100644
> --- a/src/pacman/sync.c
> +++ b/src/pacman/sync.c
> @@ -133,13 +133,20 @@ static int sync_cleancache(int level)
> /* incomplete cleanup */
> DIR *dir;
> struct dirent *ent;
> - /* Let's vastly improve the way this is done. Before, we went by package
> - * name. Instead, let's only keep packages we have installed. Open up each
> - * package and see if it has an entry in the local DB; if not, delete it.
> - */
> + /* Open up each package and see if it has an entry in the local DB; if not,
> + * delete it. */
You simplified the comment, but still left "local" here. Is that misleading now?
> printf(_("Cache directory: %s\n"), cachedir);
> - if(!yesno(1, _("Do you want to remove uninstalled packages from cache?"))) {
> - return(0);
> + if(config->cleanmethod == PM_CLEAN_KEEPCUR) {
> + if(!yesno(1, _("Do you want to remove outdated packages from cache?"))) {
> + return(0);
> + }
> + } else if(config->cleanmethod == PM_CLEAN_KEEPINST) {
> + if(!yesno(1, _("Do you want to remove uninstalled packages from cache?"))) {
> + return(0);
> + }
> + } else {
> + /* this should not happen : the config parsing doesn't set any other value */
> + return(1);
Good, and I like the unreachable sanity check, as it should prevent
any "doh!" moments in the future. However, can you do the if/else if
statements in the order of the enum? This will also place the default
option first, which seems logical to me. Switch statment instead of
if? Not sure, but worth considering. gcc can do a little more static
code checking in that case (like see if you are missing values from
your enum in your case statements).
> }
> printf(_("removing old packages from cache... "));
>
> @@ -153,13 +160,14 @@ static int sync_cleancache(int level)
> /* step through the directory one file at a time */
> while((ent = readdir(dir)) != NULL) {
> char path[PATH_MAX];
> - pmpkg_t *localpkg = NULL, *dbpkg = NULL;
> + int delete = 1;
> + pmpkg_t *localpkg = NULL, *pkg = NULL;
>
> if(!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, "..")) {
> continue;
> }
> /* build the full filepath */
> - snprintf(path, PATH_MAX, "%s/%s", cachedir, ent->d_name);
> + snprintf(path, PATH_MAX, "%s%s", cachedir, ent->d_name);
>
> /* attempt to load the package, skip file on failures as we may have
> * files here that aren't valid packages. we also don't need a full
> @@ -167,19 +175,39 @@ static int sync_cleancache(int level)
> if(alpm_pkg_load(path, 0, &localpkg) != 0 || localpkg == NULL) {
> continue;
> }
> - /* check if this package is in the local DB */
> - dbpkg = alpm_db_get_pkg(db_local, alpm_pkg_get_name(localpkg));
> - if(dbpkg == NULL) {
> - /* delete package, not present in local DB */
> - unlink(path);
> - } else if(alpm_pkg_vercmp(alpm_pkg_get_version(localpkg),
> - alpm_pkg_get_version(dbpkg)) != 0) {
> - /* delete package, it was found but version differs */
> - unlink(path);
> + if(config->cleanmethod == PM_CLEAN_KEEPCUR) {
> + /* check if this package is in a sync DB */
> + alpm_list_t *sync_dbs = alpm_option_get_syncdbs();
> + alpm_list_t *j;
> +
> + for(j = sync_dbs; j; j = alpm_list_next(j)) {
> + pmdb_t *db = alpm_list_getdata(j);
> + pkg = alpm_db_get_pkg(db, alpm_pkg_get_name(localpkg));
> + if(pkg != NULL && alpm_pkg_vercmp(alpm_pkg_get_version(localpkg),
> + alpm_pkg_get_version(pkg)) == 0) {
> + /* package was found in a sync DB and version matches, keep it */
> + delete = 0;
> + break;
> + }
> + }
> + } else if(config->cleanmethod == PM_CLEAN_KEEPINST) {
> + /* check if this package is in the local DB */
> + pkg = alpm_db_get_pkg(db_local, alpm_pkg_get_name(localpkg));
> + if(pkg != NULL && alpm_pkg_vercmp(alpm_pkg_get_version(localpkg),
> + alpm_pkg_get_version(pkg)) == 0) {
> + /* package was found in local DB and version matches, keep it */
> + delete = 0;
> + }
> + } else {
> + /* this should not happen : the config parsing doesn't set any other value */
> + delete = 0;
Switch order here too please.
> }
> - /* else version was the same, so keep the package */
> /* free the local file package */
> alpm_pkg_free(localpkg);
> +
> + if(delete) {
> + unlink(path);
> + }
> }
> printf(_("done.\n"));
> } else {
If you address my comments, I'll pull this in, good work.
-Dan
More information about the pacman-dev
mailing list