[pacman-dev] [PATCH 1/4] CLI args: add pactest with an invalid combination

newer
[pacman-dev] [PATCH 1/3] makepkg:...

Xavier Chantry

10 Oct 2010 10 Oct '10

7:06 p.m.

From: Jakob Gruber Pacman should catch cases in which the passed arguments don't apply to the current operation (sync/query/...) Also see FS#20950 --- test/pacman/tests/pacman005.py | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-) create mode 100644 test/pacman/tests/pacman005.py diff --git a/test/pacman/tests/pacman005.py b/test/pacman/tests/pacman005.py new file mode 100644 index 0000000..99c25b2 --- /dev/null +++ b/test/pacman/tests/pacman005.py @@ -0,0 +1,11 @@ +self.description = "Test invalid combination of command line options (-Qy)" + +p = pmpkg("foobar") +self.addpkg2db("local", p) + +self.args = "-Qy" + +self.addrule("PACMAN_RETCODE=1") + +self.expectfailure = True + -- 1.7.3.1

Show replies by date

Xavier Chantry

10 Oct 10 Oct

7:06 p.m.

New subject: [pacman-dev] [PATCH 2/4] CLI args: reorganize parsing

From: Jakob Gruber Split parsing of CLI arguments into separate functions: parsearg_op (operations) parsearg_global (global options) parsearg_{database,query,remove,sync,deptest,upgrade} Organization strictly follows the manpage (even where the manpage is incorrect) - these cases will be fixed in the following commits. Switch cases are copy/pasted and statements unrelated to chosen operation are deleted. Parsing logic adjusted as follows: 1) Parse operation 2) If we can bail out early (duplicate op, help/version requested) do so 3) Parse arguments again: foreach arg: if arg is operation: continue tryparse_args_specific_to_op if unsuccessful tryparse_args_global if unsuccessful print error message and exit --- src/pacman/pacman.c | 440 +++++++++++++++++++++++++--------------- test/pacman/tests/pacman005.py | 3 - 2 files changed, 280 insertions(+), 163 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 049bc40..f725071 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -352,6 +352,230 @@ static void setlibpaths(void) #define check_optarg() if(!optarg) { return(1); } +/** Helper function for parsing operation from command-line arguments. + * @param opt Keycode returned by getopt_long + * @param dryrun If nonzero, application state is NOT changed + * @return 0 if opt was handled, 1 if it was not handled + */ +static int parsearg_op(int opt, int dryrun) +{ + switch(opt) { + /* operations */ + case 'D': + if(dryrun) break; + config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_DATABASE); break; + case 'Q': + if(dryrun) break; + config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_QUERY); break; + case 'R': + if(dryrun) break; + config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_REMOVE); break; + case 'S': + if(dryrun) break; + config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_SYNC); break; + case 'T': + if(dryrun) break; + config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_DEPTEST); break; + case 'U': + if(dryrun) break; + config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_UPGRADE); break; + case 'V': + if(dryrun) break; + config->version = 1; break; + case 'h': + if(dryrun) break; + config->help = 1; break; + default: + return(1); + } + return(0); +} + +/** Helper functions for parsing command-line arguments. + * @param opt Keycode returned by getopt_long + * @return 0 on success, 1 on failure + */ +static int parsearg_global(int opt) +{ + switch(opt) { + case OP_ASDEPS: config->flags |= PM_TRANS_FLAG_ALLDEPS; break; + case OP_ASEXPLICIT: config->flags |= PM_TRANS_FLAG_ALLEXPLICIT; break; + case OP_ARCH: check_optarg(); setarch(optarg); break; + case OP_ASK: + check_optarg(); + config->noask = 1; + config->ask = atoi(optarg); + break; + case OP_CACHEDIR: + check_optarg(); + 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 OP_CONFIG: + check_optarg(); + if(config->configfile) { + free(config->configfile); + } + config->configfile = strndup(optarg, PATH_MAX); + break; + 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 */ + if(optarg) { + unsigned short debug = atoi(optarg); + switch(debug) { + case 2: + config->logmask |= PM_LOG_FUNCTION; /* fall through */ + case 1: + config->logmask |= PM_LOG_DEBUG; + break; + default: + pm_printf(PM_LOG_ERROR, _("'%s' is not a valid debug level\n"), + optarg); + return(1); + } + } else { + config->logmask |= PM_LOG_DEBUG; + } + /* progress bars get wonky with debug on, shut them off */ + config->noprogressbar = 1; + break; + case OP_LOGFILE: + check_optarg(); + config->logfile = strndup(optarg, PATH_MAX); + break; + case OP_NOCONFIRM: config->noconfirm = 1; break; + case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; + case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; + case OP_PRINTFORMAT: + check_optarg(); + config->print_format = strdup(optarg); + break; + case 'b': + check_optarg(); + config->dbpath = strdup(optarg); + break; + case 'd': + config->op_q_deps = 1; + config->flags |= PM_TRANS_FLAG_NODEPS; + break; + case 'f': config->flags |= PM_TRANS_FLAG_FORCE; break; + case 'p': config->op_q_isfile = 1; config->print = 1; break; + case 'r': check_optarg(); config->rootdir = strdup(optarg); break; + case 'v': (config->verbose)++; break; + default: return(1); + } + return(0); +} + +static int parsearg_database(int opt) +{ + switch(opt) { + default: return(1); + } + return(0); +} + +static int parsearg_query(int opt) +{ + switch(opt) { + case 'c': config->op_q_changelog = 1; break; + case 'e': config->op_q_explicit = 1; break; + case 'g': (config->group)++; break; + case 'i': (config->op_q_info)++; break; + case 'k': config->op_q_check = 1; break; + case 'l': config->op_q_list = 1; break; + case 'm': config->op_q_foreign = 1; break; + case 'o': config->op_q_owns = 1; break; + case 'q': config->quiet = 1; break; + case 's': config->op_q_search = 1; break; + case 't': config->op_q_unrequired = 1; break; + case 'u': config->op_q_upgrade = 1; break; + default: return(1); + } + return(0); +} + +static int parsearg_remove(int opt) +{ + switch(opt) { + case 'c': config->flags |= PM_TRANS_FLAG_CASCADE; break; + case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break; + case 'n': config->flags |= PM_TRANS_FLAG_NOSAVE; break; + case 's': + if(config->flags & PM_TRANS_FLAG_RECURSE) { + config->flags |= PM_TRANS_FLAG_RECURSEALL; + } else { + config->flags |= PM_TRANS_FLAG_RECURSE; + } + break; + case 'u': config->flags |= PM_TRANS_FLAG_UNNEEDED; break; + default: return(1); + } + return(0); +} + +static int parsearg_sync(int opt) +{ + alpm_list_t *list = NULL, *item = NULL; /* lists for splitting strings */ + + switch(opt) { + case OP_IGNORE: + check_optarg(); + 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 OP_IGNOREGROUP: + check_optarg(); + 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 OP_NEEDED: config->flags |= PM_TRANS_FLAG_NEEDED; break; + case 'c': (config->op_s_clean)++; break; + case 'g': (config->group)++; break; + case 'i': (config->op_s_info)++; break; + case 'l': config->op_q_list = 1; break; + case 'q': config->quiet = 1; break; + case 's': config->op_s_search = 1; break; + case 'u': (config->op_s_upgrade)++; break; + case 'w': + config->op_s_downloadonly = 1; + config->flags |= PM_TRANS_FLAG_DOWNLOADONLY; + config->flags |= PM_TRANS_FLAG_NOCONFLICTS; + break; + case 'y': (config->op_s_sync)++; break; + default: return(1); + } + return(0); +} + +static int parsearg_deptest(int opt) +{ + switch(opt) { + default: return(1); + } + return(0); +} + +static int parsearg_upgrade(int opt) +{ + switch(opt) { + case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break; + default: return(1); + } + return(0); +} + /** Parse command-line arguments for each operation. * @param argc argc * @param argv argv @@ -361,6 +585,8 @@ static int parseargs(int argc, char *argv[]) { int opt; int option_index = 0; + int result; + const char *optstring = "DQRSTUVb:cdefghiklmnopqr:stuvwy"; static struct option opts[] = { {"database", no_argument, 0, 'D'}, @@ -418,175 +644,22 @@ static int parseargs(int argc, char *argv[]) {0, 0, 0, 0} }; - while((opt = getopt_long(argc, argv, "DQRSTUVb:cdefghiklmnopqr:stuvwy", opts, &option_index))) { - alpm_list_t *list = NULL, *item = NULL; /* lists for splitting strings */ - + /* parse operation */ + while((opt = getopt_long(argc, argv, optstring, opts, &option_index))) { if(opt < 0) { break; + } else if(opt == 0) { + continue; + } else if(opt == '?') { + return(1); } - switch(opt) { - case 0: break; - case OP_NOCONFIRM: config->noconfirm = 1; break; - case OP_CONFIG: - check_optarg(); - if(config->configfile) { - free(config->configfile); - } - config->configfile = strndup(optarg, PATH_MAX); - break; - case OP_IGNORE: - check_optarg(); - 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 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 */ - if(optarg) { - unsigned short debug = atoi(optarg); - switch(debug) { - case 2: - config->logmask |= PM_LOG_FUNCTION; /* fall through */ - case 1: - config->logmask |= PM_LOG_DEBUG; - break; - default: - pm_printf(PM_LOG_ERROR, _("'%s' is not a valid debug level\n"), - optarg); - return(1); - } - } else { - config->logmask |= PM_LOG_DEBUG; - } - /* progress bars get wonky with debug on, shut them off */ - config->noprogressbar = 1; - break; - case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; - case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; - case OP_ASK: - check_optarg(); - config->noask = 1; - config->ask = atoi(optarg); - break; - case OP_CACHEDIR: - check_optarg(); - 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 OP_ASDEPS: - config->flags |= PM_TRANS_FLAG_ALLDEPS; - break; - case OP_LOGFILE: - check_optarg(); - config->logfile = strndup(optarg, PATH_MAX); - break; - case OP_IGNOREGROUP: - check_optarg(); - 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 OP_NEEDED: config->flags |= PM_TRANS_FLAG_NEEDED; break; - case OP_ASEXPLICIT: - config->flags |= PM_TRANS_FLAG_ALLEXPLICIT; - break; - case OP_ARCH: - check_optarg(); - setarch(optarg); - break; - case OP_PRINTFORMAT: - check_optarg(); - config->print_format = strdup(optarg); - break; - case 'D': config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_DATABASE); break; - case 'Q': config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_QUERY); break; - case 'R': config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_REMOVE); break; - case 'S': config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_SYNC); break; - case 'T': config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_DEPTEST); break; - case 'U': config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_UPGRADE); break; - case 'V': config->version = 1; break; - case 'b': - check_optarg(); - config->dbpath = strdup(optarg); - break; - case 'c': - (config->op_s_clean)++; - config->flags |= PM_TRANS_FLAG_CASCADE; - config->op_q_changelog = 1; - break; - case 'd': - config->op_q_deps = 1; - config->flags |= PM_TRANS_FLAG_NODEPS; - break; - case 'e': - config->op_q_explicit = 1; - break; - case 'f': config->flags |= PM_TRANS_FLAG_FORCE; break; - case 'g': (config->group)++; break; - case 'h': config->help = 1; break; - case 'i': (config->op_q_info)++; (config->op_s_info)++; break; - case 'k': - config->flags |= PM_TRANS_FLAG_DBONLY; - config->op_q_check = 1; - break; - case 'l': config->op_q_list = 1; break; - case 'm': config->op_q_foreign = 1; break; - case 'n': config->flags |= PM_TRANS_FLAG_NOSAVE; break; - case 'o': config->op_q_owns = 1; break; - case 'p': - config->op_q_isfile = 1; - config->print = 1; - break; - case 'q': - config->quiet = 1; - break; - case 'r': - check_optarg(); - config->rootdir = strdup(optarg); - break; - case 's': - config->op_s_search = 1; - config->op_q_search = 1; - if(config->flags & PM_TRANS_FLAG_RECURSE) { - config->flags |= PM_TRANS_FLAG_RECURSEALL; - } else { - config->flags |= PM_TRANS_FLAG_RECURSE; - } - break; - case 't': - config->op_q_unrequired = 1; - break; - case 'u': - (config->op_s_upgrade)++; - config->op_q_upgrade = 1; - config->flags |= PM_TRANS_FLAG_UNNEEDED; - break; - case 'v': (config->verbose)++; break; - case 'w': - config->op_s_downloadonly = 1; - config->flags |= PM_TRANS_FLAG_DOWNLOADONLY; - config->flags |= PM_TRANS_FLAG_NOCONFLICTS; - break; - case 'y': (config->op_s_sync)++; break; - case '?': return(1); - default: return(1); - } + parsearg_op(opt, 0); } if(config->op == 0) { pm_printf(PM_LOG_ERROR, _("only one operation may be used at a time\n")); return(1); } - if(config->help) { usage(config->op, mbasename(argv[0])); return(2); @@ -596,6 +669,53 @@ static int parseargs(int argc, char *argv[]) return(2); } + /* parse all other options */ + optind = 1; + while((opt = getopt_long(argc, argv, optstring, opts, &option_index))) { + + if(opt < 0) { + break; + } else if(opt == 0) { + continue; + } else if(opt == '?') { + return(1); + } else if(parsearg_op(opt, 1) == 0) { /* opt is an operation */ + continue; + } + + switch(config->op) { + case PM_OP_DATABASE: + result = parsearg_database(opt); + break; + case PM_OP_QUERY: + result = parsearg_query(opt); + break; + case PM_OP_REMOVE: + result = parsearg_remove(opt); + break; + case PM_OP_SYNC: + result = parsearg_sync(opt); + break; + case PM_OP_DEPTEST: + result = parsearg_deptest(opt); + break; + case PM_OP_UPGRADE: + result = parsearg_upgrade(opt); + break; + default: + return(1); + } + if(result != 0) { + /* operation-specific option parsing failed, fall back to global options */ + result = parsearg_global(opt); + if(result != 0) { + /* global option parsing failed, abort */ + pm_printf(PM_LOG_ERROR, _("invalid option\n")); + return(result); + } + } + } + while(optind < argc) { /* add the target to our target array */ pm_targets = alpm_list_add(pm_targets, strdup(argv[optind])); diff --git a/test/pacman/tests/pacman005.py b/test/pacman/tests/pacman005.py index 99c25b2..bb21ad4 100644 --- a/test/pacman/tests/pacman005.py +++ b/test/pacman/tests/pacman005.py @@ -6,6 +6,3 @@ self.args = "-Qy" self.addrule("PACMAN_RETCODE=1") - -self.expectfailure = True - -- 1.7.3.1

