We had a bug report about mpc options given after commands - I didn't
realize they could come after commands, and broke this when I added short
options. It's fixed now! I tested my patch with all combinations of [no
command, command with arguments, command without arguments] and [no options,
options before, options after, options before and after]. I'm pretty sure I
didn't miss anything this time. It's not the totally ideal way to do this -
it still rearranges argv, just in the correct way for mpc [options] command
[options] arguments - but it's consistent with what we have so far. More
importantly, it fixes the regression.
I notice that the usage statement (printed or in man page) doesn't say
[options] like most commands do - with 20/20 hindsight, maybe specifying
there could've saved some trouble. I went ahead and made a small patch for
this too.
Finally, I while I was messing with usage statements, I modified the help
printing behavior - if the command is invalid, just print an error, usage
statement, and reminder about 'mpc help' and man page. I suspect the vast
majority of invalid commands are typos, and none of us want our screens
filled whenever we mistype.
Unfortunately, I'm behind a firewall and can't push right now - I'm
attaching patches, and as soon as I have some free (as in freedom) internet,
I'll push to my repo (git://git.musicpd.org/jefromi/mpc.git).
Jeffrey
From cb5dc0797654b4b370fed85167876195a2c8f403 Mon Sep 17 00:00:00 2001
From: Jeffrey Middleton <jefr...@gmail.com>
Date: Mon, 14 Sep 2009 09:41:34 -0500
Subject: [PATCH 1/5] options.c: allow for options before and after the command
The original patch (b3f6766) adding short option support and some new
options broke support for options after the command. This patch fixes
that. At the same time, it also fixes treatment of argument '-' (invalid
option) and '--' (treat everything following as non-option).
---
src/options.c | 47 +++++++++++++++++++++++++++++++++++++++--------
1 files changed, 39 insertions(+), 8 deletions(-)
diff --git a/src/options.c b/src/options.c
index 57fd330..34f4f67 100644
--- a/src/options.c
+++ b/src/options.c
@@ -136,20 +136,31 @@ handle_option(int c, const char *arg)
void
parse_options(int * argc_p, char ** argv)
{
- int i, optind;
+ int i;
const arg_opt_t *opt = NULL;
char * tmp;
+ int cmdind = 0;
+ int optind = 0;
for (i = 1; i < *argc_p; i++) {
const char *arg = argv[i];
size_t len = strlen(arg);
- if (len >= 2 && arg[0] == '-') {
+ if (arg[0] == '-') {
if (arg[1] == '-') {
/* arg is a long option */
char *value;
size_t name_len = len - 2;
+ /* if arg is "--", there can be no more options */
+ if (len == 2) {
+ optind = i + 1;
+ if (cmdind == 0) {
+ cmdind = optind;
+ }
+ break;
+ }
+
/* make sure we got an argument for the previous option */
if( opt && opt->argument)
option_error(ERROR_MISSING_ARGUMENT, opt->longopt, opt->argument);
@@ -177,6 +188,8 @@ parse_options(int * argc_p, char ** argv)
} else {
/* arg is a short option (or several) */
size_t j;
+ if (len == 1)
+ option_error(ERROR_UNKNOWN_OPTION, arg, NULL);
for (j=1; j<len; j++) {
/* make sure we got an argument for the previous option */
@@ -196,15 +209,25 @@ parse_options(int * argc_p, char ** argv)
}
}
} else {
- /* No '-'; arg is an option argument or command. */
+ /* No '-' */
if (opt && opt->argument) {
+ /* arg is an option argument */
handle_option(opt->shortopt, arg);
opt = NULL;
} else {
- break;
+ /* arg may the command; note it and read for more options */
+ if (cmdind == 0)
+ cmdind = i;
+ /* otherwise, it is the first command argument and we are done */
+ else {
+ optind = i;
+ break;
+ }
}
}
}
+ if (optind == 0)
+ optind = i;
if (opt && opt->argument)
option_error(ERROR_MISSING_ARGUMENT, opt->longopt, opt->argument);
@@ -232,12 +255,20 @@ parse_options(int * argc_p, char ** argv)
/* Fix argv for command processing, which wants
argv[1] to be the command, and so on. */
- optind = i;
+ if (cmdind != 0)
+ argv[1] = argv[cmdind];
if (optind > 1) {
- for (i = optind; i < *argc_p; i++)
- argv[i-optind+1] = argv[i];
+ if ( optind == cmdind || cmdind == 0 ) {
+ for (i = optind + 1; i < *argc_p; i++)
+ argv[i-optind+2] = argv[i];
+
+ *argc_p -= optind - 1;
+ } else {
+ for (i = optind; i < *argc_p; i++)
+ argv[i-optind+2] = argv[i];
- *argc_p -= optind - 1;
+ *argc_p -= optind - 2;
+ }
}
}
--
1.6.5.rc1.10.g20f34
From 5bcbb0ee2caef0706f275e6f56bc0d097202505b Mon Sep 17 00:00:00 2001
From: Jeffrey Middleton <jefr...@gmail.com>
Date: Mon, 14 Sep 2009 10:03:59 -0500
Subject: [PATCH 2/5] doc: indicate options in usage statements
Done both in man page and printed usage statement; this synchronizes the
two.
---
doc/mpc.1 | 9 ++++++---
src/main.c | 2 +-
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/doc/mpc.1 b/doc/mpc.1
index 7611142..4f29a9e 100644
--- a/doc/mpc.1
+++ b/doc/mpc.1
@@ -2,11 +2,14 @@
.SH "NAME"
mpc \- Program for controlling Music Player Daemon (MPD)
.SH "SYNOPSIS"
-mpc [<command> <arguments>]
+.B mpc
+.RI [ options ]
+.I <command>
+.RI [ <arguments> ]
.SH "DESCRIPTION"
mpc is a client for MPD, the Music Player Daemon. mpc connects to a MPD and
-controls it according to commands and arguments passed to it. If no arguments
-are passed, current status is given.
+controls it according to commands and arguments passed to it. If no command
+is given, the current status is printed (same as 'mpc status').
.SH "OPTIONS"
.TP
.BI -f,--format
diff --git a/src/main.c b/src/main.c
index ebbff9a..ae71691 100644
--- a/src/main.c
+++ b/src/main.c
@@ -118,7 +118,7 @@ static int print_help(char * progname, char * command)
} else
fprintf(outfp,"unknown command \"%s\"\n",command);
}
- fprintf(outfp,"Usage: %s <command> [command args]...\n"
+ fprintf(outfp,"Usage: %s [options] <command> [<arguments>]\n"
"mpc version: "VERSION"\n",progname);
for (i=0; mpc_table[i].command; ++i) {
--
1.6.5.rc1.10.g20f34
From e216c2232279dbc7f426fb758491de4f5e474db6 Mon Sep 17 00:00:00 2001
From: Jeffrey Middleton <jefr...@gmail.com>
Date: Mon, 14 Sep 2009 10:53:20 -0500
Subject: [PATCH 3/5] doc: print only usage on unknown command
There's a good chance this was simply a typo, so just print the error
message, a usage statement, and a reminder about the man page and the
help command.
---
src/main.c | 39 +++++++++++++++++++++++----------------
1 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/src/main.c b/src/main.c
index ae71691..f6bee55 100644
--- a/src/main.c
+++ b/src/main.c
@@ -105,21 +105,28 @@ static struct command {
{ .command = NULL }
};
+static void
+print_usage(FILE * outfp, char * progname)
+{
+ fprintf(outfp,"Usage: %s [options] <command> [<arguments>]\n"
+ "mpc version: "VERSION"\n",progname);
+}
+
static int print_help(char * progname, char * command)
{
int i, max = 0;
- int ret = EXIT_FAILURE;
- FILE *outfp = stderr;
-
- if (command) {
- if (!strcmp(command,"help")) {
- outfp = stdout;
- ret = EXIT_SUCCESS;
- } else
- fprintf(outfp,"unknown command \"%s\"\n",command);
+
+ if (command && strcmp(command, "help")) {
+ fprintf(stderr,"unknown command \"%s\"\n",command);
+ print_usage(stderr, progname);
+ fprintf(stderr,"See man 1 mpc or use 'mpc help' for more help.\n");
+ return EXIT_FAILURE;
}
- fprintf(outfp,"Usage: %s [options] <command> [<arguments>]\n"
- "mpc version: "VERSION"\n",progname);
+
+ print_usage(stdout, progname);
+ printf("\n");
+
+ printf("Commands:\n");
for (i=0; mpc_table[i].command; ++i) {
if (mpc_table[i].help) {
@@ -129,7 +136,7 @@ static int print_help(char * progname, char * command)
}
}
- fprintf(outfp, "%s %*s Displays status\n",progname,max," ");
+ printf(" %s %*s Displays status\n",progname,max," ");
for (i=0; mpc_table[i].command; ++i) {
int spaces;
@@ -139,14 +146,14 @@ static int print_help(char * progname, char * command)
spaces = max-(strlen(mpc_table[i].command)+strlen(mpc_table[i].usage));
spaces += !spaces ? 0 : 1;
- fprintf(outfp,"%s %s %s%*s%s\n",progname,
+ printf(" %s %s %s%*s%s\n",progname,
mpc_table[i].command,mpc_table[i].usage,
spaces," ",mpc_table[i].help);
}
- fprintf(outfp,"For more information about these and other "
- "options look at man 1 mpc\n");
- return ret;
+ printf("For more information about these and other "
+ "options look at man 1 mpc\n");
+ return EXIT_SUCCESS;
}
static mpd_Connection * setup_connection (void)
--
1.6.5.rc1.10.g20f34
From 9f57d4cbbf789bc7e5ef588f59c3728b24e92bdd Mon Sep 17 00:00:00 2001
From: Jeffrey Middleton <jefr...@gmail.com>
Date: Mon, 14 Sep 2009 10:54:18 -0500
Subject: [PATCH 4/5] doc: print option help with mpc help
Loop through the defined option and print shortopt, longopt, and
description.
---
src/main.c | 7 +++++--
src/options.c | 36 ++++++++++++++++++++++++++++--------
src/options.h | 1 +
3 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/src/main.c b/src/main.c
index f6bee55..0e8066c 100644
--- a/src/main.c
+++ b/src/main.c
@@ -126,6 +126,10 @@ static int print_help(char * progname, char * command)
print_usage(stdout, progname);
printf("\n");
+ printf("Options:\n");
+ print_option_help();
+ printf("\n");
+
printf("Commands:\n");
for (i=0; mpc_table[i].command; ++i) {
@@ -151,8 +155,7 @@ static int print_help(char * progname, char * command)
spaces," ",mpc_table[i].help);
}
- printf("For more information about these and other "
- "options look at man 1 mpc\n");
+ printf("\nSee man 1 mpc for more information about mpc commands and options\n");
return EXIT_SUCCESS;
}
diff --git a/src/options.c b/src/options.c
index 34f4f67..97ceb44 100644
--- a/src/options.c
+++ b/src/options.c
@@ -24,6 +24,8 @@
#include <string.h>
#include <stdio.h>
+#define MAX_LONGOPT_LENGTH 32
+
#define ERROR_UNKNOWN_OPTION 0x01
#define ERROR_BAD_ARGUMENT 0x02
#define ERROR_GOT_ARGUMENT 0x03
@@ -33,7 +35,7 @@ typedef struct {
int shortopt;
const char *longopt;
const char *argument;
- const char *descrition;
+ const char *description;
} arg_opt_t;
@@ -44,13 +46,13 @@ options_t options = {
};
static const arg_opt_t option_table[] = {
- { 'v', "verbose", NULL, "Verbose output" },
- { 'q', "quiet", NULL, "Don't print status" },
- { 'q', "no-status", NULL, "Don't print status" },
- { 'h', "host", "PORT", "Connect to server on host [" DEFAULT_HOST "]" },
- { 'P', "password", "PASSWORD", "Connect to server with password" },
- { 'p', "port", "HOST", "Connect to server on port [" DEFAULT_PORT "]" },
- { 'f', "format", "FORMAT", "Output status with format [" DEFAULT_FORMAT "]" }
+ { 'v', "verbose", NULL, "Give verbose output" },
+ { 'q', "quiet", NULL, "Suppress status message" },
+ { 'q', "no-status", NULL, "synonym for --quiet" },
+ { 'h', "host", "<host>", "Connect to server on <host>" },
+ { 'P', "password", "<password>", "Connect to server using password <password>" },
+ { 'p', "port", "<port>", "Connect to server port <port>" },
+ { 'f', "format", "<format>", "Print status with format <format>" },
};
static const unsigned option_table_size = sizeof(option_table) / sizeof(option_table[0]);
@@ -134,6 +136,24 @@ handle_option(int c, const char *arg)
}
void
+print_option_help(void)
+{
+ unsigned i;
+
+ for (i = 0; i < option_table_size; i++) {
+ printf(" -%c, ", option_table[i].shortopt);
+ if (option_table[i].argument)
+ printf("--%s=%-*s",
+ option_table[i].longopt,
+ 20 - strlen(option_table[i].longopt),
+ option_table[i].argument);
+ else
+ printf("--%-20s ", option_table[i].longopt);
+ printf("%s\n", option_table[i].description);
+ }
+}
+
+void
parse_options(int * argc_p, char ** argv)
{
int i;
diff --git a/src/options.h b/src/options.h
index 79b678e..255fd25 100644
--- a/src/options.h
+++ b/src/options.h
@@ -34,6 +34,7 @@ typedef struct {
} options_t;
+void print_option_help(void);
void parse_options(int * argc_p, char ** argv);
void options_init(void);
--
1.6.5.rc1.10.g20f34
From 8df767a13a108dfe7482779226e699c83a11cce9 Mon Sep 17 00:00:00 2001
From: Jeffrey Middleton <jefr...@gmail.com>
Date: Mon, 14 Sep 2009 11:02:01 -0500
Subject: [PATCH 5/5] doc: verb agreement in command descriptions
These should all use the imperative mood (the verbs shouldn't have an
's' on the end).
---
src/main.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/main.c b/src/main.c
index 0e8066c..12e8e44 100644
--- a/src/main.c
+++ b/src/main.c
@@ -75,11 +75,11 @@ static struct command {
{"playlist", 0, 0, 0, cmd_playlist, "", "Print the current playlist"},
{"listall", 0, -1, 2, cmd_listall, "[<file>]", "List all songs in the music dir"},
{"ls", 0, -1, 2, cmd_ls, "[<directory>]", "List the contents of <directory>"},
- {"lsplaylists", 0, -1, 2, cmd_lsplaylists, "", "Lists currently available playlists"},
+ {"lsplaylists", 0, -1, 2, cmd_lsplaylists, "", "List currently available playlists"},
{"load", 0, -1, 1, cmd_load, "<file>", "Load <file> as a playlist"},
- {"save", 1, 1, 0, cmd_save, "<file>", "Saves a playlist as <file>"},
- {"rm", 1, 1, 0, cmd_rm, "<file>", "Removes a playlist"},
- {"volume", 0, 1, 0, cmd_volume, "[+-]<num>", "Sets volume to <num> or adjusts by [+-]<num>"},
+ {"save", 1, 1, 0, cmd_save, "<file>", "Save a playlist as <file>"},
+ {"rm", 1, 1, 0, cmd_rm, "<file>", "Remove a playlist"},
+ {"volume", 0, 1, 0, cmd_volume, "[+-]<num>", "Set volume to <num> or adjusts by [+-]<num>"},
{"repeat", 0, 1, 0, cmd_repeat, "<on|off>", "Toggle repeat mode, or specify state"},
{"random", 0, 1, 0, cmd_random, "<on|off>", "Toggle random mode, or specify state"},
{"single", 0, 1, 0, cmd_single, "<on|off>", "Toggle single mode, or specify state"},
@@ -88,9 +88,9 @@ static struct command {
{"find", 2, -1, 0, cmd_find, "<type> <query>", "Find a song (exact match)"},
{"list", 1, -1, 0, cmd_list, "<type> [<type> <query>]", "Show all tags of <type>"},
{"crossfade", 0, 1, 0, cmd_crossfade, "[<seconds>]", "Set and display crossfade settings"},
- {"update", 0, -1, 2, cmd_update, "[<path>]", "Scans music directory for updates"},
- {"stats", 0, -1, 0, cmd_stats, "", "Displays statistics about MPD"},
- {"version", 0, 0, 0, cmd_version, "", "Reports version of MPD"},
+ {"update", 0, -1, 2, cmd_update, "[<path>]", "Scan music directory for updates"},
+ {"stats", 0, -1, 0, cmd_stats, "", "Display statistics about MPD"},
+ {"version", 0, 0, 0, cmd_version, "", "Report version of MPD"},
/* loadtab, lstab, and tab used for completion-scripting only */
{"loadtab", 0, 1, 0, cmd_loadtab, "<directory>", NULL},
{"lstab", 0, 1, 0, cmd_lstab, "<directory>", NULL},
@@ -140,7 +140,7 @@ static int print_help(char * progname, char * command)
}
}
- printf(" %s %*s Displays status\n",progname,max," ");
+ printf(" %s %*s Display status\n",progname,max," ");
for (i=0; mpc_table[i].command; ++i) {
int spaces;
--
1.6.5.rc1.10.g20f34
------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
_______________________________________________
Musicpd-dev-team mailing list
Musicpd-dev-team@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
