threads / patch / 16759
Subject: [EGIT PATCH 0/2] *** SUBJECT HERE ***
Robin RosenbergThe few plugin unit tests we have is a sad story. Cheer up a bit.
Robin Rosenberg (2):
Revert "Fix commit id in egit test
T0001_ConnectProviderOperationTest"
Fixed an old failed EGit unit test..../egit/core/internal/mapping/T0002_history.java | 5 ++--- .../op/T0001_ConnectProviderOperationTest.java | 3 ++- 2 files changed, 4 insertions(+), 4 deletions(-)
Robin Rosenberg[EGIT PATCH 1/2] Revert "Fix commit id in egit test T0001_ConnectProviderOperationTest"
This reverts commit 61133091d5f22398828b350ff772165e9945db8a.
Bisect says this is the commit that failed, which is odd. Bad QA.
Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com> --- .../op/T0001_ConnectProviderOperationTest.java | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/org.spearce.egit.core.test/src/org/spearce/egit/core/op/T0001_ConnectProviderOperationTest.java b/org.spearce.egit.core.test/src/org/spearce/egit/core/op/T0001_ConnectProviderOperationTest.java index 0ce2d7f..aae1ef4 100644 --- a/org.spearce.egit.core.test/src/org/spearce/egit/core/op/T0001_ConnectProviderOperationTest.java +++ b/org.spearce.egit.core.test/src/org/spearce/egit/core/op/T0001_ConnectProviderOperationTest.java @@ -99,8 +99,9 @@ assertTrue("tree missing", new File(gitDir, "objects/08/ccc3d91a14d337a45f355d3d63bd97fd5e4db9").exists()); assertTrue("tree missing", new File(gitDir, "objects/9d/aeec817090098f05eeca858e3a552d78b0a346").exists()); + assertTrue("commit missing", new File(gitDir, - "objects/09/6f1a84091b90b6d9fb12f95848da69496305c1").exists()); + "objects/45/df73fd9abbc2c61620c036948b1157e4d21253").exists()); ConnectProviderOperation operation = new ConnectProviderOperation( project.getProject(), null);
-- 1.6.0.3.640.g6331a
Robin Rosenberg[EGIT PATCH 2/2] Fixed an old failed EGit unit test.
The index was dropped from the history with Shawns revision walker.
Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com> --- .../egit/core/internal/mapping/T0002_history.java | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/org.spearce.egit.core.test/src/org/spearce/egit/core/internal/mapping/T0002_history.java b/org.spearce.egit.core.test/src/org/spearce/egit/core/internal/mapping/T0002_history.java index b540e10..71f5cc5 100644 --- a/org.spearce.egit.core.test/src/org/spearce/egit/core/internal/mapping/T0002_history.java +++ b/org.spearce.egit.core.test/src/org/spearce/egit/core/internal/mapping/T0002_history.java @@ -98,8 +98,7 @@ public void testShallowHistory() { IFileHistoryProvider fileHistoryProvider = provider.getFileHistoryProvider(); IFileHistory fileHistory = fileHistoryProvider.getFileHistoryFor(project.getProject().getWorkspace().getRoot().findMember("Project-1/A.txt"), IFileHistoryProvider.SINGLE_LINE_OF_DESCENT, new NullProgressMonitor()); IFileRevision[] fileRevisions = fileHistory.getFileRevisions(); - assertEquals(2, fileRevisions.length); - assertEquals("Index", fileRevisions[0].getContentIdentifier()); - assertEquals("6dd8f0b51204fa24a01734971947847549ec4ba8", fileRevisions[1].getContentIdentifier()); + assertEquals(1, fileRevisions.length); + assertEquals("6dd8f0b51204fa24a01734971947847549ec4ba8", fileRevisions[0].getContentIdentifier()); } }
-- 1.6.0.3.640.g6331a
Shawn O. PearceRe: [EGIT PATCH 1/2] Revert "Fix commit id in egit test T0001_ConnectProviderOperationTest"
Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> This reverts commit 61133091d5f22398828b350ff772165e9945db8a. > > Bisect says this is the commit that failed, which is odd. Bad QA.
Hmmph. This revert fails here.
> diff --git a/org.spearce.egit.core.test/src/org/spearce/egit/core/op/T0001_ConnectProviderOperationTest.java b/org.spearce.egit.core.test/src/org/spearce/egit/core/op/T0001_ConnectProviderOperationTest.java
> index 0ce2d7f..aae1ef4 100644
> --- a/org.spearce.egit.core.test/src/org/spearce/egit/core/op/T0001_ConnectProviderOperationTest.java
> +++ b/org.spearce.egit.core.test/src/org/spearce/egit/core/op/T0001_ConnectProviderOperationTest.java
> @@ -99,8 +99,9 @@ assertTrue("tree missing", new File(gitDir,
> "objects/08/ccc3d91a14d337a45f355d3d63bd97fd5e4db9").exists());
> assertTrue("tree missing", new File(gitDir,
> "objects/9d/aeec817090098f05eeca858e3a552d78b0a346").exists());
> +
> assertTrue("commit missing", new File(gitDir,
> - "objects/09/6f1a84091b90b6d9fb12f95848da69496305c1").exists());
> + "objects/45/df73fd9abbc2c61620c036948b1157e4d21253").exists());Debugging this test shows that the commit we created in the test is actually:
-- $ git cat-file commit 4c1bc1435f93c9409c93db5239e111271a8ccf55 tree 9daeec817090098f05eeca858e3a552d78b0a346 author J. Git <j.git@egit.org> 60876086400 +0100 committer J. Git <j.git@egit.org> 60876086400 +0100
testNewUnsharedFile
Junit tests --
Which has me starting to wonder, what the heck is different between systems that is being reflected in this commit object? The only thing I can think of is the timestamp we are creating by the deprecated Date constructor call back on line 82. Perhaps on different JVMs it is using different values for the hh:mm:ss parts of the timestamp value?
-- Shawn.
Robin RosenbergRe: [EGIT PATCH 1/2] Revert "Fix commit id in egit test T0001_ConnectProviderOperationTest"
onsdag 17 december 2008 17:09:33 skrev Shawn O. Pearce:
> The only thing I can think of is the timestamp we are creating by > the deprecated Date constructor call back on line 82. Perhaps > on different JVMs it is using different values for the hh:mm:ss > parts of the timestamp value?
Indeed it is. Date is dependent on local time zone.
>From e4d10dea5d62210868d71384e559a0d3ef1ca55d Mon Sep 17 00:00:00 2001
From: Robin Rosenberg <robin.rosenberg@dewire.com> Date: Wed, 17 Dec 2008 22:46:48 +0100 Subject: [EGIT PATCH 1/2 v2] Fix testcase that was sensitive to the local time zone.
Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com> --- .../op/T0001_ConnectProviderOperationTest.java | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/org.spearce.egit.core.test/src/org/spearce/egit/core/op/T0001_ConnectProviderOperationTest.java b/org.spearce.egit.core.test/src/org/spearce/egit/core/op/T0001_ConnectProviderOperationTest.java index 0ce2d7f..092c048 100644 --- a/org.spearce.egit.core.test/src/org/spearce/egit/core/op/T0001_ConnectProviderOperationTest.java +++ b/org.spearce.egit.core.test/src/org/spearce/egit/core/op/T0001_ConnectProviderOperationTest.java @@ -80,7 +80,7 @@ public void testNewUnsharedFile() throws CoreException, IOException, Commit commit = new Commit(thisGit); commit.setTree(rootTree); commit.setAuthor(new PersonIdent("J. Git", "j.git@egit.org", new Date( - 1999, 1, 1), TimeZone.getTimeZone("GMT+1"))); + 60876075600000L), TimeZone.getTimeZone("GMT+1"))); commit.setCommitter(commit.getAuthor()); commit.setMessage("testNewUnsharedFile\n\nJunit tests\n"); ObjectId id = writer.writeCommit(commit);
-- 1.6.0.3.640.g6331a
Shawn O. PearceRe: [EGIT PATCH 1/2] Revert "Fix commit id in egit test T0001_ConnectProviderOperationTest"
Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> onsdag 17 december 2008 17:09:33 skrev Shawn O. Pearce: > > The only thing I can think of is the timestamp we are creating by > > the deprecated Date constructor call back on line 82. Perhaps > > on different JVMs it is using different values for the hh:mm:ss > > parts of the timestamp value? > > Indeed it is. Date is dependent on local time zone.
Thanks, much better.
-- Shawn.
Robin Rosenberg[EGIT PATCH] Committer, author and tagger time should not be parsed as 32 bit signed int
If not dates past 2038 will be parsed the wrong way when parsed into a RevCommit or RevTag object.
Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com> --- .../src/org/spearce/jgit/util/RawParseUtils.java | 58 +++++++++++++++++++- 1 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/util/RawParseUtils.java b/org.spearce.jgit/src/org/spearce/jgit/util/RawParseUtils.java index 55a3001..74fe506 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/util/RawParseUtils.java +++ b/org.spearce.jgit/src/org/spearce/jgit/util/RawParseUtils.java @@ -135,7 +135,7 @@ public static int formatBase10(final byte[] b, int o, int value) { } /** - * Parse a base 10 numeric from a sequence of ASCII digits. + * Parse a base 10 numeric from a sequence of ASCII digits into an int. * <p> * Digit sequences can begin with an optional run of spaces before the * sequence, and may start with a '+' or a '-' to indicate sign position. @@ -189,6 +189,60 @@ public static final int parseBase10(final byte[] b, int ptr, } /** + * Parse a base 10 numeric from a sequence of ASCII digits into a long. + * <p> + * Digit sequences can begin with an optional run of spaces before the + * sequence, and may start with a '+' or a '-' to indicate sign position. + * Any other characters will cause the method to stop and return the current + * result to the caller. + * + * @param b + * buffer to scan. + * @param ptr + * position within buffer to start parsing digits at. + * @param ptrResult + * optional location to return the new ptr value through. If null + * the ptr value will be discarded. + * @return the value at this location; 0 if the location is not a valid + * numeric. + */ + public static final long parseLongBase10(final byte[] b, int ptr, + final MutableInteger ptrResult) { + long r = 0; + int sign = 0; + try { + final int sz = b.length; + while (ptr < sz && b[ptr] == ' ') + ptr++; + if (ptr >= sz) + return 0; + + switch (b[ptr]) { + case '-': + sign = -1; + ptr++; + break; + case '+': + ptr++; + break; + } + + while (ptr < sz) { + final byte v = digits[b[ptr]]; + if (v < 0) + break; + r = (r * 10) + v; + ptr++; + } + } catch (ArrayIndexOutOfBoundsException e) { + // Not a valid digit. + } + if (ptrResult != null) + ptrResult.value = ptr; + return sign < 0 ? -r : r; + } + + /** * Parse a Git style timezone string. * <p> * The sequence "-0315" will be parsed as the numeric value -195, as the @@ -414,7 +468,7 @@ public static PersonIdent parsePersonIdent(final byte[] raw, final int nameB) { final String email = decode(cs, raw, emailB, emailE - 1); final MutableInteger ptrout = new MutableInteger(); - final int when = parseBase10(raw, emailE + 1, ptrout); + final long when = parseLongBase10(raw, emailE + 1, ptrout); final int tz = parseTimeZoneOffset(raw, ptrout.value); return new PersonIdent(name, email, when * 1000L, tz);
-- 1.6.0.3.640.g6331a
Shawn O. PearceRe: [EGIT PATCH] Committer, author and tagger time should not be parsed as 32 bit signed int
Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> If not dates past 2038 will be parsed the wrong way when > parsed into a RevCommit or RevTag object.
Uhm, sure.
But there's also the commitTime field in RevCommit, its used to sort commits during walking. In 2038 that will also overflow.
Also, if you search the code for '2038' you'll find a remark about the year 2038 in DirCacheEntry.smudgeRacilyClean. Last I looked at the git sources I think this field in the index is treated as a signed time_t so we can't set the high bits and extend it out another 60+ years.
Honestly, I'm not sure this patch is worth the code duplication without fixing our other two known 2038 problem spots... and I really don't want to make RevCommit.commitTime into a long, as that will bloat out the object allocations and slow down the comparsion on 32 bit JVMs. Right now at the end of 2008 the memory isn't as readily available and there's still a lot of 32 bit JVMs. Another 10 years we'll probably be looking at 256 bit wide registers being very common, and 1 PB of core memory, and extending these fields out to a long will be trivial. And we'll still have 20 years to make the transition.
I'd rather just tag the fields with "2038" so we can search for them in the future. Like say this:
--8<-- Mark the other two locations that break in the year 2038
When the 32 bit timestamp rolls over in 2038 these spots in JGit will break, unless we upgrade them to use a 64 bit long before then. For now its too time and memory intensive to use a full long here, but in another 10 years we should have enough computing power that this is a moot point and we can upgrade the code paths marked with by 'git grep 2038'.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- .../src/org/spearce/jgit/revwalk/RevCommit.java | 2 ++ .../src/org/spearce/jgit/util/RawParseUtils.java | 2 ++ 2 files changed, 4 insertions(+), 0 deletions(-)
org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java, org.spearce.jgit/src/org/spearce/jgit/util/RawParseUtils.java
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java index 9d30018..bcfd8c4 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java +++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java @@ -129,6 +129,8 @@ else if (nParents == 1) { ptr = RawParseUtils.committer(raw, ptr); if (ptr > 0) { ptr = RawParseUtils.nextLF(raw, ptr, '>'); + + // In 2038 commitTime will overflow unless it is changed to long. commitTime = RawParseUtils.parseBase10(raw, ptr, null); } diff --git a/org.spearce.jgit/src/org/spearce/jgit/util/RawParseUtils.java b/org.spearce.jgit/src/org/spearce/jgit/util/RawParseUtils.java index 55a3001..c2d591b 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/util/RawParseUtils.java +++ b/org.spearce.jgit/src/org/spearce/jgit/util/RawParseUtils.java @@ -414,6 +414,8 @@ public static PersonIdent parsePersonIdent(final byte[] raw, final int nameB) { final String email = decode(cs, raw, emailB, emailE - 1); final MutableInteger ptrout = new MutableInteger(); + + // In 2038 "when" will overflow. Switch to a long before then. final int when = parseBase10(raw, emailE + 1, ptrout); final int tz = parseTimeZoneOffset(raw, ptrout.value);
-- 1.6.1.rc3.302.gb14d9 -- Shawn.
Robin RosenbergRe: [EGIT PATCH] Committer, author and tagger time should not be parsed as 32 bit signed int
onsdag 17 december 2008 23:48:44 skrev Shawn O. Pearce:
> Robin Rosenberg <robin.rosenberg@dewire.com> wrote: > > If not dates past 2038 will be parsed the wrong way when > > parsed into a RevCommit or RevTag object. > > Uhm, sure. > > But there's also the commitTime field in RevCommit, its used to > sort commits during walking. In 2038 that will also overflow.
Ok, but that's not used for display purposes, which what I saw. How huge is the cost of a long here. Most processor handle longs well today, though many megs of them will hurt the cache.
> Also, if you search the code for '2038' you'll find a remark > about the year 2038 in DirCacheEntry.smudgeRacilyClean. Last > I looked at the git sources I think this field in the index is > treated as a signed time_t so we can't set the high bits and > extend it out another 60+ years.
That is another time stamp. It is a file time stamp, which we never store in the Git object database. The index can be revised over time since it is purely local structure. If it weren't for the need to operate with C Git in the same repo we could have used a different format, and nobody would notice. (The first jgit versions used a different index structure).
time_t is signed, but it is also 64 bit on 64-bit linux so it doesn't overflow in 2038.
$ cev 'printf("sizeof time =%d\n",sizeof(time_t));' sizeof time =8
> I'd rather just tag the fields with "2038" so we can search for > them in the future. Like say this:
There should be a comment on the commitTime field too if we choose that option.
-- robin