[EGIT PATCH 0/2] *** SUBJECT HERE ***

The 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(-)

This reverts commit 61133091d5f22398828b350ff772165e9945db8a.

Bisect says this is the commit that failed, which is odd. Bad QA.

Signed-off-by: Robin Rosenberg <redacted>
---
 .../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

The index was dropped from the history with Shawns revision walker.

Signed-off-by: Robin Rosenberg <redacted>
---
 .../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

Robin Rosenberg [off-list ref] wrote:

This reverts commit 61133091d5f22398828b350ff772165e9945db8a.

Bisect says this is the commit that failed, which is odd. Bad QA.

Hmmph.  This revert fails here.

Debugging this test shows that the commit we created in the test
is actually:

--
$ git cat-file commit 4c1bc1435f93c9409c93db5239e111271a8ccf55
tree 9daeec817090098f05eeca858e3a552d78b0a346
author J. Git [off-list ref] 60876086400 +0100
committer J. Git [off-list ref] 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.

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 <redacted>
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 <redacted>
---
 .../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

Robin Rosenberg [off-list ref] wrote:

onsdag 17 december 2008 17:09:33 skrev Shawn O. Pearce:

quoted

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.

If not dates past 2038 will be parsed the wrong way when
parsed into a RevCommit or RevTag object.

Signed-off-by: Robin Rosenberg <redacted>
---
 .../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

Robin Rosenberg [off-list ref] 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 <redacted>
---
 .../src/org/spearce/jgit/revwalk/RevCommit.java    |    2 ++
 .../src/org/spearce/jgit/util/RawParseUtils.java   |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

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.

onsdag 17 december 2008 23:48:44 skrev Shawn O. Pearce:

Robin Rosenberg [off-list ref] wrote:

quoted

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