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?
It's easier to search for a non-empty error message...
>
> Attached patch implements those two changes.
>
+1
