You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@lyra.org> on 2004/01/07 11:38:50 UTC
date parser
Before talking about why I vetoed putting the date parser into the 1.0
stabilization branch, I figured it would be best to review what a veto
means. Some people here will certainly be familiar with this, but I'm not
sure that's the case for everyone. The biggest thing, I think, is that a
veto doesn't have to be a big deal. In essence, it is merely a brake on
coding, pending discussion. So...
By definition, a veto only applies to code changes (rather than, say,
process discussions or release decisions). The veto must be accompanies by
a technical reason. There is some grey area on what "technical" can mean,
but the vetoer receives the benefit of the doubt. Along with the reason,
there must also be a willingness and an effort to work with the coder and
the community to work through the issues that are behind the reason for
the veto. IOW, unjustified or drive-by vetoes are not allowed; there must
be something that people can focus on and work together to solve. The
intent is always to remove the veto.
Vetoes can be applied before or after code is committed. There is also no
concept of a "statute of limitations." A commit from a year ago could be
vetoed as a bad idea for whatever reason (iow, just cuz it has been there
doesn't make it any less of a bad idea :-). If the veto arrives before the
code is committed, then it simply becomes a discussion before a commit can
occur. If the commit has already occurred, then the discussion also takes
place, but a release cannot be made which incorporates the code, unless or
until the veto is lifted or the commit is reverted.
Back to the matter at hand...
I'm only -0 on the concept of a date parser rewrite. If we were at another
time in our "release curve", then it would be no big deal. However, that
isn't the case here, so this kind of rewrite introduces a measure of risk
into the release. Today, getdate.y has not changed since we imported it
into the repository back in August, 2001; no idea about its history within
the old CVS repos. During that whole time, I don't recall any "big"
problems that people have actually experienced. It seems the largest issue
is missing the ISO formats, rather than a defect, per se. Against that
stability, we're talking about introducing new, untried code with some
number of potential issues.
The current patch still seems to have a few open issues on the recognized
formats (e.g. cvs/rcs formats, or use of a slash). At this point in the
release, I'd hope we would have a more solid closure/consensus on the
formats before application.
My second issue is that the patch is incomplete. I recognize that is
simply because it is meant as a "talking reference", but that is too loose
for application to the 1.0 branch. The Windows build still makes reference
to getdate.*, and there aren't any regression tests around this
functionality to give some confidence that we aren't introducing bugs.
The final point kind of balances itself out: this code doesn't get used or
exercised that much, so there isn't a lot of need to "fix" it. But by the
same token, that also means that introducing problems here will have a
very low impact.
I worry that even if each of these things were addressed, then at that
point, we'll be even further down the road where the risk/benefit tradeoff
is even more weighed against changes.
I believe the proper resolution is to recognize that the date formats are
just another "contract" (to borrow ghudson's terms w.r.t APIs). SVN will
make certain guarantees about specific forms of input. Pass something else
at your own risk. If you depend upon behavior outside of the contract,
then too bad if we need to alter code. We do this in many areas of the
code. Where we find implicit contracts, then we try to document those to
make them explicit. I'd like to posit that we're in the exact same
situation here. Our cmdline client has got a lot of implicit behaviors
that we try to document, and I believe that the acceptable formats are
simply one of those situations.
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: date parser
Posted by Russell Yanofsky <re...@columbia.edu>.
Greg Stein wrote:
> ...
> I believe the proper resolution is to recognize that the date formats
> are just another "contract" (to borrow ghudson's terms w.r.t APIs).
> SVN will make certain guarantees about specific forms of input. Pass
> something else at your own risk. If you depend upon behavior outside
> of the contract, then too bad if we need to alter code. We do this in
> many areas of the code. Where we find implicit contracts, then we try
> to document those to make them explicit. I'd like to posit that we're
> in the exact same situation here.
It's not the "exact same situtation" because it does more than just restrict
use of an interface, it also removes functionality. If you're going to
remove the ability for users to use dates like "yesterday" or "3 weeks ago"
or whatever, doing it before 1.0 will likely tick off a smaller number of
users and be easier to justify to them.
I don't have an opinion on the bigger issue. The current date parser is ugly
on so many levels, but the risks of replacing it now are also obvious.
- Russ
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: date parser
Posted by mark benedetto king <mb...@lowlatency.com>.
On Wed, Jan 07, 2004 at 03:38:50AM -0800, Greg Stein wrote:
> I'm only -0 on the concept of a date parser rewrite. If we were at another
> time in our "release curve", then it would be no big deal. However, that
I believe the exact opposite; it was never given priority because there
were better things to do, but now that we are where we are in the "release
curve" (or, more optimistically: the "adoption curve"), it becomes very
important that we fix certain things that need fixing before the toothpaste
is out of the tube.
> isn't the case here, so this kind of rewrite introduces a measure of risk
> into the release. Today, getdate.y has not changed since we imported it
> into the repository back in August, 2001; no idea about its history within
> the old CVS repos. During that whole time, I don't recall any "big"
> problems that people have actually experienced. It seems the largest issue
> is missing the ISO formats, rather than a defect, per se. Against that
> stability, we're talking about introducing new, untried code with some
> number of potential issues.
As Erik Huelsmann has pointed out, it's possible that it hasn't changed
because no one has dared change it. If the comments at the top are to
believed, it hasn't changed in 13 years, potentially because people were
too afraid. Personally, I dont believe that it was because people didn't
want to; I know that I had so much trouble with cvs's -D option that
I nearly gave up on it completely, rather than trying to sort out which
date formats actually did what I expected.
> The current patch still seems to have a few open issues on the recognized
> formats (e.g. cvs/rcs formats, or use of a slash). At this point in the
> release, I'd hope we would have a more solid closure/consensus on the
> formats before application.
One nice thing about this matcher is that it makes it easy for us to support
new formats. We can have a quick vote on the formats that should go in,
and use and document those.
> My second issue is that the patch is incomplete. I recognize that is
> simply because it is meant as a "talking reference", but that is too loose
> for application to the 1.0 branch. The Windows build still makes reference
> to getdate.*, and there aren't any regression tests around this
> functionality to give some confidence that we aren't introducing bugs.
C. Scott Ananian's patches came with a set of tests. I can integrate the
ones that are applicable.
>
> The final point kind of balances itself out: this code doesn't get used or
> exercised that much, so there isn't a lot of need to "fix" it. But by the
> same token, that also means that introducing problems here will have a
> very low impact.
>
> I worry that even if each of these things were addressed, then at that
> point, we'll be even further down the road where the risk/benefit tradeoff
> is even more weighed against changes.
Right, except as I noted before, I believe the opposite. :-)
>
> I believe the proper resolution is to recognize that the date formats are
> just another "contract" (to borrow ghudson's terms w.r.t APIs). SVN will
> make certain guarantees about specific forms of input. Pass something else
> at your own risk. If you depend upon behavior outside of the contract,
> then too bad if we need to alter code. We do this in many areas of the
> code. Where we find implicit contracts, then we try to document those to
> make them explicit. I'd like to posit that we're in the exact same
> situation here. Our cmdline client has got a lot of implicit behaviors
> that we try to document, and I believe that the acceptable formats are
> simply one of those situations.
>
This particular contract would be difficult to honor. It is not a matter
of "foo must not be NULL" or even "you must have called apr_initialize()
before you call apr_pool_create()"; it has to do with a difficult to
express limitation on the form of a text string, which is likely to have
been provided by a user (not generated algorithmically). Because the
data is likely to be user-provided, consumers of our APIs have three
choices:
1.) Pass the limitations of our contract on to their users. Or worse,
forget to do so (or even worse, forget to do "obvious" things like
add a mutex to protect both svn_opt_parse_revision and
svn_opt_args_to_target_array).
2.) Enforce the contract on their users (or worse, misenforce it). This
will require that they implement a validator of similar algorithmic
complexity to the one proposed.
3.) Eschew svn_opt_parse_revision and svn_opt_args_to_target_array in
favor of hand-rolled implementations under their control. This
could lead to several mutually incompatible svn revision date-format
domains, each with its own qualities (or lack thereof).
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: date parser
Posted by Daniel Stenberg <da...@haxx.se>.
On Wed, 7 Jan 2004, Greg Hudson wrote:
> The current date code is not thread-safe. That's a bug. Nobody cares right
> now because nobody is using the client code in a threaded program, but
> people will care eventually. If we continue to use getdate.y in svn 1.0, we
> will be put in the unpleasant situation of having to (practically) sacrifice
> compatibility in order to get thread-safety.
Making getdate.y thread-safe is not very hard. We already have a thread-safe
version of getdate.y in the curl project:
http://cvs.php.net/cvs.php/curl/lib/getdate.y
I'm not taking a position in this discussion, I'm only bringing this info for
others to use or ignore.
--
-=- Daniel Stenberg -=- http://daniel.haxx.se -=-
ech`echo xiun|tr nu oc|sed 'sx\([sx]\)\([xoi]\)xo un\2\1 is xg'`ol
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: date parser
Posted by Greg Hudson <gh...@MIT.EDU>.
I'd like to call out one particular response, and an associated point.
On Wed, 2004-01-07 at 10:14, kfogel@collab.net wrote:
> A. Document a limited number of formats that we support, state
> explicitly that using anything else is "at your own risk", with
> results neither defined nor guaranteed to be supported in future
> releases.
I think this plan is hopelessly naive. If svn 1.1 breaks, say, MM/DD/YY
date support, or changes its interpretation so that it sometimes means
DD/MM/YY, then we will be blamed as a moving target regardless of what
the svn 1.0 documentation says.
The current date code is not thread-safe. That's a bug. Nobody cares
right now because nobody is using the client code in a threaded program,
but people will care eventually. If we continue to use getdate.y in svn
1.0, we will be put in the unpleasant situation of having to
(practically) sacrifice compatibility in order to get thread-safety.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: date parser
Posted by Ron Bieber <ro...@bieberlabs.com>.
On Wed, 2004-01-07 at 17:37, Vincent Lefevre wrote:
> On 2004-01-07 11:34:31 -0500, mark benedetto king wrote:
> > It's more likely that since we currently claim "our date parser works
> > just like CVS's" and most of our users are familiar with CVS's, they
> > just don't bother complaining.
>
> Well, as a CVS and Subversion user, I *never* use absolute dates
> (mainly because the MM/DD/YY format is non-sense for me; probably
> ambiguous, misleading and counterintuitive for many European users).
> I just tried the ISO format in the past (with CVS), because this was
> the most logical way to provide a date, but it didn't work. Another
> problem with Subversion is that the date format isn't documented
> with "svn help" (or if it is, I don't know where).
Just as another point of view, having used CVS for quite a while to
manage a pretty large repository at my company, the only time I know of
that we use the date functionality is when an automated cruisecontrol
build kicks off, in which case the tool uses dates in it's cvs log
command to get changes since the last valid build. I'm much more
likely to check out by tag or individual revision (as is my staff).
With that said, however, I'm sure there are instances in which dates are
indispensable (say svn:external situations with mixed repositorys).
That functionality aside (and it not being available in CVS), I have
never had a good reason to pull source by date.
Having been a very excited subversion user here at home since 0.17 (and
by the way, waiting patiently for 1.0 so that I can justify converting
our CVS repository over because SVN is so much more elegant) I have not
yet had the need to pull source by date using subversion either.
As an aside, you guys have done a PHENOMENAL job in creating a viable
alternative to CVS. I have spent more time than I care to account for
talking about this "new version control tool" to my staff. While I
understand the arguments for changing the parser now, my experience
(however limited that might be) causes me to believe that a version 1.0
with the same parser as CVS is much more important to allow people to
convert than is the date parser, considering that anyone that IS using
dates is used to the quirkyness of CVS anyway.
For me, the publishing of a 1.0 release is the one "feature" I have been
waiting for to justify a full on conversion from CVS to SVN with my
management. The date parser doesn't add any weight to my arguments.
For those of us that already know how stable SVN is, we can try to
justify it to management by our experience, however in many companies,
beta doesn't cut it and the software must be "officially" released.
Just my $0.02
-- Ron
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: date parser
Posted by Vincent Lefevre <vi...@vinc17.org>.
On 2004-01-07 11:34:31 -0500, mark benedetto king wrote:
> It's more likely that since we currently claim "our date parser works
> just like CVS's" and most of our users are familiar with CVS's, they
> just don't bother complaining.
Well, as a CVS and Subversion user, I *never* use absolute dates
(mainly because the MM/DD/YY format is non-sense for me; probably
ambiguous, misleading and counterintuitive for many European users).
I just tried the ISO format in the past (with CVS), because this was
the most logical way to provide a date, but it didn't work. Another
problem with Subversion is that the date format isn't documented
with "svn help" (or if it is, I don't know where).
--
Vincent Lefèvre <vi...@vinc17.org> - Web: <http://www.vinc17.org/> - 100%
validated (X)HTML - Acorn Risc PC, Yellow Pig 17, Championnat International
des Jeux Mathématiques et Logiques, TETRHEX, etc.
Work: CR INRIA - computer arithmetic / SPACES project at LORIA
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: date parser
Posted by mark benedetto king <mb...@lowlatency.com>.
On Wed, Jan 07, 2004 at 09:14:05AM -0600, kfogel@collab.net wrote:
> "Erik Huelsmann" <e....@gmx.net> writes:
> > > Today, getdate.y has not changed since we imported it
> > > into the repository back in August, 2001; no idea about its history within
> > > the old CVS repos.
> >
> > Which might be a symptom of:
> > 1) Extremely nice and foreseeing coding; or
>
> Some evidence for (1) is that we hardly ever get bug reports about the
> date parsing.
Well, considering that issue #408 is the oldest issue still around,
it's possible that they all went to complain and noticed that there
was already an open issue. I doubt it, though.
It's more likely that since we currently claim "our date parser works
just like CVS's" and most of our users are familiar with CVS's, they
just don't bother complaining.
>
> > 2) Extremely obfuscated code which nobody dares touching and everybody has
> > something better to do anyway.
>
> If there had ever been a serious bug, sure we would have touched the
> code, no matter how obfuscated, and maybe rewritten it earlier. The
> fact that we haven't done so is still evidence of (1).
Any threaded access to svn_opt_parse_revision() is a latent bug. Care
to audit svnup, subclipse, pysvn, svk, ...?
>
> I like Greg Stein's solution:
>
> A. Document a limited number of formats that we support, state
> explicitly that using anything else is "at your own risk", with
> results neither defined nor guaranteed to be supported in future
> releases.
I've written separately about the potential pitfalls with this approach.
> B. Revisit the rewrite issue in a more leisurely fashion after 1.0.
> MBK's patch doesn't have to be wasted. It's approximately a
> bajillion times cleaner and more comprehensible than the current
> code, and easier to extend. So, if we decide to splice it in
> after 1.0, all we have to do is make sure it supports all the
> *documented* formats from (A), plus any new ones we want to get
> from the upgrade, and then we're done. Obviously, we'll try not
> to deprecate formats unnecessarily, but it's not a disaster if
> suddenly "3" stops being a valid date. We don't need to treat
> the loss of some edge formats as an earth-shaking disaster --
> it's just something to do a cost/benefit analysis on, like
> anything else.
"last thursday" and "two weeks ago" are exactly the kinds of reasonable
formats that people will probably become accustomed to, and that we'll
probably want to deprecate.
> > What has been marked as a long lasting problem is the number of dependencies
> > to build Subversion. Removing getdate.* might relieve us from one of those
> > dependencies.
>
> How big is this burden, really? We hardly ever heard about it before
> this thread, which only started because of the upcoming 1.0 release.
>
So far, we have a limited number of non-developers building from source.
Most developers probably have bison installed already, or know how to do
so.
As an aside, as a developer, I take warning messages about conflicts in
the grammar at compile time as a sign of inattention to detail, and that
colored my first impression of svn.
>
> It would be painful to discover a subtle date parsing bug right after
> releasing 1.0. By dropping in a new parser now, we increase the
> chance of that by a *lot* (because the current code has had orders of
> magnitude more exposure than the proposed replacement).
>
Forget the parser; I can see bugs in the *lexer* on casual inspection!
We're just ignoring them, because they don't make the client crash, and
if svn log doesn't do what you expected you just try, try again, and
blame your own incompetence rather than the date-parser's.
"You're surprised? Congratulations, you've discovered a feature!
Amaze your friends with your new-found knowledge!"
Because we're not classifying the host of problems with the existing
parser as bugs, any replacement is guaranteed to be much more risky
than doing nothing at all.
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: date parser
Posted by kf...@collab.net.
"Erik Huelsmann" <e....@gmx.net> writes:
> > Today, getdate.y has not changed since we imported it
> > into the repository back in August, 2001; no idea about its history within
> > the old CVS repos.
>
> Which might be a symptom of:
> 1) Extremely nice and foreseeing coding; or
Some evidence for (1) is that we hardly ever get bug reports about the
date parsing.
> 2) Extremely obfuscated code which nobody dares touching and everybody has
> something better to do anyway.
If there had ever been a serious bug, sure we would have touched the
code, no matter how obfuscated, and maybe rewritten it earlier. The
fact that we haven't done so is still evidence of (1).
> > During that whole time, I don't recall any "big"
> > problems that people have actually experienced. It seems the largest issue
> > is missing the ISO formats, rather than a defect, per se.
>
> The ISO date formats might be considered a defect in the code, since it is
> too hard to add them to the current code without overseeing all the
> consequences...
I like Greg Stein's solution:
A. Document a limited number of formats that we support, state
explicitly that using anything else is "at your own risk", with
results neither defined nor guaranteed to be supported in future
releases.
B. Revisit the rewrite issue in a more leisurely fashion after 1.0.
MBK's patch doesn't have to be wasted. It's approximately a
bajillion times cleaner and more comprehensible than the current
code, and easier to extend. So, if we decide to splice it in
after 1.0, all we have to do is make sure it supports all the
*documented* formats from (A), plus any new ones we want to get
from the upgrade, and then we're done. Obviously, we'll try not
to deprecate formats unnecessarily, but it's not a disaster if
suddenly "3" stops being a valid date. We don't need to treat
the loss of some edge formats as an earth-shaking disaster --
it's just something to do a cost/benefit analysis on, like
anything else.
> What has been marked as a long lasting problem is the number of dependencies
> to build Subversion. Removing getdate.* might relieve us from one of those
> dependencies.
How big is this burden, really? We hardly ever heard about it before
this thread, which only started because of the upcoming 1.0 release.
> > Against that
> > stability, we're talking about introducing new, untried code with some
> > number of potential issues.
>
> True.
It would be painful to discover a subtle date parsing bug right after
releasing 1.0. By dropping in a new parser now, we increase the
chance of that by a *lot* (because the current code has had orders of
magnitude more exposure than the proposed replacement).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: date parser
Posted by Erik Huelsmann <e....@gmx.net>.
[ snip ]
> Today, getdate.y has not changed since we imported it
> into the repository back in August, 2001; no idea about its history within
> the old CVS repos.
Which might be a symptom of:
1) Extremely nice and foreseeing coding; or
2) Extremely obfuscated code which nobody dares touching and everybody has
something better to do anyway.
> During that whole time, I don't recall any "big"
> problems that people have actually experienced. It seems the largest issue
> is missing the ISO formats, rather than a defect, per se.
The ISO date formats might be considered a defect in the code, since it is
too hard to add them to the current code without overseeing all the
consequences...
What has been marked as a long lasting problem is the number of dependencies
to build Subversion. Removing getdate.* might relieve us from one of those
dependencies.
> Against that
> stability, we're talking about introducing new, untried code with some
> number of potential issues.
True.
[ snip ]
> The final point kind of balances itself out: this code doesn't get used or
> exercised that much, so there isn't a lot of need to "fix" it. But by the
> same token, that also means that introducing problems here will have a
> very low impact.
And since the new date parser takes away *a lot* of functionality, we might
as well risk it. And make sure users don't get used to using it anyway. (even
without it being officially supported as you suggest below)
> I worry that even if each of these things were addressed, then at that
> point, we'll be even further down the road where the risk/benefit tradeoff
> is even more weighed against changes.
Which - by your own argument above - are to be considered slim, since
Subversion addresses it's changes across entire trees with a single ID: the
revnumber.
> I believe the proper resolution is to recognize that the date formats are
> just another "contract" (to borrow ghudson's terms w.r.t APIs). SVN will
> make certain guarantees about specific forms of input. Pass something else
> at your own risk. If you depend upon behavior outside of the contract,
> then too bad if we need to alter code. We do this in many areas of the
> code. Where we find implicit contracts, then we try to document those to
> make them explicit. I'd like to posit that we're in the exact same
> situation here. Our cmdline client has got a lot of implicit behaviors
> that we try to document, and I believe that the acceptable formats are
> simply one of those situations.
By the time we want to replace the old code with the currently new code,
people will want to keep their old formats: no way it's going to go if we allow
it now.
--
+++ GMX - die erste Adresse für Mail, Message, More +++
Neu: Preissenkung für MMS und FreeMMS! http://www.gmx.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: date parser
Posted by kv...@users.sourceforge.net.
Greg Hudson wrote:
[snip]
> (How is getdate.y incorrect? It's not thread-safe. It recognizes many
> things it shouldn't, and doesn't recognize some things it should. It
> parses MM/DD/YY[YY] in a US-locale-centric manner. Just for starters.)
Just to emphasize a non-technical point: In my opinion US interpretation
of a common date format is an important UI issue. The confusion for
international users is significant (particularly for non-techies).
Personally I would be happy with (unambiguous) ISO dates.
Regards
-Morten
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: date parser
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Jan 07, 2004 at 12:55:09PM -0500, Greg Hudson wrote:
> On Wed, 2004-01-07 at 06:38, Greg Stein wrote:
> > By definition, a veto only applies to code changes (rather than, say,
> > process discussions or release decisions).
>
> Except in this case, the STATUS file says "A change needs three +1
> votes... and no vetoes, to go into the release." So the meaning of this
> particular veto is rather different from what you described.
Euh. This *is* about a code change (replace the parser). What I meant
above is that a person can't say "I veto making the 0.36 release now." By
its very nature, votes and vetoes control what goes *into* a release.
> > The current patch still seems to have a few open issues on the recognized
> > formats (e.g. cvs/rcs formats, or use of a slash). At this point in the
> > release, I'd hope we would have a more solid closure/consensus on the
> > formats before application.
>
> This is FUD.
Easy, please. That is a loaded word. My point was that discussion is
ongoing, rather than "done". As I said, I would hope to see a much more
solid "done" before adjusting the 1.0 branch. I do agree that the basic
code appears to be "done".
> We know we can add formats more easily than we can take
> them away, so mbk chose to start conservatively. Given that, it's
> positively expected that someone will identify a candidate to be added.
Sure.
> That doesn't translate into an "open issue" with the patch.
*shrug* I see it as an issue. If the patch was, "we discussed the right
set of formats over the past few months, and it now implements the
consensus", then I wouldn't see an issue. As it is, since the set of
formats is *not* "closed", then I see us applying further tweaks to this
area. And the notion of "further tweaks" bothers me for a branch that is
trying to *avoid* tweakage.
(yet I do agree that in concrete terms the "further tweaks" will be *very*
minimal risk exposure, especially compared to the initial introduction of
the new parser)
> > My second issue is that the patch is incomplete. I recognize that is
> > simply because it is meant as a "talking reference", but that is too loose
>...
> I don't think the patch was meant as a "talking reference." This is the
Karl asked mbk for an patch so that a concrete discussion could be
started. Otherwise, it was looking to be an abstract "yah. the current
parser sucks. so now what?" type of discussion.
> first indication I've seen that the patch is incomplete.
The docs are incomplete, although I know that has been noted and a patch
is being developed for that.
>...
> that. The lack of regression tests does not seem like it should be
> considered a show-stopper, since we have no such tests around getdate.y.
>
> ("But we have three years of experience around getdate.y" is an obvious
> answer, but it lacks perspective. People don't use dates much, so we're
> unlikely to have found out if people encountered weird corner-case
> behavior with getdate.y.)
We right regression tests to look for, well, regressions :-). In this
case, we want to ensure that when the new parser is put into place, that
the formats we expect will be parsed properly. And that the formats which
are illegal will properly generate errors. Since we also expect to add a
format or two, that will change the ordering of trying out the various
formats. It is possible that an alteration of the ordering will cause a
particular input to be picked up by a new pattern format and, thus, be
parsed as a different value.
We don't have any regression tests because the code is assumed to be
correct and we haven't made any changes to it. If somebody made a change,
then we might have wanted tests to ensure that the introduction of those
changes would not have changed the interpretation of (some set of) certain
formats.
At some point in the future, it might be nice to have regression tests as
a way to generate code coverage reports, and that would have (hopefully)
covered the various behaviors of getdate.y. But that is an ideal position
for quite another time.
>...
> Regardless of how important a piece of code is, we should try to ensure
> that it is correct and maintainable--if not now, then eventually. I am
Agreed.
> adamant about fixing the date parser before 1.0 (now that mbk has
> provided an avenue to do so) because if we don't fix it now, we have no
> sane road map towards making it correct or maintainable later. (As I
> noted before, I don't consider your road map sane, even a little bit.)
Obviously, we disagree here :-), and that is the nature of the voting
process.
>...
> Karl wrote:
> > It would be painful to discover a subtle date parsing bug right after
> > releasing 1.0.
>
> No, not really. An obscure date-parsing bug seems like the epitome of
> painlessness, as far as bugs go. We'd fix it in 1.0.1 and that would be
> that.
And then we'd need to get that rolled out...
> (Also, we're parsing a handful of date formats here, not C code. There
> just aren't all that many corner cases.)
Then let's see a regression suite to verify that.
As I said, I'm -0 on the whole concept here, which means that there is
room for resolution. Gathering the pieces together here, I think that
means the following things, at least, are missing:
* fix Windows build
* add regression suite to validate new date parser
* doc changes
* not a holdup, but needs to be done: adjust packages' build reqts to
eliminate bison
I'll happily re-review at that point, and if there aren't any further
problems, then I'll change my vote to a -0.
I'd still rather see us specifying a contract and adjusting the docs, but
a -0 means I won't get in the way of the new parser.
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: date parser
Posted by Greg Hudson <gh...@MIT.EDU>.
On Wed, 2004-01-07 at 06:38, Greg Stein wrote:
> By definition, a veto only applies to code changes (rather than, say,
> process discussions or release decisions).
Except in this case, the STATUS file says "A change needs three +1
votes... and no vetoes, to go into the release." So the meaning of this
particular veto is rather different from what you described.
> The current patch still seems to have a few open issues on the recognized
> formats (e.g. cvs/rcs formats, or use of a slash). At this point in the
> release, I'd hope we would have a more solid closure/consensus on the
> formats before application.
This is FUD. We know we can add formats more easily than we can take
them away, so mbk chose to start conservatively. Given that, it's
positively expected that someone will identify a candidate to be added.
That doesn't translate into an "open issue" with the patch.
> My second issue is that the patch is incomplete. I recognize that is
> simply because it is meant as a "talking reference", but that is too loose
> for application to the 1.0 branch. The Windows build still makes reference
> to getdate.*, and there aren't any regression tests around this
> functionality to give some confidence that we aren't introducing bugs.
I don't think the patch was meant as a "talking reference." This is the
first indication I've seen that the patch is incomplete.
The Windows build is a good point. I'll see what I can do about fixing
that. The lack of regression tests does not seem like it should be
considered a show-stopper, since we have no such tests around getdate.y.
("But we have three years of experience around getdate.y" is an obvious
answer, but it lacks perspective. People don't use dates much, so we're
unlikely to have found out if people encountered weird corner-case
behavior with getdate.y.)
> The final point kind of balances itself out: this code doesn't get used or
> exercised that much, so there isn't a lot of need to "fix" it. But by the
> same token, that also means that introducing problems here will have a
> very low impact.
Regardless of how important a piece of code is, we should try to ensure
that it is correct and maintainable--if not now, then eventually. I am
adamant about fixing the date parser before 1.0 (now that mbk has
provided an avenue to do so) because if we don't fix it now, we have no
sane road map towards making it correct or maintainable later. (As I
noted before, I don't consider your road map sane, even a little bit.)
(How is getdate.y incorrect? It's not thread-safe. It recognizes many
things it shouldn't, and doesn't recognize some things it should. It
parses MM/DD/YY[YY] in a US-locale-centric manner. Just for starters.)
Karl wrote:
> It would be painful to discover a subtle date parsing bug right after
> releasing 1.0.
No, not really. An obscure date-parsing bug seems like the epitome of
painlessness, as far as bugs go. We'd fix it in 1.0.1 and that would be
that.
(Also, we're parsing a handful of date formats here, not C code. There
just aren't all that many corner cases.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org