You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@wandisco.com> on 2010/11/10 17:09:06 UTC
[PATCH] Improve internal diff error messages
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
Re: [PATCH] Improve internal diff error messages
Posted by Julian Foad <ju...@wandisco.com>.
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
Re: [PATCH] Improve internal diff error messages
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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
Re: [PATCH] Improve internal diff error messages
Posted by Julian Foad <ju...@wandisco.com>.
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
Re: [PATCH] Improve internal diff error messages
Posted by Julian Foad <ju...@wandisco.com>.
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
Re: [PATCH] Improve internal diff error messages
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
+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/
I haven't studied the APR interface here; is it guaranteed
that baton.err_msg is not NULL?
> switch (opt_id)
> {