You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Nuutti Kotivuori <na...@iki.fi> on 2002/05/13 22:44:02 UTC
[PATCH] Pre-release version of ISO-8601 dates
Here's a pre-release patch of ISO-8601 date conversion for all of
subversions date needs.
Please do not apply it yet - unless you want to finish it. It probably
has errors and is largely uncommented and so on.
I'm posting this here, because it would be nice for people to explore
the possible ramifications this might have on several things in the
repository. For example an old client on a new working directory will
happily segfault at the first operation - which is not so surprising.
Anyway, hope to work more on this the next few days to get it
finished.
-- Naked
Index: ./build.conf
===================================================================
--- ./build.conf
+++ ./build.conf Mon May 13 23:13:16 2002
@@ -289,6 +289,14 @@
install = test
libs = libsvn_test libsvn_subr $(SVN_APRUTIL_LIBS) $(SVN_APR_LIBS)
+# test time functions
+[time-test]
+type = exe
+path = subversion/tests/libsvn_subr
+sources = time-test.c
+install = test
+libs = libsvn_test libsvn_subr $(SVN_APRUTIL_LIBS) $(SVN_APR_LIBS)
+
# test eol conversion and keyword substitution routines
[translate-test]
type = exe
Index: ./subversion/libsvn_subr/time.c
===================================================================
--- ./subversion/libsvn_subr/time.c
+++ ./subversion/libsvn_subr/time.c Mon May 13 23:10:11 2002
@@ -29,6 +29,18 @@
/*** Code. ***/
/* Our timestamp strings look like this:
+ *
+ * "2002-05-07Thh:mm:ss.uuuuuuZ"
+ *
+ * The format is conformant with ISO-8601 and the date format required
+ * by RFC2518 for creationdate. It is a direct converision between
+ * apr_time_t and a string, so converting to string and back retains
+ * the exact value.
+ */
+static const char * const timestamp_format =
+"%04d-%02d-%02dT%02d:%02d:%02d.%06dZ";
+
+/* Our old timestamp strings looked like this:
*
* "Tue 3 Oct 2000 HH:MM:SS.UUU (day 277, dst 1, gmt_off -18000)"
*
@@ -37,12 +49,10 @@
* to completely fill in an apr_time_exp_t: tm_yday, tm_isdst,
* and tm_gmtoff.
*
- * kff todo: what about portability problems resulting from the
- * plain int assumptions below, though? Using apr_strftime() would
- * fix that, but converting the strings back is still a problem (see
- * the comment in svn_wc__time_to_string()).
+ * This format is still recognized on input, for backward
+ * compatibility, but no longer generated.
*/
-static const char * const timestamp_format =
+static const char * const old_timestamp_format =
"%s %d %s %d %02d:%02d:%02d.%06d (day %03d, dst %d, gmt_off %06d)";
@@ -58,25 +68,24 @@
furthermore their current implementations can only return success
anyway. */
- apr_time_exp_lt (&exploded_time, t);
-
- /* It would be nice to use apr_strftime(), but APR doesn't give a way
- to convert back, so we wouldn't be able to share the format string
- between the writer and reader. Sigh. Also, apr_strftime() doesn't
- offer format codes for its special tm_usec and tm_gmtoff fields. */
+ /* We get the date in GMT now -- and expect the tm_gmtoff and
+ tm_isdst to be not set. We also ignore the weekday and yearday,
+ since those are not needed. */
+
+ apr_time_exp_gmt (&exploded_time, t);
+
+ /* It would be nice to use apr_strftime(), but APR doesn't give a
+ way to convert back, so we wouldn't be able to share the format
+ string between the writer and reader. */
t_cstr = apr_psprintf (pool,
timestamp_format,
- apr_day_snames[exploded_time.tm_wday],
- exploded_time.tm_mday,
- apr_month_snames[exploded_time.tm_mon],
exploded_time.tm_year + 1900,
+ exploded_time.tm_mon + 1,
+ exploded_time.tm_mday,
exploded_time.tm_hour,
exploded_time.tm_min,
exploded_time.tm_sec,
- exploded_time.tm_usec,
- exploded_time.tm_yday + 1,
- exploded_time.tm_isdst,
- exploded_time.tm_gmtoff);
+ exploded_time.tm_usec);
return t_cstr;
}
@@ -95,30 +104,6 @@
}
-/* ### todo:
-
- Recently Branko changed svn_time_from_string (or rather, he changed
- svn_wc__string_to_time, but the function's name has changed since
- then) to use apr_implode_gmt. So now that function is using GMT,
- but its inverse above, svn_time_to_string, is using localtime.
-
- I'm not sure what the right thing to do is; see issue #404. See
- also the thread entitled "apr_implode_time and time zones" in the
- APR dev mailing list archives:
-
- http://apr.apache.org/mail/dev/200106.gz
-
- That discussion between Branko and David Reid is directly
- relevant.
-
- Note that Subversion repositories probably want to record commit
- dates in GMT. Maybe we should just make Subversion's string
- representation for times always use GMT -- that's good for
- repositories, and for working copies it doesn't really matter since
- humans don't have to read the timestamps in SVN/entries files much
- (and when they do, they can easily do the conversion). */
-
-
apr_time_t
svn_time_from_nts (const char *data)
{
@@ -126,26 +111,54 @@
char wday[4], month[4];
apr_time_t when;
- sscanf (data,
- timestamp_format,
- wday,
- &exploded_time.tm_mday,
- month,
- &exploded_time.tm_year,
- &exploded_time.tm_hour,
- &exploded_time.tm_min,
- &exploded_time.tm_sec,
- &exploded_time.tm_usec,
- &exploded_time.tm_yday,
- &exploded_time.tm_isdst,
- &exploded_time.tm_gmtoff);
-
- exploded_time.tm_year -= 1900;
- exploded_time.tm_yday -= 1;
- exploded_time.tm_wday = find_matching_string (wday, apr_day_snames);
- exploded_time.tm_mon = find_matching_string (month, apr_month_snames);
-
- apr_implode_gmt (&when, &exploded_time);
+ /* First try the new timestamp format. */
+ if (sscanf (data,
+ timestamp_format,
+ &exploded_time.tm_year,
+ &exploded_time.tm_mon,
+ &exploded_time.tm_mday,
+ &exploded_time.tm_hour,
+ &exploded_time.tm_min,
+ &exploded_time.tm_sec,
+ &exploded_time.tm_usec) == 7)
+ {
+ exploded_time.tm_year -= 1900;
+ exploded_time.tm_mon -= 1;
+ exploded_time.tm_wday = 0;
+ exploded_time.tm_yday = 0;
+ exploded_time.tm_isdst = 0;
+ exploded_time.tm_gmtoff = 0;
+
+ apr_implode_gmt (&when, &exploded_time);
+ }
+ /* Then try the compatibility option. */
+ else if (sscanf (data,
+ old_timestamp_format,
+ wday,
+ &exploded_time.tm_mday,
+ month,
+ &exploded_time.tm_year,
+ &exploded_time.tm_hour,
+ &exploded_time.tm_min,
+ &exploded_time.tm_sec,
+ &exploded_time.tm_usec,
+ &exploded_time.tm_yday,
+ &exploded_time.tm_isdst,
+ &exploded_time.tm_gmtoff) == 11)
+ {
+ exploded_time.tm_year -= 1900;
+ exploded_time.tm_yday -= 1;
+ exploded_time.tm_wday = find_matching_string (wday, apr_day_snames);
+ exploded_time.tm_mon = find_matching_string (month, apr_month_snames);
+
+ apr_implode_gmt (&when, &exploded_time);
+ }
+ /* Timestamp is something we do not recognize. */
+ else
+ {
+ /* Better zero than something random. */
+ when = 0;
+ }
return when;
}
Index: ./subversion/tests/libsvn_subr/time-test.c
===================================================================
--- ./subversion/tests/libsvn_subr/time-test.c
+++ ./subversion/tests/libsvn_subr/time-test.c Mon May 13 22:20:49 2002
@@ -0,0 +1,165 @@
+/*
+ * path-test.c -- test the path functions
+ *
+ * ====================================================================
+ * Copyright (c) 2000-2002 CollabNet. All rights reserved.
+ *
+ * This software is licensed as described in the file COPYING, which
+ * you should have received as part of this distribution. The terms
+ * are also available at http://subversion.tigris.org/license-1.html.
+ * If newer versions of this license are posted there, you may use a
+ * newer version instead, at your option.
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals. For exact contribution history, see the revision
+ * history and logs, available at http://subversion.tigris.org/.
+ * ====================================================================
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <apr_general.h>
+#include "svn_time.h"
+#include "svn_test.h"
+
+
+apr_time_t test_timestamp = APR_TIME_C(1021316450966679);
+const char *test_timestring =
+"2002-05-13T19:00:50.966679Z";
+const char *test_old_timestring =
+"Mon 13 May 2002 22:00:50.966679 (day 133, dst 1, gmt_off 010800)";
+
+
+static svn_error_t *
+test_time_to_nts (const char **msg,
+ svn_boolean_t msg_only,
+ apr_pool_t *pool)
+{
+ const char *timestring;
+
+ *msg = "test svn_time_to_nts";
+
+ if (msg_only)
+ return SVN_NO_ERROR;
+
+ timestring = svn_time_to_nts(test_timestamp,pool);
+
+ if (strcmp(timestring,test_timestring) != 0)
+ {
+ return svn_error_createf
+ (SVN_ERR_TEST_FAILED, 0, NULL, pool,
+ "svn_time_to_nts (%" APR_TIME_T_FMT
+ ") returned date string '%s' instead of '%s'",
+ test_timestamp,timestring,test_timestring);
+ }
+
+ return SVN_NO_ERROR;
+}
+
+
+static svn_error_t *
+test_time_from_nts (const char **msg,
+ svn_boolean_t msg_only,
+ apr_pool_t *pool)
+{
+ apr_time_t timestamp;
+
+ *msg = "test svn_time_from_nts";
+
+ if (msg_only)
+ return SVN_NO_ERROR;
+
+ timestamp = svn_time_from_nts(test_timestring);
+
+ if (timestamp != test_timestamp)
+ {
+ return svn_error_createf
+ (SVN_ERR_TEST_FAILED, 0, NULL, pool,
+ "svn_time_from_nts (%s) returned time '%" APR_TIME_T_FMT
+ "' instead of '%" APR_TIME_T_FMT "'",
+ test_timestring,timestamp,test_timestamp);
+ }
+
+ return SVN_NO_ERROR;
+}
+
+
+static svn_error_t *
+test_time_from_nts_old (const char **msg,
+ svn_boolean_t msg_only,
+ apr_pool_t *pool)
+{
+ apr_time_t timestamp;
+
+ *msg = "test svn_time_from_nts (old format)";
+
+ if (msg_only)
+ return SVN_NO_ERROR;
+
+ timestamp = svn_time_from_nts(test_old_timestring);
+
+ if (timestamp != test_timestamp)
+ {
+ return svn_error_createf
+ (SVN_ERR_TEST_FAILED, 0, NULL, pool,
+ "svn_time_from_nts (%s) returned time '%" APR_TIME_T_FMT
+ "' instead of '%" APR_TIME_T_FMT "'",
+ test_old_timestring,timestamp,test_timestamp);
+ }
+
+ return SVN_NO_ERROR;
+}
+
+
+static svn_error_t *
+test_time_invariant (const char **msg,
+ svn_boolean_t msg_only,
+ apr_pool_t *pool)
+{
+ apr_time_t current_timestamp = apr_time_now();
+ const char *timestring;
+ apr_time_t timestamp;
+
+ *msg = "test svn_time_to_nts and svn_time_from_nts invariant";
+
+ if (msg_only)
+ return SVN_NO_ERROR;
+
+ timestring = svn_time_to_nts(current_timestamp,pool);
+ timestamp = svn_time_from_nts(timestring);
+
+ if (timestamp != current_timestamp)
+ {
+ return svn_error_createf
+ (SVN_ERR_TEST_FAILED, 0, NULL, pool,
+ "svn_time_from_nts ( svn_time_to_nts (n) ) returned time '%" APR_TIME_T_FMT
+ "' instead of '%" APR_TIME_T_FMT "'",
+ timestamp,current_timestamp);
+ }
+
+ return SVN_NO_ERROR;
+}
+
+
+
+/* The test table. */
+
+svn_error_t * (*test_funcs[]) (const char **msg,
+ svn_boolean_t msg_only,
+ apr_pool_t *pool) = {
+ 0,
+ test_time_to_nts,
+ test_time_from_nts,
+ test_time_from_nts_old,
+ test_time_invariant,
+ 0
+};
+
+
+
+/*
+ * local variables:
+ * eval: (load-file "../../../tools/dev/svn-dev.el")
+ * end:
+ */
+
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Pre-release version of ISO-8601 dates
Posted by Philip Martin <ph...@codematters.co.uk>.
Nuutti Kotivuori <na...@iki.fi> writes:
> Index: ./subversion/libsvn_subr/time.c
> ===================================================================
[snip]
> apr_time_t
> svn_time_from_nts (const char *data)
> {
It's not part of your current patch, but this function can obviously
fail. I think it should be
svn_error_t *
svn_time_from_nts(apr_time_t *when, const char *data, apr_pool_t *pool)
> @@ -126,26 +111,54 @@
> char wday[4], month[4];
> apr_time_t when;
>
> - sscanf (data,
> - timestamp_format,
> - wday,
> - &exploded_time.tm_mday,
> - month,
> - &exploded_time.tm_year,
> - &exploded_time.tm_hour,
> - &exploded_time.tm_min,
> - &exploded_time.tm_sec,
> - &exploded_time.tm_usec,
> - &exploded_time.tm_yday,
> - &exploded_time.tm_isdst,
> - &exploded_time.tm_gmtoff);
> -
> - exploded_time.tm_year -= 1900;
> - exploded_time.tm_yday -= 1;
> - exploded_time.tm_wday = find_matching_string (wday, apr_day_snames);
> - exploded_time.tm_mon = find_matching_string (month, apr_month_snames);
> -
> - apr_implode_gmt (&when, &exploded_time);
> + /* First try the new timestamp format. */
> + if (sscanf (data,
> + timestamp_format,
> + &exploded_time.tm_year,
> + &exploded_time.tm_mon,
> + &exploded_time.tm_mday,
> + &exploded_time.tm_hour,
> + &exploded_time.tm_min,
> + &exploded_time.tm_sec,
> + &exploded_time.tm_usec) == 7)
> + {
> + exploded_time.tm_year -= 1900;
> + exploded_time.tm_mon -= 1;
> + exploded_time.tm_wday = 0;
> + exploded_time.tm_yday = 0;
> + exploded_time.tm_isdst = 0;
> + exploded_time.tm_gmtoff = 0;
> +
> + apr_implode_gmt (&when, &exploded_time);
> + }
> + /* Then try the compatibility option. */
> + else if (sscanf (data,
> + old_timestamp_format,
> + wday,
> + &exploded_time.tm_mday,
> + month,
> + &exploded_time.tm_year,
> + &exploded_time.tm_hour,
> + &exploded_time.tm_min,
> + &exploded_time.tm_sec,
> + &exploded_time.tm_usec,
> + &exploded_time.tm_yday,
> + &exploded_time.tm_isdst,
> + &exploded_time.tm_gmtoff) == 11)
> + {
> + exploded_time.tm_year -= 1900;
> + exploded_time.tm_yday -= 1;
> + exploded_time.tm_wday = find_matching_string (wday, apr_day_snames);
> + exploded_time.tm_mon = find_matching_string (month, apr_month_snames);
> +
> + apr_implode_gmt (&when, &exploded_time);
> + }
> + /* Timestamp is something we do not recognize. */
> + else
> + {
> + /* Better zero than something random. */
> + when = 0;
Then we could return an error here, instead of silently ignoring the
problem.
> + }
>
> return when;
> }
--
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Pre-release version of ISO-8601 dates
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Greg Stein <gs...@lyra.org> writes:
> > So - what should be done about those function calls? If conversion to
> > function that can return errors and handling all callers is wanted, I
> > can try to do it. Or such a job could just as well be listed as just
> > another bitesized task. I would, personally, rather get this format
> > changing off my hands ASAP - and the error handling can be fixed after
> > that - but you may feel differently.
>
> +1 on doing it in two passes.
+1 here too.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Pre-release version of ISO-8601 dates
Posted by Greg Stein <gs...@lyra.org>.
On Tue, May 14, 2002 at 09:00:48AM +0300, Nuutti Kotivuori wrote:
>...
> So - what should be done about those function calls? If conversion to
> function that can return errors and handling all callers is wanted, I
> can try to do it. Or such a job could just as well be listed as just
> another bitesized task. I would, personally, rather get this format
> changing off my hands ASAP - and the error handling can be fixed after
> that - but you may feel differently.
+1 on doing it in two passes.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Pre-release version of ISO-8601 dates
Posted by Nuutti Kotivuori <na...@iki.fi>.
Karl Fogel wrote:
> Karl Fogel <kf...@newton.ch.collab.net> writes:
>> The doc string for svn_time_from_nts() in svn_time.h should mention
>> this behavior (of returning 0 when it can't parse the timestring).
>
> ...But Philip's suggestion to just return error instead is better!
>
> :-)
Philip Martin wrote:
> Nuutti Kotivuori <na...@iki.fi> writes:
>
>> Index: ./subversion/libsvn_subr/time.c
>> ===================================================================
>
> [snip]
>
>> apr_time_t
>> svn_time_from_nts (const char *data)
>> {
>
> It's not part of your current patch, but this function can obviously
> fail. I think it should be
>
> svn_error_t * svn_time_from_nts(apr_time_t *when, const char *data,
> apr_pool_t *pool)
Yes, returning an error would be good. Also, the used apr_time_exp_gmt
and apr_implode_gmt can return errors (well, atleast the latter can).
But this would mean to touch each and every invocation of
svn_time_from_nts - which is a bit bigger job.
So the returning zero is an intermediate form (the previous version
just silently returned random times) that I left as is for this
release of the patch. But a viable solution indeed is not to document
the returning of 0 - but to fix the logic.
So - what should be done about those function calls? If conversion to
function that can return errors and handling all callers is wanted, I
can try to do it. Or such a job could just as well be listed as just
another bitesized task. I would, personally, rather get this format
changing off my hands ASAP - and the error handling can be fixed after
that - but you may feel differently.
-- Naked
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Pre-release version of ISO-8601 dates
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Karl Fogel <kf...@newton.ch.collab.net> writes:
> The doc string for svn_time_from_nts() in svn_time.h should mention
> this behavior (of returning 0 when it can't parse the timestring).
...But Philip's suggestion to just return error instead is better!
:-)
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Pre-release version of ISO-8601 dates
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Nuutti Kotivuori <na...@iki.fi> writes:
> Here's a pre-release patch of ISO-8601 date conversion for all of
> subversions date needs.
Thanks, this stuff looks great!
Just a small comment:
> + apr_implode_gmt (&when, &exploded_time);
> + }
> + /* Timestamp is something we do not recognize. */
> + else
> + {
> + /* Better zero than something random. */
> + when = 0;
> + }
>
> return when;
> }
The doc string for svn_time_from_nts() in svn_time.h should mention
this behavior (of returning 0 when it can't parse the timestring).
(I haven't reviewed the new regression tests; guests just arrived, so
sending this now :-) ).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org