{"thread":{"id":"36423","subject":"[PATCH] Update SVN.pm","startedAt":"2014-04-16T14:16:05Z","lastAt":"2014-04-18T06:48:17Z","messageCount":6,"participants":["Stepan Kasal","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"238935","messageId":"20140416141605.GA9692@camelia.ucw.cz","threadId":"36423","inReplyTo":null,"subject":"[PATCH] Update SVN.pm","fromName":"Stepan Kasal","fromEmail":"kasal@ucw.cz","sentAt":"2014-04-16T14:16:05Z","receivedAt":"2014-04-16T14:16:05Z","isPatch":true,"sender":{"key":"kasal@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/1481596?v=4"},"body":"From: RomanBelinsky \nDate: Tue, 11 Feb 2014 18:23:02 +0200\n\nfix parsing error for dates like:\n2014-01-07T5:58:36.048176Z\nprevious regex can parse only:\n2014-01-07T05:58:36.048176Z\nreproduced in my svn repository during conversion.\n\nSigned-off-by: Stepan Kasal \n---\n perl/Git/SVN.pm | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex a59564f..09cff13 100644\n--- a/perl/Git/SVN.pm\n+++ b/perl/Git/SVN.pm\n@@ -1321,7 +1321,7 @@ sub get_untracked {\n sub parse_svn_date {\n \tmy $date = shift || return '+0000 1970-01-01 00:00:00';\n \tmy ($Y,$m,$d,$H,$M,$S) = ($date =~ /^(\\d{4})\\-(\\d\\d)\\-(\\d\\d)T\n-\t (\\d\\d)\\:(\\d\\d)\\:(\\d\\d)\\.\\d*Z$/x) or\n+\t (\\d\\d?)\\:(\\d\\d)\\:(\\d\\d)\\.\\d*Z$/x) or\n \t croak \"Unable to parse date: $date\\n\";\n \tmy $parsed_date; # Set next.\n \n-- \n1.9.2.msysgit.0.154.g978f18d\n"},{"id":"238977","messageId":"xmqq61m9gka6.fsf@gitster.dls.corp.google.com","threadId":"36423","inReplyTo":"20140416141605.GA9692@camelia.ucw.cz","subject":"Re: [PATCH] Update SVN.pm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-16T19:13:21Z","receivedAt":"2014-04-16T19:13:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stepan Kasal writes:\n\n> From: RomanBelinsky \n> Date: Tue, 11 Feb 2014 18:23:02 +0200\n>\n> fix parsing error for dates like:\n> 2014-01-07T5:58:36.048176Z\n> previous regex can parse only:\n> 2014-01-07T05:58:36.048176Z\n> reproduced in my svn repository during conversion.\n\nInteresting. What other strange forms can they record in their\nrepositories, I have to wonder. Can they do\n\n 2014-01-07T5:8:6.048176Z\n\nfor example? I am wondering if it is simpler and less error prone\nto turn all these \"we only accept two digits\" into \"\\d+\" not only\nfor the hour part but also minute and second parts.\n\n> Signed-off-by: Stepan Kasal \n> ---\n> perl/Git/SVN.pm | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\n> index a59564f..09cff13 100644\n> --- a/perl/Git/SVN.pm\n> +++ b/perl/Git/SVN.pm\n> @@ -1321,7 +1321,7 @@ sub get_untracked {\n> sub parse_svn_date {\n> \tmy $date = shift || return '+0000 1970-01-01 00:00:00';\n> \tmy ($Y,$m,$d,$H,$M,$S) = ($date =~ /^(\\d{4})\\-(\\d\\d)\\-(\\d\\d)T\n> -\t (\\d\\d)\\:(\\d\\d)\\:(\\d\\d)\\.\\d*Z$/x) or\n> +\t (\\d\\d?)\\:(\\d\\d)\\:(\\d\\d)\\.\\d*Z$/x) or\n> \t croak \"Unable to parse date: $date\\n\";\n> \tmy $parsed_date; # Set next.\n"},{"id":"238999","messageId":"20140417052438.GA13907@camelia.ucw.cz","threadId":"36423","inReplyTo":"xmqq61m9gka6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Update SVN.pm","fromName":"Stepan Kasal","fromEmail":"kasal@ucw.cz","sentAt":"2014-04-17T05:24:38Z","receivedAt":"2014-04-17T05:24:38Z","isPatch":true,"sender":{"key":"kasal@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/1481596?v=4"},"body":"Hello,\n\nOn Wed, Apr 16, 2014 at 12:13:21PM -0700, Junio C Hamano wrote:\n> Interesting. What other strange forms can they record in their\n> repositories, I have to wonder. Can they do\n> 2014-01-07T5:8:6.048176Z\n> for example?\n\nRoman Belinsky, the author of this fix, witnessed after large scale\nconversion that the problem happens with the hour part only.\n(SVN commits from the same origin did this with hours but not with\nminutes.) Recorded here:\nhttps://github.com/msysgit/git/pull/126#discussion_r9661916\n\n> I am wondering if it is simpler and less error prone\n> to turn all these \"we only accept two digits\" into \"\\d+\" not only\n> for the hour part but also minute and second parts.\n\nBut Roman's proposed regexp nicely shows 1) what the standard is and\n2) what is the deviation.\n\nHave a nice day,\n Stepan Kasal\n\n> > Signed-off-by: Stepan Kasal \n> > ---\n> > perl/Git/SVN.pm | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\n> > index a59564f..09cff13 100644\n> > --- a/perl/Git/SVN.pm\n> > +++ b/perl/Git/SVN.pm\n> > @@ -1321,7 +1321,7 @@ sub get_untracked {\n> > sub parse_svn_date {\n> > \tmy $date = shift || return '+0000 1970-01-01 00:00:00';\n> > \tmy ($Y,$m,$d,$H,$M,$S) = ($date =~ /^(\\d{4})\\-(\\d\\d)\\-(\\d\\d)T\n> > -\t (\\d\\d)\\:(\\d\\d)\\:(\\d\\d)\\.\\d*Z$/x) or\n> > +\t (\\d\\d?)\\:(\\d\\d)\\:(\\d\\d)\\.\\d*Z$/x) or\n> > \t croak \"Unable to parse date: $date\\n\";\n> > \tmy $parsed_date; # Set next.\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"239026","messageId":"xmqqwqenety2.fsf@gitster.dls.corp.google.com","threadId":"36423","inReplyTo":"20140417052438.GA13907@camelia.ucw.cz","subject":"Re: [PATCH] Update SVN.pm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-17T17:39:49Z","receivedAt":"2014-04-17T17:39:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stepan Kasal writes:\n\n> On Wed, Apr 16, 2014 at 12:13:21PM -0700, Junio C Hamano wrote:\n>> Interesting. What other strange forms can they record in their\n>> repositories, I have to wonder. Can they do\n>> 2014-01-07T5:8:6.048176Z\n>> for example?\n>\n> Roman Belinsky, the author of this fix, witnessed after large scale\n> conversion that the problem happens with the hour part only.\n\nIs this \"large scale conversion\" done from a SVN repository that is\ncreated by bog standard SVN, or something else? How certain are we\nthat this \"hour part is broken\" is the only kind of breakage in\ntimestamps we would encouter?\n\nWhat I am trying to get at is that \"we didn't see any breakage at\npositions other than hour part after checking 2 million commits\" is\ndifferent from \"there will no breakage at positions other than hour\npart\", and by being slightly more lenient than necessary to cover\none observed case that triggered the patch, we can cover SVN\nrepositories broken in a similar but slightly different way.\n\nEspecially given that this regexp matching is not used for finding a\ntimestamp from random places but to parse out the datum we find at a\nplace where we expect to see a timestamp (check the callers), I\nthink loosening to allow single-digit minutes and seconds in the\nsame commit that allows single-digit hours would be such \"slightly\nmore lenient than necessary\" change without additional risk of\nmistaking something that is not a timestamp as a timestamp.\n\nThanks.\n"},{"id":"239028","messageId":"xmqqoazzesyi.fsf@gitster.dls.corp.google.com","threadId":"36423","inReplyTo":"20140416141605.GA9692@camelia.ucw.cz","subject":"Re: [PATCH] Update SVN.pm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-17T18:01:09Z","receivedAt":"2014-04-17T18:01:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stepan Kasal writes:\n\n> From: RomanBelinsky \n> Date: Tue, 11 Feb 2014 18:23:02 +0200\n>\n> fix parsing error for dates like:\n> 2014-01-07T5:58:36.048176Z\n> previous regex can parse only:\n> 2014-01-07T05:58:36.048176Z\n> reproduced in my svn repository during conversion.\n>\n> Signed-off-by: Stepan Kasal \n> ---\n\nTwo niggles.\n\n - The \"Subject\" line is not descriptive enough to let readers of \"git\n shortlog\" know what this change is about.\n\n - Can we have the patch signed-off by the author?\n\n\nFor the first point, I'd suggest rewriting the proposed commit\nmessage like this (this is what I came up with after reading that\nmsysgit discussion page you referred to in the other message):\n\n------------------------------------------------------\nSVN.pm::parse_svn_date: allow timestamps with a single-digit hour\n\nSome broken subversion server gives timestamps with only one digit\nin the hour part, like this:\n\n 2014-01-07T5:58:36.048176Z\n\nLoosen the regexp that expected to see two-digit hour, minute and\nsecond parts to accept a single-digit hour (but not minute or\nsecond).\n\nSigned-off-by: Stepan Kasal \n------------------------------------------------------\n\n\n> perl/Git/SVN.pm | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\n> index a59564f..09cff13 100644\n> --- a/perl/Git/SVN.pm\n> +++ b/perl/Git/SVN.pm\n> @@ -1321,7 +1321,7 @@ sub get_untracked {\n> sub parse_svn_date {\n> \tmy $date = shift || return '+0000 1970-01-01 00:00:00';\n> \tmy ($Y,$m,$d,$H,$M,$S) = ($date =~ /^(\\d{4})\\-(\\d\\d)\\-(\\d\\d)T\n> -\t (\\d\\d)\\:(\\d\\d)\\:(\\d\\d)\\.\\d*Z$/x) or\n> +\t (\\d\\d?)\\:(\\d\\d)\\:(\\d\\d)\\.\\d*Z$/x) or\n> \t croak \"Unable to parse date: $date\\n\";\n> \tmy $parsed_date; # Set next.\n"},{"id":"239061","messageId":"20140418064817.GB19647@camelia.ucw.cz","threadId":"36423","inReplyTo":"xmqqwqenety2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Update SVN.pm","fromName":"Stepan Kasal","fromEmail":"kasal@ucw.cz","sentAt":"2014-04-18T06:48:17Z","receivedAt":"2014-04-18T06:48:17Z","isPatch":true,"sender":{"key":"kasal@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/1481596?v=4"},"body":"Hello,\n\ncc'ing Roman, the original author. (I should have done that\nin the first post, sorry. I have also forwarded him another\nmail from this thread, asking him for author's sign off.)\n\nOn Thu, Apr 17, 2014 at 10:39:49AM -0700, Junio C Hamano wrote:\n> Stepan Kasal writes:\n> \n> > On Wed, Apr 16, 2014 at 12:13:21PM -0700, Junio C Hamano wrote:\n> >> Interesting. What other strange forms can they record in their\n> >> repositories, I have to wonder. Can they do\n> >> 2014-01-07T5:8:6.048176Z\n> >> for example?\n> >\n> > Roman Belinsky, the author of this fix, witnessed after large scale\n> > conversion that the problem happens with the hour part only.\n> \n> Is this \"large scale conversion\" done from a SVN repository that is\n> created by bog standard SVN, or something else?\n\nI don't know. Roman?\n\n> How certain are we that this \"hour part is broken\" is the only kind\n> of breakage in timestamps we would encouter?\n\nI would say we can be certain, as Roman said that the same PC\nthat inserts the timestamp with one-digit hours does not misformat\nminutes. (Still cited from the same discussion\nhttps://github.com/msysgit/git/pull/126#discussion_r9661916 )\n\nWe do not have code review for that bug, as far as I know, but this\nis a natural bug: a reasonably looking time \"5:08:09.048176\" is\nused in format \"%sT%s\"\n\n> [...] and by being slightly more lenient than necessary to cover\n> one observed case that triggered the patch, we can cover SVN\n> repositories broken in a similar but slightly different way.\n\nI second that, in general.\nBut my guess is that this particular \"similar but slightly\ndifferent\" breakage will never appear, so the self-documenting\noriginal fix wins for me.\n\n> Especially given that this regexp matching is not used for finding a\n> timestamp from random places [...]\n\nI agree that the broader regexp is not dangerous in this context. So\nit seems to be no big issue either way.\n\nThanks for taking this so carefully,\n\tStepan\n"}]}