[PATCH] Simplify reporting of API violations of the conflict resolver callback.

[Julian Foad <ju...@btopenworld.com>]

If the conflict resolver callback violates its API (e.g. returns NULL or
doesn't generate a result file when it says it has done), we presently
generate a custom error message:

>   if (result == NULL)
>     return svn_error_create(SVN_ERR_WC_CONFLICT_RESOLVER_FAILURE,
>                             NULL, _("Conflict callback violated API:"
>                                     " returned no results"));

This happens in 5 places and some are longer than the quoted example. My
instinct is to simplify by using just a SVN_ERR_ASSERT statement, like
this:

>   SVN_ERR_ASSERT(result);

See the attached patch.

Should I do this?

My reasons: I think it makes the normal code path easier to read. I
don't think we have custom error messages for other API violation error
returns. It gets rid of some messages that are meaningful only to
programmers but of limited use to programmers, and relieves the
translators of translating them. I can't think of strong
counter-arguments but I'm wary that there may be some.

- Julian

Re: [PATCH] Simplify reporting of API violations of the conflict resolver callback.

[Julian Foad <ju...@btopenworld.com>]

On Mon, 2008-07-14 at 19:05 -0400, Karl Fogel wrote:
> Julian Foad <ju...@btopenworld.com> writes:
> > Should I do this?
> >
> > My reasons: I think it makes the normal code path easier to read. I
> > don't think we have custom error messages for other API violation error
> > returns. It gets rid of some messages that are meaningful only to
> > programmers but of limited use to programmers, and relieves the
> > translators of translating them. I can't think of strong
> > counter-arguments but I'm wary that there may be some.
> 
> With conflict callbacks, it may be particularly easy to write a buggy
> callback, and the situation-specific error messages we give right now
> can help people figure out what they did wrong.
> 
> I have no evidence that the error message are or aren't helpful to
> people, but my instinct is to keep them.  Wish I could justify that with
> something stronger than "Let's not lose information" :-).

Oh well... I suspected some people would feel like that. To me it gets
in the way, just a little bit, of understanding the code, which is
important, but I guess there are other ways to tackle that.

- Juloi




---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Simplify reporting of API violations of the conflict resolver callback.

[Karl Fogel <kf...@red-bean.com>]

Julian Foad <ju...@btopenworld.com> writes:
> If the conflict resolver callback violates its API (e.g. returns NULL or
> doesn't generate a result file when it says it has done), we presently
> generate a custom error message:
>
>>   if (result == NULL)
>>     return svn_error_create(SVN_ERR_WC_CONFLICT_RESOLVER_FAILURE,
>>                             NULL, _("Conflict callback violated API:"
>>                                     " returned no results"));
>
> This happens in 5 places and some are longer than the quoted example. My
> instinct is to simplify by using just a SVN_ERR_ASSERT statement, like
> this:
>
>>   SVN_ERR_ASSERT(result);
>
> See the attached patch.
>
> Should I do this?
>
> My reasons: I think it makes the normal code path easier to read. I
> don't think we have custom error messages for other API violation error
> returns. It gets rid of some messages that are meaningful only to
> programmers but of limited use to programmers, and relieves the
> translators of translating them. I can't think of strong
> counter-arguments but I'm wary that there may be some.

With conflict callbacks, it may be particularly easy to write a buggy
callback, and the situation-specific error messages we give right now
can help people figure out what they did wrong.

I have no evidence that the error message are or aren't helpful to
people, but my instinct is to keep them.  Wish I could justify that with
something stronger than "Let's not lose information" :-).

-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org