Xavier Chantry

7:06 p.m.

New subject: [pacman-dev] [PATCH 3/4] CLI args: stricter/better parsing

From: Jakob Gruber In the following, the letters SRUDQ refer to the corresponding pacman operations. Most of the work in this commit is about removing as many options as possible from the global section and moving them to where they actually belong. Additionally, --ignore{,group} are added to U and --dbonly is added to S. --dbonly added to S --asdeps moved to S/U/D --asexplicit moved to S/U/D --print-format moved to S/U/R --noprogressbar moved to S/U/R --noscriptlet moved to S/U/R --ignorepkg added to U --ignoregrp added to U -d moved to S/U/R (--nodeps) and Q (--deps) -p moved to S/U/R (--print) and Q (--file) -f moved to S/U --- doc/pacman.8.txt | 26 ++++++++++------- src/pacman/pacman.c | 77 +++++++++++++++++++++++++++++--------------------- 2 files changed, 60 insertions(+), 43 deletions(-) diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt index de1f51f..4a936c8 100644 --- a/doc/pacman.8.txt +++ b/doc/pacman.8.txt @@ -158,6 +158,16 @@ Options *\--config* <'file'>:: Specify an alternate configuration file. +*\--ignore* <'package'>:: + Directs pacman to ignore upgrades of package even if there is one + available. Multiple packages can be specified by separating them + with a comma. + +*\--ignoregroup* <'group'>:: + Directs pacman to ignore upgrades of all packages in 'group' even if + there is one available. Multiple groups can be specified by + separating them with a comma. + *\--logfile* <'file'>:: Specify an alternate log file. This is an absolute path, regardless of the installation root setting. @@ -321,6 +331,10 @@ linkman:pacman.conf[5]. or '-i' flags will also display those packages in all repositories that depend on this package. +*-k, \--dbonly*:: + Adds the database entries for the specified packages but does not install any + of the files. + *-l, \--list*:: List all packages in the specified repositories. Multiple repositories can be specified on the command line. @@ -365,21 +379,11 @@ linkman:pacman.conf[5]. *\--needed*:: Don't reinstall the targets that are already up-to-date. -*\--ignore* <'package'>:: - Directs pacman to ignore upgrades of package even if there is one - available. Multiple packages can be specified by separating them - with a comma. - -*\--ignoregroup* <'group'>:: - Directs pacman to ignore upgrades of all packages in 'group' even if - there is one available. Multiple groups can be specified by - separating them with a comma. - Upgrade Options[[UO]] -------------------- *-k, \--dbonly*:: - Adds the database entries for the specified packages but do not install any + Adds the database entries for the specified packages but does not install any of the files. On an upgrade operation, the existing package and all files will be removed and the database entry for the new package will be added. diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index f725071..15abecc 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -352,6 +352,21 @@ static void setlibpaths(void) #define check_optarg() if(!optarg) { return(1); } +typedef void (*fn_add) (const char *s); + +static int parsearg_util_addlist(fn_add fn) +{ + alpm_list_t *list = NULL, *item = NULL; /* lists for splitting strings */ + + check_optarg(); + list = strsplit(optarg, ','); + for(item = list; item; item = alpm_list_next(item)) { + fn((char *)alpm_list_getdata(item)); + } + FREELIST(list); + return(0); +} + /** Helper function for parsing operation from command-line arguments. * @param opt Keycode returned by getopt_long * @param dryrun If nonzero, application state is NOT changed @@ -398,8 +413,6 @@ static int parsearg_op(int opt, int dryrun) static int parsearg_global(int opt) { switch(opt) { - case OP_ASDEPS: config->flags |= PM_TRANS_FLAG_ALLDEPS; break; - case OP_ASEXPLICIT: config->flags |= PM_TRANS_FLAG_ALLEXPLICIT; break; case OP_ARCH: check_optarg(); setarch(optarg); break; case OP_ASK: check_optarg(); @@ -449,22 +462,10 @@ static int parsearg_global(int opt) config->logfile = strndup(optarg, PATH_MAX); break; case OP_NOCONFIRM: config->noconfirm = 1; break; - case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; - case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; - case OP_PRINTFORMAT: - check_optarg(); - config->print_format = strdup(optarg); - break; case 'b': check_optarg(); config->dbpath = strdup(optarg); break; - case 'd': - config->op_q_deps = 1; - config->flags |= PM_TRANS_FLAG_NODEPS; - break; - case 'f': config->flags |= PM_TRANS_FLAG_FORCE; break; - case 'p': config->op_q_isfile = 1; config->print = 1; break; case 'r': check_optarg(); config->rootdir = strdup(optarg); break; case 'v': (config->verbose)++; break; default: return(1); @@ -475,6 +476,8 @@ static int parsearg_global(int opt) static int parsearg_database(int opt) { switch(opt) { + case OP_ASDEPS: config->flags |= PM_TRANS_FLAG_ALLDEPS; break; + case OP_ASEXPLICIT: config->flags |= PM_TRANS_FLAG_ALLEXPLICIT; break; default: return(1); } return(0); @@ -484,6 +487,7 @@ static int parsearg_query(int opt) { switch(opt) { case 'c': config->op_q_changelog = 1; break; + case 'd': config->op_q_deps = 1; break; case 'e': config->op_q_explicit = 1; break; case 'g': (config->group)++; break; case 'i': (config->op_q_info)++; break; @@ -491,6 +495,7 @@ static int parsearg_query(int opt) case 'l': config->op_q_list = 1; break; case 'm': config->op_q_foreign = 1; break; case 'o': config->op_q_owns = 1; break; + case 'p': config->op_q_isfile = 1; break; case 'q': config->quiet = 1; break; case 's': config->op_q_search = 1; break; case 't': config->op_q_unrequired = 1; break; @@ -503,9 +508,14 @@ static int parsearg_query(int opt) static int parsearg_remove(int opt) { switch(opt) { + case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; + case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; + case OP_PRINTFORMAT: check_optarg(); config->print_format = strdup(optarg); break; case 'c': config->flags |= PM_TRANS_FLAG_CASCADE; break; + case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break; case 'n': config->flags |= PM_TRANS_FLAG_NOSAVE; break; + case 'p': config->print = 1; break; case 's': if(config->flags & PM_TRANS_FLAG_RECURSE) { config->flags |= PM_TRANS_FLAG_RECURSEALL; @@ -521,30 +531,23 @@ static int parsearg_remove(int opt) static int parsearg_sync(int opt) { - alpm_list_t *list = NULL, *item = NULL; /* lists for splitting strings */ - switch(opt) { - case OP_IGNORE: - check_optarg(); - 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 OP_IGNOREGROUP: - check_optarg(); - 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 OP_ASDEPS: config->flags |= PM_TRANS_FLAG_ALLDEPS; break; + case OP_ASEXPLICIT: config->flags |= PM_TRANS_FLAG_ALLEXPLICIT; break; + case OP_IGNORE: parsearg_util_addlist(alpm_option_add_ignorepkg); break; + case OP_IGNOREGROUP: parsearg_util_addlist(alpm_option_add_ignoregrp); break; case OP_NEEDED: config->flags |= PM_TRANS_FLAG_NEEDED; break; + case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; + case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; + case OP_PRINTFORMAT: check_optarg(); config->print_format = strdup(optarg); break; case 'c': (config->op_s_clean)++; break; + case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; + case 'f': config->flags |= PM_TRANS_FLAG_FORCE; break; case 'g': (config->group)++; break; case 'i': (config->op_s_info)++; break; case 'l': config->op_q_list = 1; break; + case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break; + case 'p': config->print = 1; break; case 'q': config->quiet = 1; break; case 's': config->op_s_search = 1; break; case 'u': (config->op_s_upgrade)++; break; @@ -570,7 +573,17 @@ static int parsearg_deptest(int opt) static int parsearg_upgrade(int opt) { switch(opt) { + case OP_ASDEPS: config->flags |= PM_TRANS_FLAG_ALLDEPS; break; + case OP_ASEXPLICIT: config->flags |= PM_TRANS_FLAG_ALLEXPLICIT; break; + case OP_IGNORE: parsearg_util_addlist(alpm_option_add_ignorepkg); break; + case OP_IGNOREGROUP: parsearg_util_addlist(alpm_option_add_ignoregrp); break; + case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; + case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; + case OP_PRINTFORMAT: check_optarg(); config->print_format = strdup(optarg); break; + case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; + case 'f': config->flags |= PM_TRANS_FLAG_FORCE; break; case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break; + case 'p': config->print = 1; break; default: return(1); } return(0); -- 1.7.3.1

