On Wed, 2010-11-10, Daniel Shahaf wrote:
> +1 in genenral, except:
>
> Julian Foad wrote on Wed, Nov 10, 2010 at 16:09:06 +0000:
> > @@ -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);
> >
>
> s/NULL/err/
No, because 'err' is an APR status code, not an svn_error_t *. The only
two values it can have here are
* APR_BADCH -- Found a bad option character
* APR_BADARG -- No argument followed the option flag
Maybe safer to say "Error in options to internal diff: %s", in case some
of the sub-messages don't refer explicitly to "options".
> I haven't studied the APR interface here; is it guaranteed
> that baton.err_msg is not NULL?
I think so. apr_getopt_long() says "On error, a message will be printed
to stdout unless os->err is set to 0.". Hmm, that's not accurate: it's
supposed to be printed via os->errfn, not necessarily to stdout. And
that's "errfn"; there is no "os->err".
I guess we can't trust the doc string too much. It would be safer to
provide a default, in case it sometimes doesn't call os->errfn. How
about we just initialize it to an empty string? The result wouldn't
look 100% perfect if there are such cases, but close enough?
Attached patch implements those two changes.
- 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 = { "", 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,
+ _("Error in options to internal diff: %s"),
+ opt_parsing_error_baton.err_msg);
switch (opt_id)
{
