[pacman-dev] [PATCH 0/5] Miscellaneous pacman-conf tweaks
From: Ivy Foster <iff@escondida.tk> I started fiddling around with pacman-conf today, because I wanted to add short options to it, and then just sort of...kept on for a bit. Oh, I could be wrong, but I think that 'pacman-conf --repo <foo>' pretty much covers [FS#25568][1]. [1]: https://bugs.archlinux.org/task/25568 Ivy Foster (5): pacman-conf: accept short options pacman-conf: simplify usage() pacman-conf: Make myname and myver into cpp macros MYNAME and MYVER pacman-conf: Make cleanup run automagically at exit pacman-conf.c: make parse_opts only parse opts src/pacman/pacman-conf.c | 84 +++++++++++++++++++++++++----------------------- 1 file changed, 43 insertions(+), 41 deletions(-) -- 2.16.1
From: Ivy Foster <iff@escondida.tk> Also change '-?' no longer to be an error, because if we're going to check for it anyway, why make it an error? Signed-off-by: Ivy Foster <iff@escondida.tk> --- src/pacman/pacman-conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 1e6f55f9..5347a837 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -59,7 +59,7 @@ static void parse_opts(int argc, char **argv) int c; config_file = CONFFILE; - const char *short_opts = ""; + const char *short_opts = "c:hlR:r:Vv?"; struct option long_opts[] = { { "config" , required_argument , NULL , 'c' }, { "rootdir" , required_argument , NULL , 'R' }, @@ -89,6 +89,7 @@ static void parse_opts(int argc, char **argv) verbose = 1; break; case 'h': + case '?': usage(0); break; case 'V': @@ -96,7 +97,6 @@ static void parse_opts(int argc, char **argv) cleanup(); exit(0); break; - case '?': default: usage(1); break; -- 2.16.1
On 02/08/2018 11:49 PM, iff@escondida.tk wrote:
From: Ivy Foster <iff@escondida.tk>
Also change '-?' no longer to be an error, because if we're going to check for it anyway, why make it an error?
Hmm, why are we checking for it at all? I see no reason to treat it differently from any other unknown option, and I *just* removed the undocumented --usage variant from vercmp as I think [-h|--help] is more than enough for everyone and everything. Can we just remove '-?' altogether? We don't use it anywhere else, --help is pretty standard, and I feel like '-?' will just be confusing and possibly too Windows'y to boot. -- Eli Schwartz Bug Wrangler and Trusted User
On 02/08/18 at 10:49pm, iff@escondida.tk wrote:
From: Ivy Foster <iff@escondida.tk>
Also change '-?' no longer to be an error, because if we're going to check for it anyway, why make it an error?
Because '?' indicates an error.
From: Ivy Foster <iff@escondida.tk> Signed-off-by: Ivy Foster <iff@escondida.tk> --- src/pacman/pacman-conf.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 5347a837..a503ece9 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -34,24 +34,21 @@ static void cleanup(void) config_free(config); } -static void usage(int ret) +static void usage(FILE *stream) { - FILE *stream = (ret ? stderr : stdout); -#define hputs(x) fputs(x"\n", stream) - hputs("pacman-conf - query pacman's configuration file"); - hputs("usage: pacman-conf [options] [<directive>...]"); - hputs(" pacman-conf (--repo-list|--help|--version)"); - hputs("options:"); - hputs(" --config=<path> set an alternate configuration file"); - hputs(" --rootdir=<path> set an alternate installation root"); - hputs(" --repo=<remote> query options for a specific repo"); - hputs(" --verbose always show directive names"); - hputs(" --repo-list list configured repositories"); - hputs(" --help display this help information"); - hputs(" --version display version information"); -#undef hputs - cleanup(); - exit(ret); + static const char help[] = + "pacman-conf: query pacman's configuration file\n" + "usage: pacman-conf [options] [directive]\n" + " pacman-conf [ { -l, --repo-list } | { -h, --help } | { -V, --version } ]\n" + "options:\n" + " -c, --config=<file> Read configuration from <file>\n" + " -h, --help Print help\n" + " -l, --repo-list List configured repositories\n" + " -r, --repo=<remote> Query options for a specific repo\n" + " -R, --rootdir=<dir> Use <dir> as system root\n" + " -v, --verbose Always show directive names\n" + " -V, --version Print version\n"; + fprintf(stream, "%s", help); } static void parse_opts(int argc, char **argv) @@ -90,15 +87,18 @@ static void parse_opts(int argc, char **argv) break; case 'h': case '?': - usage(0); - break; + usage(stdout); + cleanup(); + exit(0); case 'V': printf("%s v%s\n", myname, myver); cleanup(); exit(0); break; default: - usage(1); + usage(stderr); + cleanup(); + exit(1); break; } } -- 2.16.1
From: Ivy Foster <iff@escondida.tk> This allows MYNAME to be used in place of the literal string "pacman-conf" in usage Signed-off-by: Ivy Foster <iff@escondida.tk> --- src/pacman/pacman-conf.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index a503ece9..7d09fd4b 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -21,7 +21,8 @@ #include <string.h> #include "conf.h" -const char *myname = "pacman-conf", *myver = "1.0.0"; +#define MYNAME "pacman-conf" +#define MYVER "1.0.0" alpm_list_t *directives = NULL; char sep = '\n', *repo_name = NULL; @@ -37,9 +38,9 @@ static void cleanup(void) static void usage(FILE *stream) { static const char help[] = - "pacman-conf: query pacman's configuration file\n" - "usage: pacman-conf [options] [directive]\n" - " pacman-conf [ { -l, --repo-list } | { -h, --help } | { -V, --version } ]\n" + MYNAME ": query pacman's configuration file\n" + "usage: " MYNAME " [options] [directive]\n" + " " MYNAME " [ { -l, --repo-list } | { -h, --help } | { -V, --version } ]\n" "options:\n" " -c, --config=<file> Read configuration from <file>\n" " -h, --help Print help\n" @@ -91,7 +92,7 @@ static void parse_opts(int argc, char **argv) cleanup(); exit(0); case 'V': - printf("%s v%s\n", myname, myver); + printf("%s v%s\n", MYNAME, MYVER); cleanup(); exit(0); break; -- 2.16.1
From: Ivy Foster <iff@escondida.tk> Since using atexit(3) draws in stdlib anyway and this changes every exit call at large and return call in main(), go ahead and use EXIT_SUCCESS and EXIt_FAILURE, too. Signed-off-by: Ivy Foster <iff@escondida.tk> --- src/pacman/pacman-conf.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 7d09fd4b..aa102031 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -18,7 +18,9 @@ */ #include <getopt.h> +#include <stdlib.h> #include <string.h> + #include "conf.h" #define MYNAME "pacman-conf" @@ -89,18 +91,13 @@ static void parse_opts(int argc, char **argv) case 'h': case '?': usage(stdout); - cleanup(); - exit(0); + exit(EXIT_SUCCESS); case 'V': printf("%s v%s\n", MYNAME, MYVER); - cleanup(); - exit(0); - break; + exit(EXIT_SUCCESS); default: usage(stderr); - cleanup(); - exit(1); - break; + exit(EXIT_FAILURE); } } @@ -401,11 +398,15 @@ int main(int argc, char **argv) { int ret = 0; + if (atexit(cleanup) != 0) { + fputs("error: cannot set exit function\n", stderr); + return EXIT_FAILURE; + } + config = config_new(); parse_opts(argc, argv); if(!config) { - ret = 1; - goto cleanup; + return EXIT_FAILURE; } for(; optind < argc; optind++) { @@ -419,8 +420,7 @@ int main(int argc, char **argv) if(repo_list) { if(directives) { fputs("error: directives may not be specified with --repo-list\n", stderr); - ret = 1; - goto cleanup; + return EXIT_FAILURE; } list_repos(); } else if(repo_name) { @@ -429,10 +429,7 @@ int main(int argc, char **argv) ret = list_directives(); } -cleanup: - cleanup(); - - return ret; + return ret ? EXIT_FAILURE : EXIT_SUCCESS; } /* vim: set ts=2 sw=2 noet: */ -- 2.16.1
From: Ivy Foster <iff@escondida.tk> Previously, it also parsed the config file instead of simply setting the (global) config_file variable Signed-off-by: Ivy Foster <iff@escondida.tk> --- src/pacman/pacman-conf.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index aa102031..52bbdb9e 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -100,10 +100,6 @@ static void parse_opts(int argc, char **argv) exit(EXIT_FAILURE); } } - - if(parseconfigfile(config_file) != 0 || setdefaults(config) != 0) { - fprintf(stderr, "error parsing '%s'\n", config_file); - } } static void list_repos(void) @@ -403,8 +399,16 @@ int main(int argc, char **argv) return EXIT_FAILURE; } - config = config_new(); parse_opts(argc, argv); + + if(!(config = config_new())) { + /* config_new prints the appropriate error message */ + return EXIT_FAILURE; + } + if(parseconfigfile(config_file) != 0 || setdefaults(config) != 0) { + fprintf(stderr, "error parsing '%s'\n", config_file); + return EXIT_FAILURE; + } if(!config) { return EXIT_FAILURE; } -- 2.16.1
On 02/08/2018 11:49 PM, iff@escondida.tk wrote:
From: Ivy Foster <iff@escondida.tk>
I started fiddling around with pacman-conf today, because I wanted to add short options to it, and then just sort of...kept on for a bit.
Oh, I could be wrong, but I think that 'pacman-conf --repo <foo>' pretty much covers [FS#25568][1].
I agree, and I've closed it as implemented. :) -- Eli Schwartz Bug Wrangler and Trusted User
participants (3)
-
Andrew Gregory
-
Eli Schwartz
-
iff＠escondida.tk