[pacman-dev] [PATCH] Some options were refactored
./src/pacman/pacman.h: - The defines were placed here that are used in the pacman.c source file ./src/pacman/pacman.c - Long options were refactored because of a safer and more comfortable programming style, instead of hard coding 10-15 or more integer value into the code Signed-off-by: Laszlo Papp <djszapi@archlinux.us> --- src/pacman/pacman.c | 56 +++++++++++++++++++++++++------------------------- src/pacman/pacman.h | 16 ++++++++++++++ 2 files changed, 44 insertions(+), 28 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index ac51502..2d806d4 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, NOCONFIRM_OPTID}, + {"config", required_argument, 0, CONFIG_OPTID}, + {"ignore", required_argument, 0, IGNORE_OPTID}, + {"debug", optional_argument, 0, DEBUG_OPTID}, + {"noprogressbar", no_argument, 0, NOPROGRESSBAR_OPTID}, + {"noscriptlet", no_argument, 0, NOSCRIPTLET_OPTID}, + {"ask", required_argument, 0, ASK_OPTID}, + {"cachedir", required_argument, 0, CACHEDIR_OPTID}, + {"asdeps", no_argument, 0, ASDEPS_OPTID}, + {"logfile", required_argument, 0, LOGFILE_OPTID}, + {"ignoregroup", required_argument, 0, IGNOREGROUP_OPTID}, + {"needed", no_argument, 0, NEEDED_OPTID}, + {"asexplicit", no_argument, 0, ASEXPLICIT_OPTID}, + {"arch", required_argument, 0, ARCH_OPTID}, {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 NOCONFIRM_OPTID: config->noconfirm = 1; break; + case CONFIG_OPTID: if(config->configfile) { free(config->configfile); } config->configfile = strndup(optarg, PATH_MAX); break; - case 1002: + case IGNORE_OPTID: 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 DEBUG_OPTID: /* 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 NOPROGRESSBAR_OPTID: config->noprogressbar = 1; break; + case NOSCRIPTLET_OPTID: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; + case ASK_OPTID: config->noask = 1; config->ask = atoi(optarg); break; + case CACHEDIR_OPTID: 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 ASDEPS_OPTID: config->flags |= PM_TRANS_FLAG_ALLDEPS; break; - case 1009: + case LOGFILE_OPTID: config->logfile = strndup(optarg, PATH_MAX); break; - case 1010: + case IGNOREGROUP_OPTID: 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 NEEDED_OPTID: config->flags |= PM_TRANS_FLAG_NEEDED; break; + case ASEXPLICIT_OPTID: config->flags |= PM_TRANS_FLAG_ALLEXPLICIT; break; - case 1013: + case ARCH_OPTID: setarch(optarg); break; case 'Q': config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_QUERY); break; diff --git a/src/pacman/pacman.h b/src/pacman/pacman.h index d7cb50f..f278e49 100644 --- a/src/pacman/pacman.h +++ b/src/pacman/pacman.h @@ -22,6 +22,22 @@ #include <alpm_list.h> +#define NOCONFIRM_OPTID 1000 +#define CONFIG_OPTID 1001 +#define IGNORE_OPTID 1002 +#define DEBUG_OPTID 1003 +#define NOPROGRESSBAR_OPTID 1004 +#define NOSCRIPTLET_OPTID 1005 +#define ASK_OPTID 1006 +#define CACHEDIR_OPTID 1007 +#define ASDEPS_OPTID 1008 +#define LOGFILE_OPTID 1009 +#define IGNOREGROUP_OPTID 1010 +#define NEEDED_OPTID 1011 +#define ASEXPLICIT_OPTID 1012 +#define ARCH_OPTID 1013 + + /* query.c */ int pacman_query(alpm_list_t *targets); /* remove.c */ -- 1.6.4.4
Laszlo Papp wrote:
./src/pacman/pacman.h: - The defines were placed here that are used in the pacman.c source file ./src/pacman/pacman.c - Long options were refactored because of a safer and more comfortable programming style, instead of hard coding 10-15 or more integer value into the code
Please try and make better commit messages. The title tells me nothing about the patch and the description is not much better. Allan
On Thu, Sep 24, 2009 at 11:17 PM, Allan McRae <allan@archlinux.org> wrote:
Laszlo Papp wrote:
./src/pacman/pacman.h: - The defines were placed here that are used in the pacman.c source file ./src/pacman/pacman.c - Long options were refactored because of a safer and more comfortable programming style, instead of hard coding 10-15 or more integer value into the code
Please try and make better commit messages. The title tells me nothing about the patch and the description is not much better.
By way of example, I would have done something like: ----- Replace hardcoded option numbers with #defines pacman's long option parsing used hardcoded numbers to identify them. This is not good practice, so replace them with #defined constants. Signed-off-by: Skeletor -----
Laszlo Papp wrote:
./src/pacman/pacman.h: - The defines were placed here that are used in the pacman.c source file ./src/pacman/pacman.c - Long options were refactored because of a safer and more comfortable programming style, instead of hard coding 10-15 or more integer value into the code
Signed-off-by: Laszlo Papp <djszapi@archlinux.us> --- src/pacman/pacman.c | 56 +++++++++++++++++++++++++------------------------- src/pacman/pacman.h | 16 ++++++++++++++ 2 files changed, 44 insertions(+), 28 deletions(-)
<snip>
+#define NOCONFIRM_OPTID 1000 +#define CONFIG_OPTID 1001 +#define IGNORE_OPTID 1002 +#define DEBUG_OPTID 1003 +#define NOPROGRESSBAR_OPTID 1004 +#define NOSCRIPTLET_OPTID 1005 +#define ASK_OPTID 1006 +#define CACHEDIR_OPTID 1007 +#define ASDEPS_OPTID 1008 +#define LOGFILE_OPTID 1009 +#define IGNOREGROUP_OPTID 1010 +#define NEEDED_OPTID 1011 +#define ASEXPLICIT_OPTID 1012 +#define ARCH_OPTID 1013
This patch seems fine to me. You probably want to use spaces rather than tabs to align those values. Variable tabs sizes do not matter much for start of line code indentation but they do matter in the middle of lines. I'm not sure about the variable names. Would it be better if they all started the same? i.e. OPT_FOO. That just seems nicer stylistically to me. Allan
On Fri, Sep 25, 2009 at 8:09 AM, Allan McRae <allan@archlinux.org> wrote:
Laszlo Papp wrote:
./src/pacman/pacman.h: - The defines were placed here that are used in the pacman.c source file ./src/pacman/pacman.c - Long options were refactored because of a safer and more comfortable programming style, instead of hard coding 10-15 or more integer value into the code
Signed-off-by: Laszlo Papp <djszapi@archlinux.us> --- src/pacman/pacman.c | 56 +++++++++++++++++++++++++------------------------- src/pacman/pacman.h | 16 ++++++++++++++ 2 files changed, 44 insertions(+), 28 deletions(-)
<snip> +#define NOCONFIRM_OPTID 1000 +#define CONFIG_OPTID 1001 +#define IGNORE_OPTID 1002 +#define DEBUG_OPTID 1003 +#define NOPROGRESSBAR_OPTID 1004 +#define NOSCRIPTLET_OPTID 1005 +#define ASK_OPTID 1006 +#define CACHEDIR_OPTID 1007 +#define ASDEPS_OPTID 1008 +#define LOGFILE_OPTID 1009 +#define IGNOREGROUP_OPTID 1010 +#define NEEDED_OPTID 1011 +#define ASEXPLICIT_OPTID 1012 +#define ARCH_OPTID 1013
This patch seems fine to me. You probably want to use spaces rather than tabs to align those values. Variable tabs sizes do not matter much for start of line code indentation but they do matter in the middle of lines.
I'm not sure about the variable names. Would it be better if they all started the same? i.e. OPT_FOO. That just seems nicer stylistically to me.
Allan
And what about using an enum here ?
participants (4)
-
Aaron Griffin
-
Allan McRae
-
Laszlo Papp
-
Xavier