I noticed some ugliness when I ran svn like this:
$ svn diff --internal-diff
svn: Error parsing diff options: Bad character specified on command line
Huh? I didn't specify a bad character on the command line. What
character, anyway?
The problem was that my configuation file specified GNU diff as the
default diff command and GNU diff options including "-U6" as the default
diff options.
The following patch improves the situation.
My only concern is that it assumes the option-parsing error messages
produced by APR will always make sense when prefixed by "svn: Internal
diff: ". I can't guarantee that for all versions of APR.
Good enough?
- Julian
Improve option-parsing error messages from the internal diff: report which
option was bad, and don't mention the command line.
An example, assuming the config file contains "diff-extensions = -U6 -p":
Before this patch:
$ svn diff --internal-diff
svn: Error parsing diff options: Bad character specified on command line
After this patch:
$ svn diff --internal-diff
svn: Internal diff: invalid option character: U
* subversion/libsvn_diff/diff_file.c
(opt_parsing_error_baton_t): New struct.
(opt_parsing_error_func): New function.
(svn_diff_file_options_parse): Use opt_parsing_error_func() to capture any
error message produced by apr_getopt_long() and use that detailed
message instead of a more generic error message if parsing fails.
--This line, and those below, will be ignored--
Index: subversion/libsvn_diff/diff_file.c
===================================================================
--- subversion/libsvn_diff/diff_file.c (revision 1033540)
+++ subversion/libsvn_diff/diff_file.c (working copy)
@@ -562,12 +562,42 @@ svn_diff_file_options_create(apr_pool_t
return apr_pcalloc(pool, sizeof(svn_diff_file_options_t));
}
+/* A baton for use with opt_parsing_error_func(). */
+struct opt_parsing_error_baton_t
+{
+ const char *err_msg;
+ apr_pool_t *pool;
+};
+
+/* A function to receive and store an error message from apr_getopt_long().
+ * This enables us to get the detailed error message including which option
+ * was wrong.
+ * Implements apr_getopt_err_fn_t. */
+static void
+opt_parsing_error_func(void *baton,
+ const char *fmt, ...)
+{
+ struct opt_parsing_error_baton_t *b = baton;
+ va_list ap;
+
+ va_start(ap, fmt);
+ b->err_msg = apr_pvsprintf(b->pool, fmt, ap);
+ va_end(ap);
+
+ /* Skip the error message's leading ": " if present. This exploits
+ * knowledge that apr_getopt_long's error message sometimes (or perhaps
+ * always) begins with ": " and then the message. */
+ if (strncmp(b->err_msg, ": ", 2) == 0)
+ b->err_msg += 2;
+}
+
svn_error_t *
svn_diff_file_options_parse(svn_diff_file_options_t *options,
const apr_array_header_t *args,
apr_pool_t *pool)
{
apr_getopt_t *os;
+ struct opt_parsing_error_baton_t opt_parsing_error_baton = { NULL, pool };
/* Make room for each option (starting at index 1) plus trailing NULL. */
const char **argv = apr_palloc(pool, sizeof(char*) * (args->nelts + 2));
@@ -576,8 +606,9 @@ svn_diff_file_options_parse(svn_diff_fil
argv[args->nelts + 1] = NULL;
apr_getopt_init(&os, pool, args->nelts + 1, argv);
- /* No printing of error messages, please! */
- os->errfn = NULL;
+ os->errfn = opt_parsing_error_func;
+ os->errarg = &opt_parsing_error_baton;
+
while (1)
{
const char *opt_arg;
@@ -587,7 +618,12 @@ svn_diff_file_options_parse(svn_diff_fil
if (APR_STATUS_IS_EOF(err))
break;
if (err)
- return svn_error_wrap_apr(err, _("Error parsing diff options"));
+ /* Avoid displaying APR's generic error message associated with
+ * status code ERR, because at least one such message refers to the
+ * "command line" which may not be where our options came from. */
+ return svn_error_createf(SVN_ERR_INVALID_DIFF_OPTION, NULL,
+ _("Internal diff: %s"),
+ opt_parsing_error_baton.err_msg);
switch (opt_id)
{