Xavier Chantry

7:06 p.m.

New subject: [pacman-dev] [PATCH 4/4] CLI args: update --help and manpage

The three parts (help, manpage and code) are now organized in the same way and much easier to compare : - specific options - install/upgrade options for -S and -U - transaction options for -S -R and -U - global options After this re-organization, it was easy to update and sync the three components together. Duplication is also avoided. Signed-off-by: Xavier Chantry --- doc/pacman.8.txt | 110 ++++++++++++++++------------------ src/pacman/pacman.c | 164 +++++++++++++++++++++++++-------------------------- 2 files changed, 133 insertions(+), 141 deletions(-) diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt index 4a936c8..f389dfd 100644 --- a/doc/pacman.8.txt +++ b/doc/pacman.8.txt @@ -104,35 +104,12 @@ to determine which packages need upgrading. This behavior operates as follows: Options ------- -*\--asdeps*:: - Install packages non-explicitly; in other words, fake their install reason - to be installed as a dependency. This is useful for makepkg and other - build from source tools that need to install dependencies before building - the package. - -*\--asexplicit*:: - Install packages explicitly; in other words, fake their install reason to - be explicitly installed. This is useful if you want to mark a dependency - as explicitly installed so it will not be removed by the '\--recursive' - remove operation. - *-b, \--dbpath* <'path'>:: Specify an alternative database location (a typical default is ``/var/lib/pacman''). This should not be used unless you know what you are doing. *NOTE*: if specified, this is an absolute path and the root path is not automatically prepended. -*-d, \--nodeps*:: - Skips all dependency checks. Normally, pacman will always check a - package's dependency fields to ensure that all dependencies are - installed and there are no package conflicts in the system. - -*-f, \--force*:: - Bypass file conflict checks and overwrite conflicting files. If the - package that is about to be installed contains files that are already - installed, this option will cause all those files to be overwritten. - This option should be used with care, ideally not at all. - *-r, \--root* <'path'>:: Specify an alternative installation root (default is ``/''). This should not be used as a way to install software into ``/usr/local'' instead of @@ -145,9 +122,8 @@ Options *-v, \--verbose*:: Output paths such as as the Root, Conf File, DB Path, Cache Dirs, etc. -*\--debug*:: - Display debug messages. When reporting bugs, this option is recommended - to be used. +*\--arch* <'arch'>:: + Specify an alternate architecture. *\--cachedir* <'dir'>:: Specify an alternative package cache location (a typical default is @@ -158,15 +134,9 @@ Options *\--config* <'file'>:: Specify an alternate configuration file. -*\--ignore* <'package'>:: - Directs pacman to ignore upgrades of package even if there is one - available. Multiple packages can be specified by separating them - with a comma. - -*\--ignoregroup* <'group'>:: - Directs pacman to ignore upgrades of all packages in 'group' even if - there is one available. Multiple groups can be specified by - separating them with a comma. +*\--debug*:: + Display debug messages. When reporting bugs, this option is recommended + to be used. *\--logfile* <'file'>:: Specify an alternate log file. This is an absolute path, regardless of @@ -176,6 +146,18 @@ Options Bypass any and all ``Are you sure?'' messages. It's not a good idea to do this unless you want to run pacman from a script. +Transaction Options (apply to '-S', '-R' and '-U') +-------------------------------------------------- +*-d, \--nodeps*:: + Skips all dependency checks. Normally, pacman will always check a + package's dependency fields to ensure that all dependencies are + installed and there are no package conflicts in the system. + +*-k, \--dbonly*:: + Adds/Removes the database entry only, leaves all files in place. + On an upgrade operation, the existing package and all files + will be removed and the database entry for the new package will be added. + *\--noprogressbar*:: Do not show a progress bar when downloading files. This can be useful for scripts that call pacman and capture the output. @@ -184,19 +166,46 @@ Options If an install scriptlet exists, do not execute it. Do not use this unless you know what you are doing. -*\--arch* <'arch'>:: - Specify an alternate architecture. - *-p, \--print*:: Only print the targets instead of performing the actual operation (sync, - remove or upgrade). Use '\--print-format' to specify how targets are - displayed. The default format string is "%l", which displays url with '-S', - filename with '-U' and pkgname-pkgver with '-R'. + remove or upgrade). Use '\--print-format' to specify how targets are + displayed. The default format string is "%l", which displays url with + '-S', filename with '-U' and pkgname-pkgver with '-R'. *\--print-format* <'format'>:: Specify a printf-like format to control the output of the '\--print' - operation. The possible are attributes are : %n for pkgname, %v for pkgver, %l - for location, %r for repo and %s for size. + operation. The possible are attributes are : %n for pkgname, %v for pkgver, + %l for location, %r for repo and %s for size. + +Upgrade Options (apply to 'S' and 'U')[[UO]] +-------------------------------------------- +*-f, \--force*:: + Bypass file conflict checks and overwrite conflicting files. If the + package that is about to be installed contains files that are already + installed, this option will cause all those files to be overwritten. + This option should be used with care, ideally not at all. + +*\--asdeps*:: + Install packages non-explicitly; in other words, fake their install reason + to be installed as a dependency. This is useful for makepkg and other + build from source tools that need to install dependencies before building + the package. + +*\--asexplicit*:: + Install packages explicitly; in other words, fake their install reason to + be explicitly installed. This is useful if you want to mark a dependency + as explicitly installed so it will not be removed by the '\--recursive' + remove operation. + +*\--ignore* <'package'>:: + Directs pacman to ignore upgrades of package even if there is one + available. Multiple packages can be specified by separating them + with a comma. + +*\--ignoregroup* <'group'>:: + Directs pacman to ignore upgrades of all packages in 'group' even if + there is one available. Multiple groups can be specified by + separating them with a comma. Query Options[[QO]] ------------------- @@ -284,9 +293,6 @@ Remove Options[[RO]] or more target packages. This operation is recursive, and must be used with care since it can remove many potentially needed packages. -*-k, \--dbonly*:: - Removes the database entry only. Leaves all files in place. - *-n, \--nosave*:: Instructs pacman to ignore file backup designations. Normally, when a file is removed from the system the database is checked to see if the @@ -331,10 +337,6 @@ linkman:pacman.conf[5]. or '-i' flags will also display those packages in all repositories that depend on this package. -*-k, \--dbonly*:: - Adds the database entries for the specified packages but does not install any - of the files. - *-l, \--list*:: List all packages in the specified repositories. Multiple repositories can be specified on the command line. @@ -380,14 +382,6 @@ linkman:pacman.conf[5]. Don't reinstall the targets that are already up-to-date. -Upgrade Options[[UO]] --------------------- -*-k, \--dbonly*:: - Adds the database entries for the specified packages but does not install any - of the files. On an upgrade operation, the existing package and all files - will be removed and the database entry for the new package will be added. - - Handling Config Files[[HCF]] ---------------------------- Pacman uses the same logic as rpm to determine action against files that are diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 15abecc..933698e 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -90,26 +90,14 @@ static void usage(int op, const char * const myname) printf("%s: %s {-R --remove} [%s] <%s>\n", str_usg, myname, str_opt, str_pkg); printf("%s:\n", str_opt); printf(_(" -c, --cascade remove packages and all packages that depend on them\n")); - printf(_(" -d, --nodeps skip dependency checks\n")); printf(_(" -k, --dbonly only remove database entries, do not remove files\n")); printf(_(" -n, --nosave remove configuration files as well\n")); printf(_(" -s, --recursive remove dependencies also (that won't break packages)\n" " (-ss includes explicitly installed dependencies too)\n")); printf(_(" -u, --unneeded remove unneeded packages (that won't break packages)\n")); - printf(_(" --print only print the targets instead of performing the operation\n")); - printf(_(" --print-format <string>\n" - " specify how the targets should be printed\n")); } else if(op == PM_OP_UPGRADE) { printf("%s: %s {-U --upgrade} [%s] <%s>\n", str_usg, myname, str_opt, str_file); printf("%s:\n", str_opt); - printf(_(" --asdeps install packages as non-explicitly installed\n")); - printf(_(" --asexplicit install packages as explicitly installed\n")); - printf(_(" -d, --nodeps skip dependency checks\n")); - printf(_(" -f, --force force install, overwrite conflicting files\n")); - printf(_(" -k, --dbonly add database entries, do not install or keep existing files\n")); - printf(_(" --print only print the targets instead of performing the operation\n")); - printf(_(" --print-format <string>\n" - " specify how the targets should be printed\n")); } else if(op == PM_OP_QUERY) { printf("%s: %s {-Q --query} [%s] [%s]\n", str_usg, myname, str_opt, str_pkg); printf("%s:\n", str_opt); @@ -123,50 +111,59 @@ static void usage(int op, const char * const myname) printf(_(" -m, --foreign list installed packages not found in sync db(s) [filter]\n")); printf(_(" -o, --owns <file> query the package that owns <file>\n")); printf(_(" -p, --file <package> query a package file instead of the database\n")); + printf(_(" -q, --quiet show less information for query and search\n")); printf(_(" -s, --search <regex> search locally-installed packages for matching strings\n")); printf(_(" -t, --unrequired list packages not required by any package [filter]\n")); printf(_(" -u, --upgrades list outdated packages [filter]\n")); - printf(_(" -q, --quiet show less information for query and search\n")); } else if(op == PM_OP_SYNC) { printf("%s: %s {-S --sync} [%s] [%s]\n", str_usg, myname, str_opt, str_pkg); printf("%s:\n", str_opt); - printf(_(" --asdeps install packages as non-explicitly installed\n")); - printf(_(" --asexplicit install packages as explicitly installed\n")); printf(_(" -c, --clean remove old packages from cache directory (-cc for all)\n")); - printf(_(" -d, --nodeps skip dependency checks\n")); - printf(_(" -f, --force force install, overwrite conflicting files\n")); printf(_(" -g, --groups view all members of a package group\n")); printf(_(" -i, --info view package information\n")); printf(_(" -l, --list <repo> view a list of packages in a repo\n")); + printf(_(" -q, --quiet show less information for query and search\n")); printf(_(" -s, --search <regex> search remote repositories for matching strings\n")); printf(_(" -u, --sysupgrade upgrade installed packages (-uu allows downgrade)\n")); printf(_(" -w, --downloadonly download packages but do not install/upgrade anything\n")); printf(_(" -y, --refresh download fresh package databases from the server\n")); printf(_(" --needed don't reinstall up to date packages\n")); - printf(_(" --ignore <pkg> ignore a package upgrade (can be used more than once)\n")); - printf(_(" --ignoregroup <grp>\n" - " ignore a group upgrade (can be used more than once)\n")); - printf(_(" --print only print the targets instead of performing the operation\n")); - printf(_(" --print-format <string>\n" - " specify how the targets should be printed\n")); - printf(_(" -q, --quiet show less information for query and search\n")); } else if (op == PM_OP_DATABASE) { printf("%s: %s {-D --database} <%s> <%s>\n", str_usg, myname, str_opt, str_pkg); printf("%s:\n", str_opt); printf(_(" --asdeps mark packages as non-explicitly installed\n")); printf(_(" --asexplicit mark packages as explicitly installed\n")); } + switch(op) { + case PM_OP_SYNC: + case PM_OP_UPGRADE: + printf(_(" -f, --force force install, overwrite conflicting files\n")); + printf(_(" -k, --dbonly add database entries, do not install or keep existing files\n")); + printf(_(" --asdeps install packages as non-explicitly installed\n")); + printf(_(" --asexplicit install packages as explicitly installed\n")); + printf(_(" --ignore <pkg> ignore a package upgrade (can be used more than once)\n")); + printf(_(" --ignoregroup <grp>\n" + " ignore a group upgrade (can be used more than once)\n")); + /* pass through */ + case PM_OP_REMOVE: + printf(_(" -d, --nodeps skip dependency checks\n")); + printf(_(" --noprogressbar do not show a progress bar when downloading files\n")); + printf(_(" --noscriptlet do not execute the install scriptlet if one exists\n")); + printf(_(" --print only print the targets instead of performing the operation\n")); + printf(_(" --print-format <string>\n" + " specify how the targets should be printed\n")); + break; + } + + printf(_(" -b, --dbpath <path> set an alternate database location\n")); + printf(_(" -r, --root <path> set an alternate installation root\n")); + printf(_(" -v, --verbose be verbose\n")); + printf(_(" --arch <arch> set an alternate architecture\n")); + printf(_(" --cachedir <dir> set an alternate package cache location\n")); printf(_(" --config <path> set an alternate configuration file\n")); + printf(_(" --debug display debug messages\n")); printf(_(" --logfile <path> set an alternate log file\n")); printf(_(" --noconfirm do not ask for any confirmation\n")); - printf(_(" --noprogressbar do not show a progress bar when downloading files\n")); - printf(_(" --noscriptlet do not execute the install scriptlet if one exists\n")); - printf(_(" -v, --verbose be verbose\n")); - printf(_(" --debug display debug messages\n")); - printf(_(" -r, --root <path> set an alternate installation root\n")); - printf(_(" -b, --dbpath <path> set an alternate database location\n")); - printf(_(" --cachedir <dir> set an alternate package cache location\n")); - printf(_(" --arch <arch> set an alternate architecture\n")); } } @@ -505,17 +502,31 @@ static int parsearg_query(int opt) return(0); } -static int parsearg_remove(int opt) +/* options common to -S -R -U */ +static int parsearg_trans(int opt) { switch(opt) { + case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; + case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break; case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; - case OP_PRINTFORMAT: check_optarg(); config->print_format = strdup(optarg); break; + case 'p': config->print = 1; break; + case OP_PRINTFORMAT: + check_optarg(); + config->print_format = strdup(optarg); + break; + default: return(1); + } + return(0); +} + +static int parsearg_remove(int opt) +{ + if (parsearg_trans(opt) == 0) + return(0); + switch(opt) { case 'c': config->flags |= PM_TRANS_FLAG_CASCADE; break; - case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; - case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break; case 'n': config->flags |= PM_TRANS_FLAG_NOSAVE; break; - case 'p': config->print = 1; break; case 's': if(config->flags & PM_TRANS_FLAG_RECURSE) { config->flags |= PM_TRANS_FLAG_RECURSEALL; @@ -529,25 +540,36 @@ static int parsearg_remove(int opt) return(0); } -static int parsearg_sync(int opt) +/* options common to -S -U */ +static int parsearg_upgrade(int opt) { + if (parsearg_trans(opt) == 0) + return(0); switch(opt) { + case 'f': config->flags |= PM_TRANS_FLAG_FORCE; break; case OP_ASDEPS: config->flags |= PM_TRANS_FLAG_ALLDEPS; break; case OP_ASEXPLICIT: config->flags |= PM_TRANS_FLAG_ALLEXPLICIT; break; - case OP_IGNORE: parsearg_util_addlist(alpm_option_add_ignorepkg); break; - case OP_IGNOREGROUP: parsearg_util_addlist(alpm_option_add_ignoregrp); break; + case OP_IGNORE: + parsearg_util_addlist(alpm_option_add_ignorepkg); + break; + case OP_IGNOREGROUP: + parsearg_util_addlist(alpm_option_add_ignoregrp); + break; + default: return(1); + } + return(0); +} + +static int parsearg_sync(int opt) +{ + if (parsearg_upgrade(opt) == 0) + return(0); + switch(opt) { case OP_NEEDED: config->flags |= PM_TRANS_FLAG_NEEDED; break; - case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; - case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; - case OP_PRINTFORMAT: check_optarg(); config->print_format = strdup(optarg); break; case 'c': (config->op_s_clean)++; break; - case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; - case 'f': config->flags |= PM_TRANS_FLAG_FORCE; break; case 'g': (config->group)++; break; case 'i': (config->op_s_info)++; break; case 'l': config->op_q_list = 1; break; - case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break; - case 'p': config->print = 1; break; case 'q': config->quiet = 1; break; case 's': config->op_s_search = 1; break; case 'u': (config->op_s_upgrade)++; break; @@ -562,33 +584,6 @@ static int parsearg_sync(int opt) return(0); } -static int parsearg_deptest(int opt) -{ - switch(opt) { - default: return(1); - } - return(0); -} - -static int parsearg_upgrade(int opt) -{ - switch(opt) { - case OP_ASDEPS: config->flags |= PM_TRANS_FLAG_ALLDEPS; break; - case OP_ASEXPLICIT: config->flags |= PM_TRANS_FLAG_ALLEXPLICIT; break; - case OP_IGNORE: parsearg_util_addlist(alpm_option_add_ignorepkg); break; - case OP_IGNOREGROUP: parsearg_util_addlist(alpm_option_add_ignoregrp); break; - case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; - case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; - case OP_PRINTFORMAT: check_optarg(); config->print_format = strdup(optarg); break; - case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; - case 'f': config->flags |= PM_TRANS_FLAG_FORCE; break; - case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break; - case 'p': config->print = 1; break; - default: return(1); - } - return(0); -} - /** Parse command-line arguments for each operation. * @param argc argc * @param argv argv @@ -664,6 +659,7 @@ static int parseargs(int argc, char *argv[]) } else if(opt == 0) { continue; } else if(opt == '?') { + /* unknown option, getopt printed an error */ return(1); } parsearg_op(opt, 0); @@ -685,12 +681,12 @@ static int parseargs(int argc, char *argv[]) /* parse all other options */ optind = 1; while((opt = getopt_long(argc, argv, optstring, opts, &option_index))) { - if(opt < 0) { break; } else if(opt == 0) { continue; } else if(opt == '?') { + /* this should have failed during first pass already */ return(1); } else if(parsearg_op(opt, 1) == 0) { /* opt is an operation */ continue; @@ -710,22 +706,24 @@ static int parseargs(int argc, char *argv[]) result = parsearg_sync(opt); break; case PM_OP_DEPTEST: - result = parsearg_deptest(opt); + result = 1; break; case PM_OP_UPGRADE: result = parsearg_upgrade(opt); break; default: + pm_printf(PM_LOG_ERROR, _("no operation specified (use -h for help)\n")); return(1); } + if (result == 0) + continue; + + /* fall back to global options */ + result = parsearg_global(opt); if(result != 0) { - /* operation-specific option parsing failed, fall back to global options */ - result = parsearg_global(opt); - if(result != 0) { - /* global option parsing failed, abort */ - pm_printf(PM_LOG_ERROR, _("invalid option\n")); - return(result); - } + /* global option parsing failed, abort */ + pm_printf(PM_LOG_ERROR, _("invalid option\n")); + return(result); } } -- 1.7.3.1

Allan McRae

11 Oct 11 Oct

1:30 a.m.

New subject: [pacman-dev] [PATCH 4/4] CLI args: update --help and manpage

On 11/10/10 05:06, Xavier Chantry wrote:

...

The three parts (help, manpage and code) are now organized in the same way and much easier to compare : - specific options - install/upgrade options for -S and -U - transaction options for -S -R and -U - global options

After this re-organization, it was easy to update and sync the three components together. Duplication is also avoided.

Signed-off-by: Xavier Chantry

...

./src/pacman/pacman -D --help usage: lt-pacman {-D --database} <options>

This had actually been on my TODO list for a while so it is good to see it fixed. Overall, I think these look good. One small point: options: --asdeps mark packages as non-explicitly installed --asexplicit mark packages as explicitly installed -b, --dbpath <path> set an alternate database location -r, --root <path> set an alternate installation root -v, --verbose be verbose --arch <arch> set an alternate architecture --cachedir <dir> set an alternate package cache location --config <path> set an alternate configuration file --debug display debug messages --logfile <path> set an alternate log file --noconfirm do not ask for any confirmation On my TODO list, I had also had removing --arch and --cachedir from -D and -Q as they are useless options there as far as I can tell. And --cachedir from -U. Slightly related, I wonder if we can shorten the description of "-k, --dbonly" and "--print" so they do not loop on an 80 width terminal. Or maybe add some line wrapping. Allan

Xavier Chantry

11:34 a.m.

New subject: [pacman-dev] [PATCH 4/4] CLI args: update --help and manpage

On Mon, Oct 11, 2010 at 3:30 AM, Allan McRae wrote:

...

On 11/10/10 05:06, Xavier Chantry wrote:

...
The three parts (help, manpage and code) are now organized in the same way and much easier to compare : - specific options - install/upgrade options for -S and -U - transaction options for -S -R and -U - global options

After this re-organization, it was easy to update and sync the three components together. Duplication is also avoided.

Signed-off-by: Xavier Chantry

This had actually been on my TODO list for a while so it is good to see it fixed. Overall, I think these look good. One small point:

...
./src/pacman/pacman -D --help usage: lt-pacman {-D --database} <options> options: --asdeps mark packages as non-explicitly installed --asexplicit mark packages as explicitly installed -b, --dbpath <path> set an alternate database location -r, --root <path> set an alternate installation root -v, --verbose be verbose --arch <arch> set an alternate architecture --cachedir <dir> set an alternate package cache location --config <path> set an alternate configuration file --debug display debug messages --logfile <path> set an alternate log file --noconfirm do not ask for any confirmation

On my TODO list, I had also had removing --arch and --cachedir from -D and -Q as they are useless options there as far as I can tell. And --cachedir from -U.

I hesitated about arch, but it looked like a general configuration option that would not hurt even for ops where it is not used. Same for cachedir. But also same for ignore, which is also specified in pacman.conf Jakob actually moved Ignore from just -S operation to global, which is also specified in pacman.conf I believe there is a difference between ignore and arch/cachedir : - arch/cachedir clearly have no impact on -D or -Q, they are just useless but can be mentioned - ignore/ignoregroup could have an impact on -D, -Q or -R but they don't have any, which might not be obvious That said, I am fine with moving stuff around, I will let Dan have the final vote on where arch cachedir and ignore should be.

...

Slightly related, I wonder if we can shorten the description of "-k, --dbonly" and "--print" so they do not loop on an 80 width terminal. Or maybe add some line wrapping.

Well I was looking at french translation, where half the description do not even fit a 100width terminal, so I don't really care :p But now that you mention dbonly, is this behavior really fine : -k, --dbonly Adds/Removes the database entry only, leaves all files in place. On an upgrade operation, the existing package and all files will be removed and the database entry for the new package will be added. I.E. when -S and -U are upgrade and not install, and they remove the existing local package, dbonly does not apply to the removal. files are actually deleted. This is quite weird for a 'dbonly' option, isn't it ? What if you want to use 'dbonly' because you modified all the package files (for example manual make install), but you just want to use and update the db entry with an arch package. There might be one case where I could actually need that. But as it is, dbonly looks quite weird and useless. the patch would just be 3 lines : diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index dfaba03..e0099fb 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -311,6 +311,9 @@ int _alpm_upgraderemove_package(pmpkg_t *oldpkg, pmpkg_t *newpkg, pmtrans_t *tra _alpm_log(PM_LOG_DEBUG, "removing old package first (%s-%s)\n", oldpkg->name, oldpkg->version); + if(trans->flags & PM_TRANS_FLAG_DBONLY) + goto db; + /* copy the remove skiplist over */ skip_remove = alpm_list_join(alpm_list_strdup(trans->skip_remove),alpm_list_strdup(handle->noupgrade)); @@ -345,6 +348,7 @@ int _alpm_upgraderemove_package(pmpkg_t *oldpkg, pmpkg_t *newpkg, pmtrans_t *tra alpm_list_free(newfiles); FREELIST(skip_remove); +db: /* remove the package from the database */ _alpm_log(PM_LOG_DEBUG, "updating database\n"); _alpm_log(PM_LOG_DEBUG, "removing database entry '%s'\n", pkgname); then the help and the description would also be shorter :)

Dan McGee

12 Oct 12 Oct

2:04 a.m.

New subject: [pacman-dev] [PATCH 4/4] CLI args: update --help and manpage

On Sun, Oct 10, 2010 at 8:30 PM, Allan McRae wrote:

...

On 11/10/10 05:06, Xavier Chantry wrote:

...
The three parts (help, manpage and code) are now organized in the same way and much easier to compare : - specific options - install/upgrade options for -S and -U - transaction options for -S -R and -U - global options

After this re-organization, it was easy to update and sync the three components together. Duplication is also avoided.

Signed-off-by: Xavier Chantry

This had actually been on my TODO list for a while so it is good to see it fixed. Overall, I think these look good. One small point:

...
./src/pacman/pacman -D --help usage: lt-pacman {-D --database} <options> options: --asdeps mark packages as non-explicitly installed --asexplicit mark packages as explicitly installed -b, --dbpath <path> set an alternate database location -r, --root <path> set an alternate installation root -v, --verbose be verbose --arch <arch> set an alternate architecture --cachedir <dir> set an alternate package cache location --config <path> set an alternate configuration file --debug display debug messages --logfile <path> set an alternate log file --noconfirm do not ask for any confirmation

On my TODO list, I had also had removing --arch and --cachedir from -D and -Q as they are useless options there as far as I can tell. And --cachedir from -U.

Slightly related, I wonder if we can shorten the description of "-k, --dbonly" and "--print" so they do not loop on an 80 width terminal. Or maybe add some line wrapping.

I'm not too thrilled abuot this patch for the fact that we lost alpha ordering by shortopt for sync/upgrade/remove options. This is a step backwards in my opinion. I can deal with the dbpath/root/every single op uses these things being at the bottom (and I think a blank line separating them would be good too), but the list seems very mismanaged for the main ops now. I pulled the other two patches in, but not this one. -Dan

Xavier Chantry

8:34 a.m.

New subject: [pacman-dev] [PATCH 4/4] CLI args: update --help and manpage

On Tue, Oct 12, 2010 at 4:04 AM, Dan McGee wrote:

...

I'm not too thrilled abuot this patch for the fact that we lost alpha ordering by shortopt for sync/upgrade/remove options. This is a step backwards in my opinion.

I can deal with the dbpath/root/every single op uses these things being at the bottom (and I think a blank line separating them would be good too), but the list seems very mismanaged for the main ops now.

I pulled the other two patches in, but not this one.

usage: pacman {-S --sync} [options] [package(s)] options: --asdeps install packages as non-explicitly installed --asexplicit install packages as explicitly installed -c, --clean remove old packages from cache directory (-cc for all) -d, --nodeps skip dependency checks -f, --force force install, overwrite conflicting files -g, --groups view all members of a package group -i, --info view package information -l, --list <repo> view a list of packages in a repo -s, --search <regex> search remote repositories for matching strings -u, --sysupgrade upgrade installed packages (-uu allows downgrade) -w, --downloadonly download packages but do not install/upgrade anything -y, --refresh download fresh package databases from the server --needed don't reinstall up to date packages --ignore <pkg> ignore a package upgrade (can be used more than once) --ignoregroup <grp> ignore a group upgrade (can be used more than once) --print only print the targets instead of performing the operation --print-format <string> specify how the targets should be printed -q, --quiet show less information for query and search --config <path> set an alternate configuration file --logfile <path> set an alternate log file --noconfirm do not ask for any confirmation --noprogressbar do not show a progress bar when downloading files --noscriptlet do not execute the install scriptlet if one exists -v, --verbose be verbose --debug display debug messages -r, --root <path> set an alternate installation root -b, --dbpath <path> set an alternate database location --cachedir <dir> set an alternate package cache location --arch <arch> set an alternate architecture I don't understand why it is fine to have -q -v -r -b mixed up like that for the global section, but not have two other similar sections, which makes it much nicer to check consistency of code, --help and manpage. Isn't it nice to have that ? Maybe you can only appreciate that patch if you try checking the consistency before. Otherwise we can just do pacman -Sh | sort : --arch <arch> set an alternate architecture --asdeps install packages as non-explicitly installed --asexplicit install packages as explicitly installed -b, --dbpath <path> set an alternate database location --cachedir <dir> set an alternate package cache location -c, --clean remove old packages from cache directory (-cc for all) --config <path> set an alternate configuration file --debug display debug messages -d, --nodeps skip dependency checks -f, --force force install, overwrite conflicting files -g, --groups view all members of a package group ignore a group upgrade (can be used more than once) --ignoregroup <grp> --ignore <pkg> ignore a package upgrade (can be used more than once) -i, --info view package information -k, --dbonly only modify database entries, not package files -l, --list <repo> view a list of packages in a repo --logfile <path> set an alternate log file --needed don't reinstall up to date packages --noconfirm do not ask for any confirmation --noprogressbar do not show a progress bar when downloading files --noscriptlet do not execute the install scriptlet if one exists --print-format <string> --print only print the targets instead of performing the operation -q, --quiet show less information for query and search -r, --root <path> set an alternate installation root specify how the targets should be printed -s, --search <regex> search remote repositories for matching strings -u, --sysupgrade upgrade installed packages (-uu allows downgrade) -v, --verbose be verbose -w, --downloadonly download packages but do not install/upgrade anything -y, --refresh download fresh package databases from the server I suppose I could do that in code, just replace all printf by addlist, sort the list at the end and print it. That way the options will always appear in the same order, no matter how they are written in the code.

Dan McGee

1:05 p.m.

New subject: [pacman-dev] [PATCH 4/4] CLI args: update --help and manpage

On Tue, Oct 12, 2010 at 3:34 AM, Xavier Chantry wrote:

...

On Tue, Oct 12, 2010 at 4:04 AM, Dan McGee wrote:

...
I'm not too thrilled abuot this patch for the fact that we lost alpha ordering by shortopt for sync/upgrade/remove options. This is a step backwards in my opinion.

I can deal with the dbpath/root/every single op uses these things being at the bottom (and I think a blank line separating them would be good too), but the list seems very mismanaged for the main ops now.

I pulled the other two patches in, but not this one.

usage: pacman {-S --sync} [options] [package(s)] options: --asdeps install packages as non-explicitly installed --asexplicit install packages as explicitly installed -c, --clean remove old packages from cache directory (-cc for all) -d, --nodeps skip dependency checks -f, --force force install, overwrite conflicting files -g, --groups view all members of a package group -i, --info view package information -l, --list <repo> view a list of packages in a repo -s, --search <regex> search remote repositories for matching strings -u, --sysupgrade upgrade installed packages (-uu allows downgrade) -w, --downloadonly download packages but do not install/upgrade anything -y, --refresh download fresh package databases from the server --needed don't reinstall up to date packages --ignore <pkg> ignore a package upgrade (can be used more than once) --ignoregroup <grp> ignore a group upgrade (can be used more than once) --print only print the targets instead of performing the operation --print-format <string> specify how the targets should be printed -q, --quiet show less information for query and search --config <path> set an alternate configuration file --logfile <path> set an alternate log file --noconfirm do not ask for any confirmation --noprogressbar do not show a progress bar when downloading files --noscriptlet do not execute the install scriptlet if one exists -v, --verbose be verbose --debug display debug messages -r, --root <path> set an alternate installation root -b, --dbpath <path> set an alternate database location --cachedir <dir> set an alternate package cache location --arch <arch> set an alternate architecture

I don't understand why it is fine to have -q -v -r -b mixed up like that for the global section, but not have two other similar sections, which makes it much nicer to check consistency of code, --help and manpage. Isn't it nice to have that ? Maybe you can only appreciate that patch if you try checking the consistency before.

I don't know- I'm just saying it is a lot more noticeable now than it ever was before, maybe I was just too familiar with it. I'm not sure how the manpage plays into this though, I was only talking about --help.

...

Otherwise we can just do pacman -Sh | sort : --arch <arch> set an alternate architecture --asdeps install packages as non-explicitly installed --asexplicit install packages as explicitly installed -b, --dbpath <path> set an alternate database location --cachedir <dir> set an alternate package cache location -c, --clean remove old packages from cache directory (-cc for all) --config <path> set an alternate configuration file --debug display debug messages -d, --nodeps skip dependency checks -f, --force force install, overwrite conflicting files -g, --groups view all members of a package group ignore a group upgrade (can be used more than once) --ignoregroup <grp> --ignore <pkg> ignore a package upgrade (can be used more than once) -i, --info view package information -k, --dbonly only modify database entries, not package files -l, --list <repo> view a list of packages in a repo --logfile <path> set an alternate log file --needed don't reinstall up to date packages --noconfirm do not ask for any confirmation --noprogressbar do not show a progress bar when downloading files --noscriptlet do not execute the install scriptlet if one exists --print-format <string> --print only print the targets instead of performing the operation -q, --quiet show less information for query and search -r, --root <path> set an alternate installation root specify how the targets should be printed -s, --search <regex> search remote repositories for matching strings -u, --sysupgrade upgrade installed packages (-uu allows downgrade) -v, --verbose be verbose -w, --downloadonly download packages but do not install/upgrade anything -y, --refresh download fresh package databases from the server

I suppose I could do that in code, just replace all printf by addlist, sort the list at the end and print it. That way the options will always appear in the same order, no matter how they are written in the code.

I thought of this but it almost seemed silly. However, it would work for the most part, right? I'm not against it. -Dan

Xavier Chantry

14 Oct 14 Oct

8:08 a.m.

New subject: [pacman-dev] [PATCH 2/3] pacman: sort --help output

Example with pacman -Uh : options: --arch <arch> set an alternate architecture --asdeps install packages as non-explicitly installed --asexplicit install packages as explicitly installed --cachedir <dir> set an alternate package cache location --config <path> set an alternate configuration file --debug display debug messages --ignore <pkg> ignore a package upgrade (can be used more than once) --ignoregroup <grp> ignore a group upgrade (can be used more than once) --logfile <path> set an alternate log file --noconfirm do not ask for any confirmation --noprogressbar do not show a progress bar when downloading files --noscriptlet do not execute the install scriptlet if one exists --print only print the targets instead of performing the operation --print-format <string> specify how the targets should be printed -b, --dbpath <path> set an alternate database location -d, --nodeps skip dependency checks -f, --force force install, overwrite conflicting files -k, --dbonly add database entries, do not install or keep existing files -r, --root <path> set an alternate installation root -v, --verbose be verbose --- src/pacman/pacman.c | 110 +++++++++++++++++++++++++++----------------------- src/pacman/util.h | 1 + 2 files changed, 60 insertions(+), 51 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 933698e..f402eef 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -65,6 +65,8 @@ static alpm_list_t *pm_targets; */ static void usage(int op, const char * const myname) { +#define addlist(s) (list = alpm_list_add(list, s)) + alpm_list_t *list = NULL, *i; /* prefetch some strings for usage below, which moves a lot of calls * out of gettext. */ char const * const str_opt = _("options"); @@ -89,82 +91,88 @@ static void usage(int op, const char * const myname) if(op == PM_OP_REMOVE) { printf("%s: %s {-R --remove} [%s] <%s>\n", str_usg, myname, str_opt, str_pkg); printf("%s:\n", str_opt); - printf(_(" -c, --cascade remove packages and all packages that depend on them\n")); - printf(_(" -k, --dbonly only remove database entries, do not remove files\n")); - printf(_(" -n, --nosave remove configuration files as well\n")); - printf(_(" -s, --recursive remove dependencies also (that won't break packages)\n" + addlist(_(" -c, --cascade remove packages and all packages that depend on them\n")); + addlist(_(" -k, --dbonly only remove database entries, do not remove files\n")); + addlist(_(" -n, --nosave remove configuration files as well\n")); + addlist(_(" -s, --recursive remove dependencies also (that won't break packages)\n" " (-ss includes explicitly installed dependencies too)\n")); - printf(_(" -u, --unneeded remove unneeded packages (that won't break packages)\n")); + addlist(_(" -u, --unneeded remove unneeded packages (that won't break packages)\n")); } else if(op == PM_OP_UPGRADE) { printf("%s: %s {-U --upgrade} [%s] <%s>\n", str_usg, myname, str_opt, str_file); printf("%s:\n", str_opt); } else if(op == PM_OP_QUERY) { printf("%s: %s {-Q --query} [%s] [%s]\n", str_usg, myname, str_opt, str_pkg); printf("%s:\n", str_opt); - printf(_(" -c, --changelog view the changelog of a package\n")); - printf(_(" -d, --deps list packages installed as dependencies [filter]\n")); - printf(_(" -e, --explicit list packages explicitly installed [filter]\n")); - printf(_(" -g, --groups view all members of a package group\n")); - printf(_(" -i, --info view package information (-ii for backup files)\n")); - printf(_(" -k, --check check that the files owned by the package(s) are present\n")); - printf(_(" -l, --list list the contents of the queried package\n")); - printf(_(" -m, --foreign list installed packages not found in sync db(s) [filter]\n")); - printf(_(" -o, --owns <file> query the package that owns <file>\n")); - printf(_(" -p, --file <package> query a package file instead of the database\n")); - printf(_(" -q, --quiet show less information for query and search\n")); - printf(_(" -s, --search <regex> search locally-installed packages for matching strings\n")); - printf(_(" -t, --unrequired list packages not required by any package [filter]\n")); - printf(_(" -u, --upgrades list outdated packages [filter]\n")); + addlist(_(" -c, --changelog view the changelog of a package\n")); + addlist(_(" -d, --deps list packages installed as dependencies [filter]\n")); + addlist(_(" -e, --explicit list packages explicitly installed [filter]\n")); + addlist(_(" -g, --groups view all members of a package group\n")); + addlist(_(" -i, --info view package information (-ii for backup files)\n")); + addlist(_(" -k, --check check that the files owned by the package(s) are present\n")); + addlist(_(" -l, --list list the contents of the queried package\n")); + addlist(_(" -m, --foreign list installed packages not found in sync db(s) [filter]\n")); + addlist(_(" -o, --owns <file> query the package that owns <file>\n")); + addlist(_(" -p, --file <package> query a package file instead of the database\n")); + addlist(_(" -q, --quiet show less information for query and search\n")); + addlist(_(" -s, --search <regex> search locally-installed packages for matching strings\n")); + addlist(_(" -t, --unrequired list packages not required by any package [filter]\n")); + addlist(_(" -u, --upgrades list outdated packages [filter]\n")); } else if(op == PM_OP_SYNC) { printf("%s: %s {-S --sync} [%s] [%s]\n", str_usg, myname, str_opt, str_pkg); printf("%s:\n", str_opt); - printf(_(" -c, --clean remove old packages from cache directory (-cc for all)\n")); - printf(_(" -g, --groups view all members of a package group\n")); - printf(_(" -i, --info view package information\n")); - printf(_(" -l, --list <repo> view a list of packages in a repo\n")); - printf(_(" -q, --quiet show less information for query and search\n")); - printf(_(" -s, --search <regex> search remote repositories for matching strings\n")); - printf(_(" -u, --sysupgrade upgrade installed packages (-uu allows downgrade)\n")); - printf(_(" -w, --downloadonly download packages but do not install/upgrade anything\n")); - printf(_(" -y, --refresh download fresh package databases from the server\n")); - printf(_(" --needed don't reinstall up to date packages\n")); + addlist(_(" -c, --clean remove old packages from cache directory (-cc for all)\n")); + addlist(_(" -g, --groups view all members of a package group\n")); + addlist(_(" -i, --info view package information\n")); + addlist(_(" -l, --list <repo> view a list of packages in a repo\n")); + addlist(_(" -q, --quiet show less information for query and search\n")); + addlist(_(" -s, --search <regex> search remote repositories for matching strings\n")); + addlist(_(" -u, --sysupgrade upgrade installed packages (-uu allows downgrade)\n")); + addlist(_(" -w, --downloadonly download packages but do not install/upgrade anything\n")); + addlist(_(" -y, --refresh download fresh package databases from the server\n")); + addlist(_(" --needed don't reinstall up to date packages\n")); } else if (op == PM_OP_DATABASE) { printf("%s: %s {-D --database} <%s> <%s>\n", str_usg, myname, str_opt, str_pkg); printf("%s:\n", str_opt); - printf(_(" --asdeps mark packages as non-explicitly installed\n")); - printf(_(" --asexplicit mark packages as explicitly installed\n")); + addlist(_(" --asdeps mark packages as non-explicitly installed\n")); + addlist(_(" --asexplicit mark packages as explicitly installed\n")); } switch(op) { case PM_OP_SYNC: case PM_OP_UPGRADE: - printf(_(" -f, --force force install, overwrite conflicting files\n")); - printf(_(" -k, --dbonly add database entries, do not install or keep existing files\n")); - printf(_(" --asdeps install packages as non-explicitly installed\n")); - printf(_(" --asexplicit install packages as explicitly installed\n")); - printf(_(" --ignore <pkg> ignore a package upgrade (can be used more than once)\n")); - printf(_(" --ignoregroup <grp>\n" + addlist(_(" -f, --force force install, overwrite conflicting files\n")); + addlist(_(" -k, --dbonly add database entries, do not install or keep existing files\n")); + addlist(_(" --asdeps install packages as non-explicitly installed\n")); + addlist(_(" --asexplicit install packages as explicitly installed\n")); + addlist(_(" --ignore <pkg> ignore a package upgrade (can be used more than once)\n")); + addlist(_(" --ignoregroup <grp>\n" " ignore a group upgrade (can be used more than once)\n")); /* pass through */ case PM_OP_REMOVE: - printf(_(" -d, --nodeps skip dependency checks\n")); - printf(_(" --noprogressbar do not show a progress bar when downloading files\n")); - printf(_(" --noscriptlet do not execute the install scriptlet if one exists\n")); - printf(_(" --print only print the targets instead of performing the operation\n")); - printf(_(" --print-format <string>\n" + addlist(_(" -d, --nodeps skip dependency checks\n")); + addlist(_(" --noprogressbar do not show a progress bar when downloading files\n")); + addlist(_(" --noscriptlet do not execute the install scriptlet if one exists\n")); + addlist(_(" --print only print the targets instead of performing the operation\n")); + addlist(_(" --print-format <string>\n" " specify how the targets should be printed\n")); break; } - printf(_(" -b, --dbpath <path> set an alternate database location\n")); - printf(_(" -r, --root <path> set an alternate installation root\n")); - printf(_(" -v, --verbose be verbose\n")); - printf(_(" --arch <arch> set an alternate architecture\n")); - printf(_(" --cachedir <dir> set an alternate package cache location\n")); - printf(_(" --config <path> set an alternate configuration file\n")); - printf(_(" --debug display debug messages\n")); - printf(_(" --logfile <path> set an alternate log file\n")); - printf(_(" --noconfirm do not ask for any confirmation\n")); + addlist(_(" -b, --dbpath <path> set an alternate database location\n")); + addlist(_(" -r, --root <path> set an alternate installation root\n")); + addlist(_(" -v, --verbose be verbose\n")); + addlist(_(" --arch <arch> set an alternate architecture\n")); + addlist(_(" --cachedir <dir> set an alternate package cache location\n")); + addlist(_(" --config <path> set an alternate configuration file\n")); + addlist(_(" --debug display debug messages\n")); + addlist(_(" --logfile <path> set an alternate log file\n")); + addlist(_(" --noconfirm do not ask for any confirmation\n")); } + list = alpm_list_msort(list, alpm_list_count(list), str_cmp); + for (i = list; i; i = alpm_list_next(i)) { + printf("%s", (char *)alpm_list_getdata(i)); + } + alpm_list_free(list); +#undef addlist } /** Output pacman version and copyright. diff --git a/src/pacman/util.h b/src/pacman/util.h index 2e77b0c..0308f6b 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -55,6 +55,7 @@ void string_display(const char *title, const char *string); void list_display(const char *title, const alpm_list_t *list); void list_display_linebreak(const char *title, const alpm_list_t *list); void display_targets(const alpm_list_t *pkgs, int install); +int str_cmp(const void *s1, const void *s2); void display_new_optdepends(pmpkg_t *oldpkg, pmpkg_t *newpkg); void display_optdepends(pmpkg_t *pkg); void print_packages(const alpm_list_t *packages); -- 1.7.3.1

Xavier Chantry

12:18 p.m.

New subject: [pacman-dev] [PATCH] Sort short options before long options in --help

$ pacman -Uh options: -b, --dbpath <path> set an alternate database location -d, --nodeps skip dependency checks -f, --force force install, overwrite conflicting files -k, --dbonly only modify database entries, not package files -r, --root <path> set an alternate installation root -v, --verbose be verbose --arch <arch> set an alternate architecture --asdeps install packages as non-explicitly installed --asexplicit install packages as explicitly installed --cachedir <dir> set an alternate package cache location --config <path> set an alternate configuration file --debug display debug messages --ignore <pkg> ignore a package upgrade (can be used more than once) --ignoregroup <grp> ignore a group upgrade (can be used more than once) --logfile <path> set an alternate log file --noconfirm do not ask for any confirmation --noprogressbar do not show a progress bar when downloading files --noscriptlet do not execute the install scriptlet if one exists --print only print the targets instead of performing the operation --print-format <string> specify how the targets should be printed Signed-off-by: Xavier Chantry --- src/pacman/pacman.c | 27 ++++++++++++++++++++++++++- 1 files changed, 26 insertions(+), 1 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 7d51124..3bfe6ef 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -28,6 +28,7 @@ #include /* atoi */ #include +#include /* isspace */ #include #include #include @@ -59,6 +60,30 @@ pmdb_t *db_local; /* list of targets specified on command line */ static alpm_list_t *pm_targets; +/* Used to sort the options in --help */ +static int options_cmp(const void *p1, const void *p2) +{ + const char *s1 = p1; + const char *s2 = p2; + int ret = 0; + + /* First skip all spaces in both strings */ + while(isspace((unsigned char)*s1)) + s1++; + while(isspace((unsigned char)*s2)) + s2++; + /* If we compare a long option (--abcd) and a short one (-a), + * the short one always wins */ + if (*(s1+1) != '-' && *(s2+1) == '-') { + ret = -1; + } else if (*(s2+1) != '-' && *(s1+1) == '-') { + ret = 1; + } else { + ret = strcmp(s1, s2); + } + return(ret); +} + /** Display usage/syntax for the specified operation. * @param op the operation code requested * @param myname basename(argv[0]) @@ -166,7 +191,7 @@ static void usage(int op, const char * const myname) addlist(_(" --logfile <path> set an alternate log file\n")); addlist(_(" --noconfirm do not ask for any confirmation\n")); } - list = alpm_list_msort(list, alpm_list_count(list), str_cmp); + list = alpm_list_msort(list, alpm_list_count(list), options_cmp); for (i = list; i; i = alpm_list_next(i)) { printf("%s", (char *)alpm_list_getdata(i)); } -- 1.7.3.1

Nagy Gabor

11:55 a.m.

New subject: [pacman-dev] [PATCH] Sort short options before long options in --help

...

$ pacman -Uh options: -b, --dbpath <path> set an alternate database location -d, --nodeps skip dependency checks -f, --force force install, overwrite conflicting files -k, --dbonly only modify database entries, not package files -r, --root <path> set an alternate installation root -v, --verbose be verbose --arch <arch> set an alternate architecture --asdeps install packages as non-explicitly installed --asexplicit install packages as explicitly installed --cachedir <dir> set an alternate package cache location --config <path> set an alternate configuration file --debug display debug messages --ignore <pkg> ignore a package upgrade (can be used more than once) --ignoregroup <grp> ignore a group upgrade (can be used more than once) --logfile <path> set an alternate log file --noconfirm do not ask for any confirmation --noprogressbar do not show a progress bar when downloading files --noscriptlet do not execute the install scriptlet if one exists --print only print the targets instead of performing the operation --print-format <string> specify how the targets should be printed

Signed-off-by: Xavier Chantry

I like this.

...

+/* Used to sort the options in --help */ +static int options_cmp(const void *p1, const void *p2) +{ + const char *s1 = p1; + const char *s2 = p2; + int ret = 0; + + /* First skip all spaces in both strings */ + while(isspace((unsigned char)*s1)) + s1++; + while(isspace((unsigned char)*s2)) + s2++; + /* If we compare a long option (--abcd) and a short one (-a), + * the short one always wins */ + if (*(s1+1) != '-' && *(s2+1) == '-') { + ret = -1;

I hope you don't pass invalid string arguments (e.g. empty string), otherwise we can get a segfault here.

...

+ } else if (*(s2+1) != '-' && *(s1+1) == '-') { + ret = 1; + } else { + ret = strcmp(s1, s2); + } + return(ret); +}

Xavier Chantry

12:56 p.m.

New subject: [pacman-dev] [PATCH] Sort short options before long options in --help

On Thu, Oct 14, 2010 at 1:55 PM, Nagy Gabor wrote:

...

I like this.

Cool :)

...

...
+ /* First skip all spaces in both strings */ + while(isspace((unsigned char)*s1)) + s1++; + while(isspace((unsigned char)*s2)) + s2++; + /* If we compare a long option (--abcd) and a short one (-a), + * the short one always wins */ + if (*(s1+1) != '-' && *(s2+1) == '-') { + ret = -1;

I hope you don't pass invalid string arguments (e.g. empty string), otherwise we can get a segfault here.

I missed something very important. I thought we had complete controls on these strings, while in fact we don't have any control at all, since it's the gettext-ed strings that I sort :P I will make a safer version, thanks.

Xavier Chantry

1:36 p.m.

New subject: [pacman-dev] [PATCH] Sort short options before long options in --help

$ pacman -Uh options: -b, --dbpath <path> set an alternate database location -d, --nodeps skip dependency checks -f, --force force install, overwrite conflicting files -k, --dbonly only modify database entries, not package files -r, --root <path> set an alternate installation root -v, --verbose be verbose --arch <arch> set an alternate architecture --asdeps install packages as non-explicitly installed --asexplicit install packages as explicitly installed --cachedir <dir> set an alternate package cache location --config <path> set an alternate configuration file --debug display debug messages --ignore <pkg> ignore a package upgrade (can be used more than once) --ignoregroup <grp> ignore a group upgrade (can be used more than once) --logfile <path> set an alternate log file --noconfirm do not ask for any confirmation --noprogressbar do not show a progress bar when downloading files --noscriptlet do not execute the install scriptlet if one exists --print only print the targets instead of performing the operation --print-format <string> specify how the targets should be printed Signed-off-by: Xavier Chantry --- src/pacman/pacman.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 files changed, 38 insertions(+), 1 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 7d51124..38d7f6a 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -26,8 +26,10 @@ #define PACKAGE_VERSION GIT_VERSION #endif +#include #include /* atoi */ #include +#include /* isspace */ #include #include #include @@ -59,6 +61,41 @@ pmdb_t *db_local; /* list of targets specified on command line */ static alpm_list_t *pm_targets; +/* Used to sort the options in --help */ +static int options_cmp(const void *p1, const void *p2) +{ + const char *s1 = p1; + const char *s2 = p2; + + assert(s1); + assert(s2); + /* First skip all spaces in both strings */ + while(isspace((unsigned char)*s1)) + s1++; + while(isspace((unsigned char)*s2)) + s2++; + /* If we compare a long option (--abcd) and a short one (-a), + * the short one always wins */ + if(*s1 == '-' && *s2 == '-') { + s1++; + s2++; + if(*s1 == '-' && *s2 == '-') { + /* two long -> strcmp */ + s1++; + s2++; + } else if(*s2 == '-') { + /* s1 short, s2 long */ + return(-1); + } else if(*s1 == '-') { + /* s1 long, s2 short */ + return(1); + } + /* two short -> strcmp */ + } + + return(strcmp(s1, s2)); +} + /** Display usage/syntax for the specified operation. * @param op the operation code requested * @param myname basename(argv[0]) @@ -166,7 +203,7 @@ static void usage(int op, const char * const myname) addlist(_(" --logfile <path> set an alternate log file\n")); addlist(_(" --noconfirm do not ask for any confirmation\n")); } - list = alpm_list_msort(list, alpm_list_count(list), str_cmp); + list = alpm_list_msort(list, alpm_list_count(list), options_cmp); for (i = list; i; i = alpm_list_next(i)) { printf("%s", (char *)alpm_list_getdata(i)); } -- 1.7.3.1

Dan McGee

1:44 p.m.

New subject: [pacman-dev] [PATCH] Sort short options before long options in --help

On Thu, Oct 14, 2010 at 8:36 AM, Xavier Chantry wrote:

...

diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 7d51124..38d7f6a 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -26,8 +26,10 @@ #define PACKAGE_VERSION GIT_VERSION #endif

+#include #include /* atoi */ #include +#include /* isspace */ #include #include #include @@ -59,6 +61,41 @@ pmdb_t *db_local; /* list of targets specified on command line */ static alpm_list_t *pm_targets;

+/* Used to sort the options in --help */ +static int options_cmp(const void *p1, const void *p2) +{ + const char *s1 = p1; + const char *s2 = p2; + + assert(s1); + assert(s2);

Is the assert stuff you added to be safer? We don't want to use this. One, they are compiled into nothing if NDEBUG is defined, and two, it is a bit silly to abort on this condition. cmp(NULL, NULL) ==0, and then just decide whether to sort NULL first or last. -Dan

Xavier Chantry

3:43 p.m.

New subject: [pacman-dev] [PATCH] Sort short options before long options in --help

On Thu, Oct 14, 2010 at 3:44 PM, Dan McGee wrote:

...

Is the assert stuff you added to be safer? We don't want to use this. One, they are compiled into nothing if NDEBUG is defined, and two, it is a bit silly to abort on this condition. cmp(NULL, NULL) ==0, and then just decide whether to sort NULL first or last.

It was not supposed to be safer, just to explicitly state that this case should not happen. If gettext returns NULL, our --help output is going to look awesome : (null)(null)(null).. It's not perfectly clear to me that an assert is worse. In the unlikely case that this could be triggered, at least an assert would fail gracefully telling us where it failed and possibly giving a core dump that would tell us more about what happened. Anyway we could debate over and over about assert usage. Maybe it's more interesting for an expensive check in a hot-path that you do not want to keep in a release build, but you want to make sure that the assert is indeed not triggered while developing / testing. I updated the patch with an explicit check (I actually already did this before choosing to use assert) : http://code.toofishes.net/cgit/xavier/pacman.git/log/?h=parseargs

4954

Age (days ago)

4958

Last active (days ago)

List overview

Download

15 comments

4 participants

participants (4)

Allan McRae
Dan McGee
Nagy Gabor
Xavier Chantry