[EGIT PATCH] Convert author and comment on demand.

[EGIT PATCH] Convert author and comment on demand.

[Robin Rosenberg]

This sppeds up reading commits a lot by only store the byte
array data when reading commits. For the eclipse plugin I only
need the tree to filter out which commits to display and I can
take the cost of converting the comments to string for the
very few commits to display. Only the displayed commits are actually
converted so this results in convertig author and comment information
for about five commits rather than 20,000 (in my repo).

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../src/org/spearce/jgit/lib/Commit.java           |   73 +++++++++++++------
 1 files changed, 50 insertions(+), 23 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Commit.java 
b/org.spearce.jgit/src/org/spearce/jgit/lib/Commit.java
index 4e03a5a..14fa602 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/Commit.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Commit.java
@@ -16,10 +16,16 @@
  */
 package org.spearce.jgit.lib;
 
+import java.io.BufferedReader;
+import java.io.ByteArrayInputStream;
 import java.io.IOException;
+import java.io.InputStreamReader;
+import java.nio.charset.Charset;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 
+import org.spearce.jgit.errors.CorruptObjectException;
 import org.spearce.jgit.errors.MissingObjectException;
 
 public class Commit implements Treeish {
@@ -39,6 +45,8 @@ public class Commit implements Treeish {
 
     private Tree treeObj;
 
+    private byte[] raw;
+
     public Commit(final Repository db) {
 	objdb = db;
 	parentIds = new ArrayList(2);
@@ -58,29 +66,7 @@ public class Commit implements Treeish {
 	    rawPtr += 48;
 	}
 
-	//
-	// if (n == null || !n.startsWith("author ")) {
-	// throw new CorruptObjectException(commitId, "no author");
-	// }
-	// author = new PersonIdent(n.substring("author ".length()));
-	//
-	// n = br.readLine();
-	// if (n == null || !n.startsWith("committer ")) {
-	// throw new CorruptObjectException(commitId, "no committer");
-	// }
-	// committer = new PersonIdent(n.substring("committer ".length()));
-	//
-	// n = br.readLine();
-	// if (n == null || !n.equals("")) {
-	// throw new CorruptObjectException(commitId, "malformed header");
-	// }
-	//
-	// tempMessage = new StringBuffer();
-	// readBuf = new char[128];
-	// while ((readLen = br.read(readBuf)) > 0) {
-	// tempMessage.append(readBuf, 0, readLen);
-	// }
-	// message = tempMessage.toString();
+	this.raw = raw;
     }
 
     public ObjectId getCommitId() {
@@ -119,6 +105,7 @@ public class Commit implements Treeish {
     }
 
     public PersonIdent getAuthor() {
+	decode();
 	return author;
     }
 
@@ -127,6 +114,7 @@ public class Commit implements Treeish {
     }
 
     public PersonIdent getCommitter() {
+	decode();
 	return committer;
     }
 
@@ -139,9 +127,48 @@ public class Commit implements Treeish {
     }
 
     public String getMessage() {
+	decode();
 	return message;
     }
 
+    private void decode() {
+	if (raw!=null) {
+	    try {
+        	BufferedReader br=new BufferedReader(new InputStreamReader(new 
ByteArrayInputStream(raw)));
+        	String n=br.readLine();
+                if (n == null || !n.startsWith("tree ")) {
+                    throw new CorruptObjectException(commitId, "no tree");
+                }
+                while ((n = br.readLine())!=null && n.startsWith("parent "))
+            	;
+                if (n == null || !n.startsWith("author ")) {
+                    throw new CorruptObjectException(commitId, "no author");
+                }
+                author = new PersonIdent(n.substring("author ".length()));
+                n = br.readLine();
+                if (n == null || !n.startsWith("committer ")) {
+                    throw new CorruptObjectException(commitId, "no 
committer");
+                }
+                committer = new 
PersonIdent(n.substring("committer ".length()));
+                n = br.readLine();
+                if (n == null || !n.equals("")) {
+                    throw new CorruptObjectException(commitId, "malformed 
header");
+                }
+                StringBuffer tempMessage = new StringBuffer();
+                char[] readBuf = new char[2048];
+                int readLen;
+		while ((readLen = br.read(readBuf)) > 0) {
+                    tempMessage.append(readBuf, 0, readLen);
+                }
+                message = tempMessage.toString();
+	    } catch (IOException e) {
+		e.printStackTrace();
+	    } finally {
+		raw = null;
+	    }
+	}
+    }
+
     public void setMessage(final String m) {
 	message = m;
     }
-- 
1.4.4.gf05d

Re: [EGIT PATCH] Convert author and comment on demand.

[Shawn Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> This sppeds up reading commits a lot by only store the byte
> array data when reading commits.

Thanks.  I was working on something similar but did not have
a chance to finish it.  I've applied your patch instead.

> +	    try {
> +        	BufferedReader br=new BufferedReader(new InputStreamReader(new 
> ByteArrayInputStream(raw)));
> +        	String n=br.readLine();

Something's wrong with your mail client... the patch was mangled.

-- 

Re: [EGIT PATCH] Convert author and comment on demand.

[Robin Rosenberg]

söndag 03 december 2006 03:16 skrev Shawn Pearce:
> Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> > This sppeds up reading commits a lot by only store the byte
> > array data when reading commits.
>
> Thanks.  I was working on something similar but did not have
> a chance to finish it.  I've applied your patch instead.
>
> > +	    try {
> > +        	BufferedReader br=new BufferedReader(new InputStreamReader(new
> > ByteArrayInputStream(raw)));
> > +        	String n=br.readLine();
>
> Something's wrong with your mail client... the patch was mangled.

Not really, only the user. KMail by default wraps lines and I didn't turn that 
off before sending. Usually I send patches just using stg mail, but this time 
I used git-format-patch and kmail. Generatig attachements may be better (next 
time).

Re: [EGIT PATCH] Convert author and comment on demand.

[Jakub Narebski]

Robin Rosenberg wrote:

> söndag 03 december 2006 03:16 skrev Shawn Pearce:
>>
>> Something's wrong with your mail client... the patch was mangled.
> 
> Not really, only the user. KMail by default wraps lines and I didn't turn that 
> off before sending. Usually I send patches just using stg mail, but this time 
> I used git-format-patch and kmail. Generatig attachements may be better (next 
> time).

It is easier to comment on patch if it is embedded in the mail, and not
in attachement. Just remember to turn off word wrapping before sending
the patch.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [EGIT PATCH] Convert author and comment on demand.

[Linus Torvalds]

On Sun, 3 Dec 2006, Jakub Narebski wrote:
> 
> It is easier to comment on patch if it is embedded in the mail, and not
> in attachement.

This is why I _hate_ patches in attachments.

Sure, I can make my mail reader show the attachments. But when I "reply", 
I want the patch to be indented with the regular "> " thing and visible in 
the reply, so that I can say "I like the patch in general, but <this> part 
of it is seriously broken".

And yes, the personal mailreader I have has a "include attachements in 
reply" mode. But I don't generally want to include attachments in any 
reply, I just want it for _patches_.

[ Side ntoe: besides, many people send broken attachments that aren't 
  marked as text, but as 8-bit binary data or something, even if it's a 
  text-patch - so "attachement problems" are almost as common as the 
  non-attachement "line wrap" or whitespace problems are - people who 
  think that attachments automatically means that the thing is correct are 
  just deluding themselves.

  The fact is, you can get attachments wrong exactly the same way you get 
  linewrapping wrong. It's just that the pure binary data is likely to 
  always make it through correctly with an attachment, but if it gets 
  marked as a binary MIME-type, that doesn't much help, because while the 
  data is there, it's still going to act the wrong way for any 
  _discussion_ about it. ]

In other words: there are lots of things that make sense as true 
attachments. It's just that "patch" is not one of them.

Patches, unlike for example full files or tar-balls, by their very design 
are (a) text and (b) something where the whole _point_ is discussion and 
quoting about them. If we didn't want to discuss the patch contents, we 
wouldn't send them as patches in the first place, they'd be git-to-git 
transfers.

So this all boils down to one thing:

	PATCHES ARE NOT SEPARATE FILES TO BE ATTACHED TO AN EMAIL.

Patches are to be considered part of the discussion, not separate. And 
thus they should be in the main body of the email, exactly so that people 
(regardless of mailer settings and details like that) will automatically 
quote them, and they get passed around as integral to the discussion as 
the discussion itself.

I just jumped in because this is a pet peeve of mine. Some people seem to 
think that patches are "binary files" just because they have whitespace 
requirements and line-wrapping matters. But whitespace and line wrapping 
is important even in regular discussion, and patches really _are_ about 
the _human_ communication, just as much - and perhaps more so - as they 
are about feeding to the "patch" program.