Re: [REVIEW]: ISO 8601 Relaxation With Err Msg's

On Wed, 2005-07-20 at 02:22 -0400, Greg Hudson wrote:
> I don't really support relaxing the date formats (for reasons I can't
> necessarily articulate, other than that we'd be departing from the ISO
> date format spec), but I also found a bunch of things wrong with this
> patch.

I should have been more precise. I am relaxing only the _input_ formats
from the user. Subversion should definitely stick to the ISO-8601
standard when it _outputs_ any date.

This was discussed on both user and dev lists, here is a good message
near the end of the thread stating the conclusions.

   http://svn.haxx.se/dev/archive-2005-07/0335.shtml

> First and foremost, you combined two changes which can easily be
> separated: relaxing the date formats, and changing the handling of
> date-parsing errors.

A split is easy to do and will expose that relaxing the date inputs does
not cause the test to fail.

> You also used C++-style comments, and generally didn't adhere to our
> formatting rules or even use internally consistent indentation.
>
> In svn_opt.h:
>
> -int svn_opt_parse_revision (svn_opt_revision_t *start_revision,
> +svn_error_t *
> + svn_opt_parse_revision (svn_opt_revision_t *start_revision,
>
> You can't change the signature of a function in the Subversion API. You
> have to rev it (look in our headers for other examples of how interfaces
> have been revved).
>
> In opt.c:
>
> + @a ret_error allows us to pass up error data without needing to
> + completely refactor the rest of this function.
>
> This is design through accretion. We don't do that.
>
> + // This error message is overridden by the one in the client.
> + return svn_error_createf (SVN_ERR_BAD_DATE, NULL,
> + "Date not in ISO-8601 or GNU format");
>
> Why?

I'm green and don't know exactly what I'm doing, hence the review and
not a patch as I struggle to come to terms with SVN's innards.

As for why the message is overwritten, the mainline code in the client
adds the string to the error message after verifying that it is UTF8
passable. Is this a good thing to do or can I append to the head of the
error message string after passing it up?

> + if (str_err == SVN_NO_ERROR)
> + return svn_cmdline_handle_exit_error (err, pool, "svn:
> ");
>
> You're comparing a char * to a constant intended to be used with
> svn_error_t *s. This block in general looks confused.

str_err is an error struct, perhaps I chose a bad name for it.

+++ subversion/clients/cmdline/main.c (working copy)
@@ -803,7 +803,7 @@
 int
 main (int argc, const char * const *argv)
 {
- svn_error_t *err;
+ svn_error_t *err, *str_err;

I do need to rethink the logic of that block of code. I'm missing some
braces which could clear things up.

I'll take some time this week and see if I can make a cleaner patch.

Thanks for the feedback,
Chris

-- 
Wireless Group
McMaster University
summer
14:23:16 up 7 days, 5:14, 2 users, load average: 0.37, 0.15, 0.15
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org