[pacman-dev] PATCH 8/8] Replace hardcoded option numbers with enumeration
Pacman's long option parsing used hardcoded numbers to identify them. This is not good practice, so replace them with enumeration constants. Signed-off-by: Laszlo Papp <djszapi@archlinux.us> --- src/pacman/conf.h | 18 ++++++++++++++++ src/pacman/pacman.c | 56 +++++++++++++++++++++++++----- -------------------- 2 files changed, 46 insertions(+), 28 deletions(-) diff --git a/src/pacman/conf.h b/src/pacman/conf.h index 3c588a7..c854df4 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -88,6 +88,24 @@ enum { PM_OP_DEPTEST }; +/* Long Operations */ +enum { + PM_LONG_OP_NOCONFIRM = 1000, + PM_LONG_OP_CONFIG, + PM_LONG_OP_IGNORE, + PM_LONG_OP_DEBUG, + PM_LONG_OP_NOPROGRESSBAR, + PM_LONG_OP_NOSCRIPTLET, + PM_LONG_OP_ASK, + PM_LONG_OP_CACHEDIR, + PM_LONG_OP_ASDEPS, + PM_LONG_OP_LOGFILE, + PM_LONG_OP_IGNOREGROUP, + PM_LONG_OP_NEEDED, + PM_LONG_OP_ASEXPLICIT, + PM_LONG_OP_ARCH +}; + /* clean method */ enum { PM_CLEAN_KEEPINST = 0, /* default */ diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index ac51502..30b39b5 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -378,20 +378,20 @@ static int parseargs(int argc, char *argv[]) {"verbose", no_argument, 0, 'v'}, {"downloadonly", no_argument, 0, 'w'}, {"refresh", no_argument, 0, 'y'}, - {"noconfirm", no_argument, 0, 1000}, - {"config", required_argument, 0, 1001}, - {"ignore", required_argument, 0, 1002}, - {"debug", optional_argument, 0, 1003}, - {"noprogressbar", no_argument, 0, 1004}, - {"noscriptlet", no_argument, 0, 1005}, - {"ask", required_argument, 0, 1006}, - {"cachedir", required_argument, 0, 1007}, - {"asdeps", no_argument, 0, 1008}, - {"logfile", required_argument, 0, 1009}, - {"ignoregroup", required_argument, 0, 1010}, - {"needed", no_argument, 0, 1011}, - {"asexplicit", no_argument, 0, 1012}, - {"arch", required_argument, 0, 1013}, + {"noconfirm", no_argument, 0, PM_LONG_OP_NOCONFIRM}, + {"config", required_argument, 0, PM_LONG_OP_CONFIG}, + {"ignore", required_argument, 0, PM_LONG_OP_IGNORE}, + {"debug", optional_argument, 0, PM_LONG_OP_DEBUG}, + {"noprogressbar", no_argument, 0, PM_LONG_OP_NOPROGRESSBAR}, + {"noscriptlet", no_argument, 0, PM_LONG_OP_NOSCRIPTLET}, + {"ask", required_argument, 0, PM_LONG_OP_ASK}, + {"cachedir", required_argument, 0, PM_LONG_OP_CACHEDIR}, + {"asdeps", no_argument, 0, PM_LONG_OP_ASDEPS}, + {"logfile", required_argument, 0, PM_LONG_OP_LOGFILE}, + {"ignoregroup", required_argument, 0, PM_LONG_OP_IGNOREGROUP}, + {"needed", no_argument, 0, PM_LONG_OP_NEEDED}, + {"asexplicit", no_argument, 0, PM_LONG_OP_ASEXPLICIT}, + {"arch", required_argument, 0, PM_LONG_OP_ARCH}, {0, 0, 0, 0} }; @@ -403,21 +403,21 @@ static int parseargs(int argc, char *argv[]) } switch(opt) { case 0: break; - case 1000: config->noconfirm = 1; break; - case 1001: + case PM_LONG_OP_NOCONFIRM: config->noconfirm = 1; break; + case PM_LONG_OP_CONFIG: if(config->configfile) { free(config->configfile); } config->configfile = strndup(optarg, PATH_MAX); break; - case 1002: + case PM_LONG_OP_IGNORE: list = strsplit(optarg, ','); for(item = list; item; item = alpm_list_next(item)) { alpm_option_add_ignorepkg((char *)alpm_list_getdata(item)); } FREELIST(list); break; - case 1003: + case PM_LONG_OP_DEBUG: /* debug levels are made more 'human readable' than using a raw logmask * here, error and warning are set in config_new, though perhaps a * --quiet option will remove these later */ @@ -440,34 +440,34 @@ static int parseargs(int argc, char *argv[]) /* progress bars get wonky with debug on, shut them off */ config->noprogressbar = 1; break; - case 1004: config->noprogressbar = 1; break; - case 1005: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; - case 1006: config->noask = 1; config->ask = atoi(optarg); break; - case 1007: + case PM_LONG_OP_NOPROGRESSBAR: config->noprogressbar = 1; break; + case PM_LONG_OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; + case PM_LONG_OP_ASK: config->noask = 1; config->ask = atoi(optarg); break; + case PM_LONG_OP_CACHEDIR: if(alpm_option_add_cachedir(optarg) != 0) { pm_printf(PM_LOG_ERROR, _("problem adding cachedir '%s' (%s)\n"), optarg, alpm_strerrorlast()); return(1); } break; - case 1008: + case PM_LONG_OP_ASDEPS: config->flags |= PM_TRANS_FLAG_ALLDEPS; break; - case 1009: + case PM_LONG_OP_LOGFILE: config->logfile = strndup(optarg, PATH_MAX); break; - case 1010: + case PM_LONG_OP_IGNOREGROUP: list = strsplit(optarg, ','); for(item = list; item; item = alpm_list_next(item)) { alpm_option_add_ignoregrp((char *)alpm_list_getdata(item)); } FREELIST(list); break; - case 1011: config->flags |= PM_TRANS_FLAG_NEEDED; break; - case 1012: + case PM_LONG_OP_NEEDED: config->flags |= PM_TRANS_FLAG_NEEDED; break; + case PM_LONG_OP_ASEXPLICIT: config->flags |= PM_TRANS_FLAG_ALLEXPLICIT; break; - case 1013: + case PM_LONG_OP_ARCH: setarch(optarg); break; case 'Q': config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_QUERY); break; -- 1.6.4.4
On Wed, Sep 30, 2009 at 9:49 PM, Laszlo Papp <djszapi@archlinux.us> wrote:
Pacman's long option parsing used hardcoded numbers to identify them. This is not good practice, so replace them with enumeration constants.
Signed-off-by: Laszlo Papp <djszapi@archlinux.us> ---
I can't apply this (or your other patches), the lines are wrapped in the patch. Please use git-send-email or some other method that doesn't wrap the lines. Other than that, this patch looks good. For the general audience, is there any reason not just to make these OP_* constants? PM_LONG_OP_ seems a bit excessive for something that isn't in the API or anything. -Dan
On Sun, Oct 11, 2009 at 10:02 PM, Dan McGee <dpmcgee@gmail.com> wrote:
On Wed, Sep 30, 2009 at 9:49 PM, Laszlo Papp <djszapi@archlinux.us> wrote:
Pacman's long option parsing used hardcoded numbers to identify them. This is not good practice, so replace them with enumeration constants.
Signed-off-by: Laszlo Papp <djszapi@archlinux.us> ---
I can't apply this (or your other patches), the lines are wrapped in the patch. Please use git-send-email or some other method that doesn't wrap the lines. Other than that, this patch looks good.
For the general audience, is there any reason not just to make these OP_* constants? PM_LONG_OP_ seems a bit excessive for something that isn't in the API or anything.
OP_* sounds good to me.
On Sun, Oct 11, 2009 at 10:09 PM, Xavier <shiningxc@gmail.com> wrote:
On Sun, Oct 11, 2009 at 10:02 PM, Dan McGee <dpmcgee@gmail.com> wrote:
On Wed, Sep 30, 2009 at 9:49 PM, Laszlo Papp <djszapi@archlinux.us> wrote:
Pacman's long option parsing used hardcoded numbers to identify them. This is not good practice, so replace them with enumeration constants.
Signed-off-by: Laszlo Papp <djszapi@archlinux.us> ---
I can't apply this (or your other patches), the lines are wrapped in the patch. Please use git-send-email or some other method that doesn't wrap the lines. Other than that, this patch looks good.
For the general audience, is there any reason not just to make these OP_* constants? PM_LONG_OP_ seems a bit excessive for something that isn't in the API or anything.
OP_* sounds good to me.
If you see into the ./src/pacman/conf.h file, you will see the existing operations enumeration start with PM_OP_*. What could I take in this case, is PM_OP_* and that enumeration okay to extend ? Best Regards, Laszlo Papp
On Sun, Oct 11, 2009 at 8:42 PM, Laszlo Papp <djszapi@archlinux.us> wrote:
On Sun, Oct 11, 2009 at 10:09 PM, Xavier <shiningxc@gmail.com> wrote:
On Sun, Oct 11, 2009 at 10:02 PM, Dan McGee <dpmcgee@gmail.com> wrote:
On Wed, Sep 30, 2009 at 9:49 PM, Laszlo Papp <djszapi@archlinux.us> wrote:
Pacman's long option parsing used hardcoded numbers to identify them. This is not good practice, so replace them with enumeration constants.
Signed-off-by: Laszlo Papp <djszapi@archlinux.us> ---
I can't apply this (or your other patches), the lines are wrapped in the patch. Please use git-send-email or some other method that doesn't wrap the lines. Other than that, this patch looks good.
For the general audience, is there any reason not just to make these OP_* constants? PM_LONG_OP_ seems a bit excessive for something that isn't in the API or anything.
OP_* sounds good to me.
If you see into the ./src/pacman/conf.h file, you will see the existing operations enumeration start with PM_OP_*. What could I take in this case, is PM_OP_* and that enumeration okay to extend ?
Those opts have a bit of a different meaning, so I don't think that is a wise idea. I'd just make it it's own enum. I'm much more worried about getting the patch in an appliable format than anything else; sorry I got cut off by my wonderful internet connection on IR earlier. Your patch definitely seems to have had newlines in it. I believe you claimed you used git send-email; that does not appear to be the case or these two email headers would be a dead giveaway (and they were not in your patch, this is from a patch Allan sent): Message-Id: <1255262385-18125-1-git-send-email-allan@archlinux.org> X-Mailer: git-send-email 1.6.4.4 -Dan
- Pacman's long option parsing used hardcoded numbers to identify them. This is not good practice, so replace them with enumeration constants. Signed-off-by: Laszlo Papp <djszapi@archlinux.us> --- If this patch is not okay, feel free to modify it then. src/pacman/conf.h | 18 ++++++++++++++++ src/pacman/pacman.c | 56 +++++++++++++++++++++++++------------------------- 2 files changed, 46 insertions(+), 28 deletions(-) diff --git a/src/pacman/conf.h b/src/pacman/conf.h index 3c588a7..c97e5d7 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -88,6 +88,24 @@ enum { PM_OP_DEPTEST }; +/* Long Operations */ +enum { + OP_NOCONFIRM = 1000, + OP_CONFIG, + OP_IGNORE, + OP_DEBUG, + OP_NOPROGRESSBAR, + OP_NOSCRIPTLET, + OP_ASK, + OP_CACHEDIR, + OP_ASDEPS, + OP_LOGFILE, + OP_IGNOREGROUP, + OP_NEEDED, + OP_ASEXPLICIT, + OP_ARCH +}; + /* clean method */ enum { PM_CLEAN_KEEPINST = 0, /* default */ diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 5e824f4..f4f8044 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -378,20 +378,20 @@ static int parseargs(int argc, char *argv[]) {"verbose", no_argument, 0, 'v'}, {"downloadonly", no_argument, 0, 'w'}, {"refresh", no_argument, 0, 'y'}, - {"noconfirm", no_argument, 0, 1000}, - {"config", required_argument, 0, 1001}, - {"ignore", required_argument, 0, 1002}, - {"debug", optional_argument, 0, 1003}, - {"noprogressbar", no_argument, 0, 1004}, - {"noscriptlet", no_argument, 0, 1005}, - {"ask", required_argument, 0, 1006}, - {"cachedir", required_argument, 0, 1007}, - {"asdeps", no_argument, 0, 1008}, - {"logfile", required_argument, 0, 1009}, - {"ignoregroup", required_argument, 0, 1010}, - {"needed", no_argument, 0, 1011}, - {"asexplicit", no_argument, 0, 1012}, - {"arch", required_argument, 0, 1013}, + {"noconfirm", no_argument, 0, OP_NOCONFIRM}, + {"config", required_argument, 0, OP_CONFIG}, + {"ignore", required_argument, 0, OP_IGNORE}, + {"debug", optional_argument, 0, OP_DEBUG}, + {"noprogressbar", no_argument, 0, OP_NOPROGRESSBAR}, + {"noscriptlet", no_argument, 0, OP_NOSCRIPTLET}, + {"ask", required_argument, 0, OP_ASK}, + {"cachedir", required_argument, 0, OP_CACHEDIR}, + {"asdeps", no_argument, 0, OP_ASDEPS}, + {"logfile", required_argument, 0, OP_LOGFILE}, + {"ignoregroup", required_argument, 0, OP_IGNOREGROUP}, + {"needed", no_argument, 0, OP_NEEDED}, + {"asexplicit", no_argument, 0, OP_ASEXPLICIT}, + {"arch", required_argument, 0, OP_ARCH}, {0, 0, 0, 0} }; @@ -403,21 +403,21 @@ static int parseargs(int argc, char *argv[]) } switch(opt) { case 0: break; - case 1000: config->noconfirm = 1; break; - case 1001: + case OP_NOCONFIRM: config->noconfirm = 1; break; + case OP_CONFIG: if(config->configfile) { free(config->configfile); } config->configfile = strndup(optarg, PATH_MAX); break; - case 1002: + case OP_IGNORE: list = strsplit(optarg, ','); for(item = list; item; item = alpm_list_next(item)) { alpm_option_add_ignorepkg((char *)alpm_list_getdata(item)); } FREELIST(list); break; - case 1003: + case OP_DEBUG: /* debug levels are made more 'human readable' than using a raw logmask * here, error and warning are set in config_new, though perhaps a * --quiet option will remove these later */ @@ -440,34 +440,34 @@ static int parseargs(int argc, char *argv[]) /* progress bars get wonky with debug on, shut them off */ config->noprogressbar = 1; break; - case 1004: config->noprogressbar = 1; break; - case 1005: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; - case 1006: config->noask = 1; config->ask = atoi(optarg); break; - case 1007: + case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; + case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; + case OP_ASK: config->noask = 1; config->ask = atoi(optarg); break; + case OP_CACHEDIR: if(alpm_option_add_cachedir(optarg) != 0) { pm_printf(PM_LOG_ERROR, _("problem adding cachedir '%s' (%s)\n"), optarg, alpm_strerrorlast()); return(1); } break; - case 1008: + case OP_ASDEPS: config->flags |= PM_TRANS_FLAG_ALLDEPS; break; - case 1009: + case OP_LOGFILE: config->logfile = strndup(optarg, PATH_MAX); break; - case 1010: + case OP_IGNOREGROUP: list = strsplit(optarg, ','); for(item = list; item; item = alpm_list_next(item)) { alpm_option_add_ignoregrp((char *)alpm_list_getdata(item)); } FREELIST(list); break; - case 1011: config->flags |= PM_TRANS_FLAG_NEEDED; break; - case 1012: + case OP_NEEDED: config->flags |= PM_TRANS_FLAG_NEEDED; break; + case OP_ASEXPLICIT: config->flags |= PM_TRANS_FLAG_ALLEXPLICIT; break; - case 1013: + case OP_ARCH: setarch(optarg); break; case 'Q': config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_QUERY); break; -- 1.6.4.4
participants (3)
-
Dan McGee
-
Laszlo Papp
-
Xavier