[pacman-dev] [PATCH] Add new CleanMethod option.

[Xavier]

On Tue, Mar 11, 2008 at 08:25:29PM -0500, Dan McGee wrote:
> On Tue, Mar 11, 2008 at 7:59 AM, Chantry Xavier <shiningxc at gmail.com> wrote:
> >  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. :)
> 

Dropped "especially", and s/used/in use/

> >   *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.
> 

Ahah, I didnt know it existed at all until I saw pacman.conf Nagy's patch
that added options.
I don't really see the point either :)


> >  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.

>

yay..
 
> >                                         } 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?
> 

Uh yes, it is, I will rephrase that.

> >                 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).
> 

Yes, keeping the same order makes sense, and using switch looks nicer too, I
will do it in both places.