Re: [PATCH] Improve internal diff error messages

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