Re: [PATCH] Update SVN.pm

From: Stepan Kasal <kasal@ucw.cz>
Date: Apr 18, 2014, 06:48 UTC
Message-ID: <20140418064817.GB19647@camelia.ucw.cz>
In-Reply-To: <xmqqwqenety2.fsf@gitster.dls.corp.google.com>

Hello,

cc'ing Roman, the original author. (I should have done that in the first post, sorry. I have also forwarded him another mail from this thread, asking him for author's sign off.)

On Thu, Apr 17, 2014 at 10:39:49AM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> Stepan Kasal <kasal@ucw.cz> writes:
> 
> > On Wed, Apr 16, 2014 at 12:13:21PM -0700, Junio C Hamano wrote:
> >> Interesting.  What other strange forms can they record in their
> >> repositories, I have to wonder.  Can they do
> >>     2014-01-07T5:8:6.048176Z
> >> for example?
> >
> > Roman Belinsky, the author of this fix, witnessed after large scale
> > conversion that the problem happens with the hour part only.
> 
> Is this "large scale conversion" done from a SVN repository that is
> created by bog standard SVN, or something else?

I don't know.  Roman?

> How certain are we that this "hour part is broken" is the only kind
> of breakage in timestamps we would encouter?

I would say we can be certain, as Roman said that the same PC that inserts the timestamp with one-digit hours does not misformat minutes. (Still cited from the same discussion https://github.com/msysgit/git/pull/126#discussion_r9661916 )

We do not have code review for that bug, as far as I know, but this is a natural bug: a reasonably looking time "5:08:09.048176" is used in format "%sT%s"

> [...] and by being slightly more lenient than necessary to cover
> one observed case that triggered the patch, we can cover SVN
> repositories broken in a similar but slightly different way.

I second that, in general. But my guess is that this particular "similar but slightly different" breakage will never appear, so the self-documenting original fix wins for me.

> Especially given that this regexp matching is not used for finding a
> timestamp from random places [...]

I agree that the broader regexp is not dangerous in this context. So it seems to be no big issue either way.

Thanks for taking this so carefully,
	Stepan