On Wed, 2010-11-10, Daniel Shahaf wrote:
> Julian Foad wrote on Wed, Nov 10, 2010 at 17:06:07 +0000:
> > 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 *.
>
> Sorry; I should have noticed that :-(
>
> > > 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?
> >
>
> How about "(no message provided by APR)" instead of an empty string?
That wording would be just noise to an end-user - no added value.
I decided to change the approach to generate a sub-error containing
APR's message, and then wrap that in a generic message. Then it doesn't
matter if there is no sub-error.
Committed in r1033888.
- Julian
> It's easier to search for a non-empty error message...
>
> >
> > Attached patch implements those two changes.
> >
>
> +1
