[JGIT] Request for help

[JGIT] Request for help

[Nasser Grainawi]

Hello all,

I'm looking to add 'git patch-id' to JGit and I could use a few 
pointers. I'm not very familiar with the JGit code base or Java, so 
please excuse any blatant oversights or unintelligent questions.

First off, is there a "hacking JGit" document anywhere? One of those 
would be great right now.

So far I'm just trying to define the inputs and outputs. On Shawn's 
suggestion I'm planning on making it part of the org.spearce.jgit.patch 
package. C Git patch-id very generically has an input of a 'patch', so 
I'm thinking this implementation should use the Patch object. Looking at 
that class it seems that has everything patch-id should need, so perhaps 
that's the only input.

As far as output, C Git patch-id has the special feature to output the 
commit-id along with the patch-id when it gets input in the format of 
git-diff-tree. Should JGit do the same or just return the patch-id? I 
don't know that this question even makes sense in the context of JGit 
(since the commit-id is almost certainly available elsewhere and someone 
calling 'getPatchId()' is likely only interested in the patch-id).

Should PatchId be a class on its own, or just a method within the Patch 
class?

Thanks,
Nasser

Re: [JGIT] Request for help

[Johannes Schindelin]

Hi,

On Wed, 2 Sep 2009, Nasser Grainawi wrote:

> I'm looking to add 'git patch-id' to JGit and I could use a few 
> pointers. I'm not very familiar with the JGit code base or Java, so 
> please excuse any blatant oversights or unintelligent questions.
> 
> First off, is there a "hacking JGit" document anywhere? One of those 
> would be great right now.

There have been some mails with details about JGit from Shawn (IIRC) to 
this very list.

> So far I'm just trying to define the inputs and outputs. On Shawn's 
> suggestion I'm planning on making it part of the org.spearce.jgit.patch 
> package. C Git patch-id very generically has an input of a 'patch', so 
> I'm thinking this implementation should use the Patch object.

C Git patch-id takes a valid patch as input; I do not think that you want 
to use the Patch object.

FWIW a patch-id is nothing else than the SHA-1 of a diff where the "diff", 
"index", "@@" lines and all the whitespace was removed.

This is not really difficult in Java, however, it relies on a working diff 
implementation (and IIRC my implementation has not yet been integrated 
into JGit).

Ciao,
Dscho

Re: [JGIT] Request for help

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Wed, 2 Sep 2009, Nasser Grainawi wrote:
> 
> > I'm looking to add 'git patch-id' to JGit and I could use a few 
> > pointers. I'm not very familiar with the JGit code base or Java, so 
> > please excuse any blatant oversights or unintelligent questions.
> > 
> > First off, is there a "hacking JGit" document anywhere? One of those 
> > would be great right now.
> 
> There have been some mails with details about JGit from Shawn (IIRC) to 
> this very list.

Yea, for the most part I think we use Eclipse, and you just have
to import JGit's top level directory into Eclipse as it comes with
Eclipse project files.  But I know some folks only use our Maven
build (under jgit-maven/jgit) or use NetBeans.  I have no idea how
to import the project into the latter or configure its unit tests
to run.
 
> > So far I'm just trying to define the inputs and outputs. On Shawn's 
> > suggestion I'm planning on making it part of the org.spearce.jgit.patch 
> > package. C Git patch-id very generically has an input of a 'patch', so 
> > I'm thinking this implementation should use the Patch object.
> 
> C Git patch-id takes a valid patch as input; I do not think that you want 
> to use the Patch object.

I think we do want to use the Patch object.  The Patch entity in
JGit is a parsed representation of the git diff or unified diff
structure.  Its relatively easy to walk over, and all of the mess
about determining line type has already been done.

We'd probably want to do something that is a lot like the object
Patch as the output of our diff routine.  A tool (e.g. Gerrit Code
Review) might only want the EditList for a given file, and not
really care about the actual formatted patch text, as it reformats
everything itself.  I think patch-id computation is along those
same lines.

If we were to compute a patch-id off an InputStream we would probably
just send it through the Patch object first.

> This is not really difficult in Java, however, it relies on a working diff 
> implementation (and IIRC my implementation has not yet been integrated 
> into JGit).

Speaking of... where does that stand?

-- 
Shawn.

jgit diff, was Re: [JGIT] Request for help

[Johannes Schindelin]

Hi,

On Wed, 2 Sep 2009, Shawn O. Pearce wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > On Wed, 2 Sep 2009, Nasser Grainawi wrote:
> > 
> > > I'm looking to add 'git patch-id' to JGit and I could use a few 
> > > pointers. I'm not very familiar with the JGit code base or Java, so 
> > > please excuse any blatant oversights or unintelligent questions.
> > > 
> > > First off, is there a "hacking JGit" document anywhere? One of those 
> > > would be great right now.
> > 
> > There have been some mails with details about JGit from Shawn (IIRC) 
> > to this very list.
> 
> Yea, for the most part I think we use Eclipse, and you just have to 
> import JGit's top level directory into Eclipse as it comes with Eclipse 
> project files.  But I know some folks only use our Maven build (under 
> jgit-maven/jgit) or use NetBeans.  I have no idea how to import the 
> project into the latter or configure its unit tests to run.

FWIW I use vim & shell most of the time (yes, even for JGit).

> > This is not really difficult in Java, however, it relies on a working 
> > diff implementation (and IIRC my implementation has not yet been 
> > integrated into JGit).
> 
> Speaking of... where does that stand?

Same as where I left off.  IOW it is a working implementation that saw 
some testing, but I simply lack the time for performance tuning.

It should not be all that bad, though.

***goes looking at 
http://repo.or.cz/w/jgit/dscho.git?a=shortlog;h=refs/heads/diff
***

Seems I misremembered a bit.  Christian provided a patch to make it 
compileable, but I think that I ran the script to verify that the diffs 
are correct on jgit.git and IIRC it completed fine.

There is a project in my day-job, however, which eats all my time at the 
moment (it is actually wrapping up a "succeeded" GSoC project where the 
student -- *sigh* -- has gone away).  So all I can do is to rebase to 
current jgit.git's 's master, to run the script, and submit the current 
patch series (valuing correctness over speed).

Ciao,
Dscho

[JGIT PATCH 0/5] jgit diff

[Johannes Schindelin]

This patch series provides a rudimentary, working implementation of "jgit 
diff".  It does not provide all modes of "git diff" -- by far! -- but it 
is robust, and should provide a good starting point for further work.

Unfortunately, I lack the time to do proper profiling/benchmarking, but I 
verified at least that it succeeds in recreating valid patches for all 
commits in jgit.git with this script:

	git rev-list HEAD |
	sed '$d' |
	while read commit
	do
	        printf "\\r$commit "
	        (export GIT_INDEX_FILE=test-index &&
	         ./jgit diff $commit^ $commit > test-patch &&
	         git read-tree $commit^ &&
	         git apply --cached test-patch &&
	         git diff --exit-code --cached $commit) || break
	done

Johannes Schindelin (5):
  Add set to IntList
  Add Myers' algorithm to generate diff scripts
  Add a test class for Myers' diff algorithm
  Prepare RawText for diff-index and diff-files
  Add the "jgit diff" command

 .../services/org.spearce.jgit.pgm.TextBuiltin      |    1 +
 .../src/org/spearce/jgit/pgm/Diff.java             |  133 +++++
 .../tst/org/spearce/jgit/diff/MyersDiffTest.java   |  103 ++++
 .../tst/org/spearce/jgit/util/IntListTest.java     |   21 +
 .../src/org/spearce/jgit/diff/DiffFormatter.java   |    2 +-
 .../src/org/spearce/jgit/diff/MyersDiff.java       |  515 ++++++++++++++++++++
 .../src/org/spearce/jgit/diff/RawText.java         |   28 +-
 .../src/org/spearce/jgit/util/IntList.java         |   17 +
 8 files changed, 818 insertions(+), 2 deletions(-)
 create mode 100644 org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Diff.java
 create mode 100644 org.spearce.jgit.test/tst/org/spearce/jgit/diff/MyersDiffTest.java
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/diff/MyersDiff.java

[JGIT PATCH 1/5] Add set to IntList

[Johannes Schindelin]

Some applications may wish to modify an int list.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .../tst/org/spearce/jgit/util/IntListTest.java     |   21 ++++++++++++++++++++
 .../src/org/spearce/jgit/util/IntList.java         |   17 ++++++++++++++++
 2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/util/IntListTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/util/IntListTest.java
index c470d55..a7a12cd 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/util/IntListTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/util/IntListTest.java
@@ -144,6 +144,27 @@ public void testClear() {
 		}
 	}
 
+	public void testSet() {
+		final IntList i = new IntList();
+		i.add(1);
+		assertEquals(1, i.size());
+		assertEquals(1, i.get(0));
+
+		i.set(0, 5);
+		assertEquals(5, i.get(0));
+
+		try {
+			i.set(5, 5);
+			fail("accepted set of 5 beyond end of list");
+		} catch (ArrayIndexOutOfBoundsException e){
+			assertTrue(true);
+		}
+
+		i.set(1, 2);
+		assertEquals(2, i.size());
+		assertEquals(2, i.get(1));
+	}
+
 	public void testToString() {
 		final IntList i = new IntList();
 		i.add(1);
diff --git a/org.spearce.jgit/src/org/spearce/jgit/util/IntList.java b/org.spearce.jgit/src/org/spearce/jgit/util/IntList.java
index 0a84793..32d24fc 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/util/IntList.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/util/IntList.java
@@ -94,6 +94,23 @@ public void add(final int n) {
 	}
 
 	/**
+	 * Assign an entry in the list.
+	 *
+	 * @param index
+	 *            index to set, must be in the range [0, {@link #size()}).
+	 * @param n
+	 *            value to store at the position.
+	 */
+	public void set(final int index, final int n) {
+		if (count < index)
+			throw new ArrayIndexOutOfBoundsException(index);
+		else if (count == index)
+			add(n);
+		else
+			entries[index] = n;
+	}
+
+	/**
 	 * Pad the list with entries.
 	 *
 	 * @param toIndex
-- 
1.6.4.297.gcb4cc

[JGIT PATCH 2/5] Add Myers' algorithm to generate diff scripts

[Johannes Schindelin]

Myers' algorithm is the standard way to generate diff scripts in an
efficient manner (especially memory-wise).

The source contains extensive documentation about the principal
ideas of the algorithm.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .../src/org/spearce/jgit/diff/MyersDiff.java       |  515 ++++++++++++++++++++
 1 files changed, 515 insertions(+), 0 deletions(-)
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/diff/MyersDiff.java

diff --git a/org.spearce.jgit/src/org/spearce/jgit/diff/MyersDiff.java b/org.spearce.jgit/src/org/spearce/jgit/diff/MyersDiff.java
new file mode 100644
index 0000000..e19cea4
--- /dev/null
+++ b/org.spearce.jgit/src/org/spearce/jgit/diff/MyersDiff.java
@@ -0,0 +1,515 @@
+/*
+ * Copyright (C) 2008-2009 Johannes E. Schindelin <johannes.schindelin@gmx.de>
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Git Development Community nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+/**
+ * Diff algorithm, based on "An O(ND) Difference Algorithm and its
+ * Variations", by Eugene Myers.
+ *
+ * The basic idea is to put the line numbers of text A as columns ("x") and the
+ * lines of text B as rows ("y").  Now you try to find the shortest "edit path"
+ * from the upper left corner to the lower right corner, where you can
+ * always go horizontally or vertically, but diagonally from (x,y) to
+ * (x+1,y+1) only if line x in text A is identical to line y in text B.
+ *
+ * Myers' fundamental concept is the "furthest reaching D-path on diagonal k":
+ * a D-path is an edit path starting at the upper left corner and containing
+ * exactly D non-diagonal elements ("differences").  The furthest reaching
+ * D-path on diagonal k is the one that contains the most (diagonal) elements
+ * which ends on diagonal k (where k = y - x).
+ *
+ * Example:
+ *
+ *    H E L L O   W O R L D
+ *    ____
+ *  L     \___
+ *  O         \___
+ *  W             \________
+ *
+ * Since every D-path has exactly D horizontal or vertical elements, it can
+ * only end on the diagonals -D, -D+2, ..., D-2, D.
+ *
+ * Since every furthest reaching D-path contains at least one furthest
+ * reaching (D-1)-path (except for D=0), we can construct them recursively.
+ *
+ * Since we are really interested in the shortest edit path, we can start
+ * looking for a 0-path, then a 1-path, and so on, until we find a path that
+ * ends in the lower right corner.
+ *
+ * To save space, we do not need to store all paths (which has quadratic space
+ * requirements), but generate the D-paths simultaneously from both sides.
+ * When the ends meet, we will have found "the middle" of the path.  From the
+ * end points of that diagonal part, we can generate the rest recursively.
+ *
+ * This only requires linear space.
+ *
+ * The overall (runtime) complexity is
+ *
+ *	O(N * D^2 + 2 * N/2 * (D/2)^2 + 4 * N/4 * (D/4)^2 + ...)
+ *	= O(N * D^2 * 5 / 4) = O(N * D^2),
+ *
+ * (With each step, we have to find the middle parts of twice as many regions
+ * as before, but the regions (as well as the D) are halved.)
+ *
+ * So the overall runtime complexity stays the same with linear space,
+ * albeit with a larger constant factor.
+ */
+
+package org.spearce.jgit.diff;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+
+import org.spearce.jgit.util.IntList;
+
+public class MyersDiff {
+	protected EditList edits;
+	protected Sequence a, b;
+
+	public MyersDiff(Sequence a, Sequence b) {
+		this.a = a;
+		this.b = b;
+		calculateEdits();
+	}
+
+	public EditList getEdits() {
+		return edits;
+	}
+
+	// TODO: use ThreadLocal for future multi-threaded operations
+	MiddleEdit middle = new MiddleEdit();
+
+	protected void calculateEdits() {
+		edits = new EditList();
+
+		middle.initialize(0, a.size(), 0, b.size());
+		if (middle.beginA >= middle.endA &&
+				middle.beginB >= middle.endB)
+			return;
+
+		calculateEdits(middle.beginA, middle.endA,
+				middle.beginB, middle.endB);
+	}
+
+	protected void calculateEdits(int beginA, int endA,
+			int beginB, int endB) {
+		Edit edit = middle.calculate(beginA, endA, beginB, endB);
+
+		if (beginA < edit.beginA || beginB < edit.beginB) {
+			int k = edit.beginB - edit.beginA;
+			int x = middle.backward.snake(k, edit.beginA);
+			calculateEdits(beginA, x, beginB, k + x);
+		}
+
+		if (edit.getType() != Edit.Type.EMPTY)
+			edits.add(edits.size(), edit);
+
+		// after middle
+		if (endA > edit.endA || endB > edit.endB) {
+			int k = edit.endB - edit.endA;
+			int x = middle.forward.snake(k, edit.endA);
+			calculateEdits(x, endA, k + x, endB);
+		}
+	}
+
+	/**
+	 * A class to help bisecting the sequences a and b to find minimal
+	 * edit paths.
+	 *
+	 * As the arrays are reused for space efficiency, you will need one
+	 * instance per thread.
+	 *
+	 * The entry function is the calculate() method.
+	 */
+	class MiddleEdit {
+		void initialize(int beginA, int endA, int beginB, int endB) {
+			this.beginA = beginA; this.endA = endA;
+			this.beginB = beginB; this.endB = endB;
+
+			// strip common parts on either end
+			int k = beginB - beginA;
+			this.beginA = forward.snake(k, beginA);
+			this.beginB = k + this.beginA;
+
+			k = endB - endA;
+			this.endA = backward.snake(k, endA);
+			this.endB = k + this.endA;
+		}
+
+		/*
+		 * This function calculates the "middle" Edit of the shortest
+		 * edit path between the given subsequences of a and b.
+		 *
+		 * Once a forward path and a backward path meet, we found the
+		 * middle part.  From the last snake end point on both of them,
+		 * we construct the Edit.
+		 *
+		 * It is assumed that there is at least one edit in the range.
+		 */
+		// TODO: measure speed impact when this is synchronized
+		Edit calculate(int beginA, int endA, int beginB, int endB) {
+			if (beginA == endA || beginB == endB)
+				return new Edit(beginA, endA, beginB, endB);
+			this.beginA = beginA; this.endA = endA;
+			this.beginB = beginB; this.endB = endB;
+
+			/*
+			 * Following the conventions in Myers' paper, "k" is
+			 * the difference between the index into "b" and the
+			 * index into "a".
+			 */
+			int minK = beginB - endA;
+			int maxK = endB - beginA;
+
+			forward.initialize(beginB - beginA, beginA, minK, maxK);
+			backward.initialize(endB - endA, endA, minK, maxK);
+
+			for (int d = 1; ; d++)
+				if (forward.calculate(d) ||
+						backward.calculate(d))
+					return edit;
+		}
+
+		/*
+		 * For each d, we need to hold the d-paths for the diagonals
+		 * k = -d, -d + 2, ..., d - 2, d.  These are stored in the
+		 * forward (and backward) array.
+		 *
+		 * As we allow subsequences, too, this needs some refinement:
+		 * the forward paths start on the diagonal forwardK =
+		 * beginB - beginA, and backward paths start on the diagonal
+		 * backwardK = endB - endA.
+		 *
+		 * So, we need to hold the forward d-paths for the diagonals
+		 * k = forwardK - d, forwardK - d + 2, ..., forwardK + d and
+		 * the analogue for the backward d-paths.  This means that
+		 * we can turn (k, d) into the forward array index using this
+		 * formula:
+		 *
+		 *	i = (d + k - forwardK) / 2
+		 *
+		 * There is a further complication: the edit paths should not
+		 * leave the specified subsequences, so k is bounded by
+		 * minK = beginB - endA and maxK = endB - beginA.  However,
+		 * (k - forwardK) _must_ be odd whenever d is odd, and it
+		 * _must_ be even when d is even.
+		 *
+		 * The values in the "forward" and "backward" arrays are
+		 * positions ("x") in the sequence a, to get the corresponding
+		 * positions ("y") in the sequence b, you have to calculate
+		 * the appropriate k and then y:
+		 *
+		 *	k = forwardK - d + i * 2
+		 *	y = k + x
+		 *
+		 * (substitute backwardK for forwardK if you want to get the
+		 * y position for an entry in the "backward" array.
+		 */
+		EditPaths forward = new ForwardEditPaths();
+		EditPaths backward = new BackwardEditPaths();
+
+		/* Some variables which are shared between methods */
+		protected int beginA, endA, beginB, endB;
+		protected Edit edit;
+
+		abstract class EditPaths {
+			private IntList x = new IntList();
+			private IntList snake = new IntList();
+			int beginK, endK, middleK;
+			int prevBeginK, prevEndK;
+			/* if we hit one end early, no need to look further */
+			int minK, maxK; // TODO: better explanation
+
+			final int getIndex(int d, int k) {
+// TODO: remove
+if (((d + k - middleK) % 2) == 1)
+	throw new RuntimeException("odd: " + d + " + " + k + " - " + middleK);
+				return (d + k - middleK) / 2;
+			}
+
+			final int getX(int d, int k) {
+// TODO: remove
+if (k < beginK || k > endK)
+	throw new RuntimeException("k " + k + " not in " + beginK + " - " + endK);
+				return x.get(getIndex(d, k));
+			}
+
+			final int getSnake(int d, int k) {
+// TODO: remove
+if (k < beginK || k > endK)
+	throw new RuntimeException("k " + k + " not in " + beginK + " - " + endK);
+				return snake.get(getIndex(d, k));
+			}
+
+			private int forceKIntoRange(int k) {
+				/* if k is odd, so must be the result */
+				if (k < minK)
+					return minK + ((k ^ minK) & 1);
+				else if (k > maxK)
+					return maxK - ((k ^ maxK) & 1);
+				return k;
+			}
+
+			void initialize(int k, int x, int minK, int maxK) {
+				this.minK = minK;
+				this.maxK = maxK;
+				beginK = endK = middleK = k;
+				this.x.clear();
+				this.x.add(x);
+				snake.clear();
+				snake.add(newSnake(k, x));
+			}
+
+			abstract int snake(int k, int x);
+			abstract int getLeft(int x);
+			abstract int getRight(int x);
+			abstract boolean isBetter(int left, int right);
+			abstract void adjustMinMaxK(final int k, final int x);
+			abstract boolean meets(int d, int k, int x, int snake);
+
+			final int newSnake(int k, int x) {
+				int y = k + x;
+				return x + (endA + 1) * y;
+			}
+
+			final int snake2x(int snake) {
+				return snake % (endA + 1);
+			}
+
+			final int snake2y(int snake) {
+				return snake / (endA + 1);
+			}
+
+			final boolean makeEdit(int snake1, int snake2) {
+				int x1 = snake2x(snake1), x2 = snake2x(snake2);
+				int y1 = snake2y(snake1), y2 = snake2y(snake2);
+				/*
+				 * Check for incompatible partial edit paths:
+				 * when there are ambiguities, we might have
+				 * hit incompatible (i.e. non-overlapping)
+				 * forward/backward paths.
+				 *
+				 * In that case, just pretend that we have
+				 * an empty edit at the end of one snake; this
+				 * will force a decision which path to take
+				 * in the next recursion step.
+				 */
+				if (x1 > x2 || y1 > y2) {
+					x1 = x2;
+					y1 = y2;
+				}
+				edit = new Edit(x1, x2, y1, y2);
+				return true;
+			}
+
+			boolean calculate(int d) {
+				prevBeginK = beginK;
+				prevEndK = endK;
+				beginK = forceKIntoRange(middleK - d);
+				endK = forceKIntoRange(middleK + d);
+				// TODO: handle i more efficiently
+				// TODO: walk snake(k, getX(d, k)) only once per (d, k)
+				// TODO: move end points out of the loop to avoid conditionals inside the loop
+				// go backwards so that we can avoid temp vars
+				for (int k = endK; k >= beginK; k -= 2) {
+					int left = -1, right = -1;
+					int leftSnake = -1, rightSnake = -1;
+					// TODO: refactor into its own function
+					if (k > prevBeginK) {
+						int i = getIndex(d - 1, k - 1);
+						left = x.get(i);
+						int end = snake(k - 1, left);
+						leftSnake = left != end ?
+							newSnake(k - 1, end) :
+							snake.get(i);
+						if (meets(d, k - 1, end, leftSnake))
+							return true;
+						left = getLeft(end);
+					}
+					if (k < prevEndK) {
+						int i = getIndex(d - 1, k + 1);
+						right = x.get(i);
+						int end = snake(k + 1, right);
+						rightSnake = right != end ?
+							newSnake(k + 1, end) :
+							snake.get(i);
+						if (meets(d, k + 1, end, rightSnake))
+							return true;
+						right = getRight(end);
+					}
+					int newX, newSnake;
+					if (k >= prevEndK ||
+							(k > prevBeginK &&
+							 isBetter(left, right))) {
+						newX = left;
+						newSnake = leftSnake;
+					}
+					else {
+						newX = right;
+						newSnake = rightSnake;
+					}
+					if (meets(d, k, newX, newSnake))
+						return true;
+					adjustMinMaxK(k, newX);
+					int i = getIndex(d, k);
+					x.set(i, newX);
+					snake.set(i, newSnake);
+				}
+				return false;
+			}
+		}
+
+		class ForwardEditPaths extends EditPaths {
+			final int snake(int k, int x) {
+				for (; x < endA && k + x < endB; x++)
+					if (!a.equals(x, b, k + x))
+						break;
+				return x;
+			}
+
+			final int getLeft(final int x) {
+				return x;
+			}
+
+			final int getRight(final int x) {
+				return x + 1;
+			}
+
+			final boolean isBetter(final int left, final int right) {
+				return left > right;
+			}
+
+			final void adjustMinMaxK(final int k, final int x) {
+				if (x >= endA || k + x >= endB) {
+					if (k > backward.middleK)
+						maxK = k;
+					else
+						minK = k;
+				}
+			}
+
+			final boolean meets(int d, int k, int x, int snake) {
+				if (k < backward.beginK || k > backward.endK)
+					return false;
+				// TODO: move out of loop
+				if (((d - 1 + k - backward.middleK) % 2) == 1)
+					return false;
+				if (x < backward.getX(d - 1, k))
+					return false;
+				makeEdit(snake, backward.getSnake(d - 1, k));
+				return true;
+			}
+		}
+
+		class BackwardEditPaths extends EditPaths {
+			final int snake(int k, int x) {
+				for (; x > beginA && k + x > beginB; x--)
+					if (!a.equals(x - 1, b, k + x - 1))
+						break;
+				return x;
+			}
+
+			final int getLeft(final int x) {
+				return x - 1;
+			}
+
+			final int getRight(final int x) {
+				return x;
+			}
+
+			final boolean isBetter(final int left, final int right) {
+				return left < right;
+			}
+
+			final void adjustMinMaxK(final int k, final int x) {
+				if (x <= beginA || k + x <= beginB) {
+					if (k > forward.middleK)
+						maxK = k;
+					else
+						minK = k;
+				}
+			}
+
+			final boolean meets(int d, int k, int x, int snake) {
+				if (k < forward.beginK || k > forward.endK)
+					return false;
+				// TODO: move out of loop
+				if (((d + k - forward.middleK) % 2) == 1)
+					return false;
+				if (x > forward.getX(d, k))
+					return false;
+				makeEdit(forward.getSnake(d, k), snake);
+				return true;
+			}
+		}
+	}
+
+	// debugging (TODO: remove)
+	public void print(Sequence s, int begin, int end) {
+		RawText raw = (RawText)s;
+		try {
+			while (begin < end) {
+				System.err.print("" + begin + ": ");
+				raw.writeLine(System.err, begin++);
+				System.err.println("");
+			}
+		} catch (Exception e) { e.printStackTrace(); }
+	}
+
+	public void print(int beginA, int endA, int beginB, int endB) {
+		System.err.println("<<<<<<");
+		print(a, beginA, endA);
+		System.err.println("======");
+		print(b, beginB, endB);
+		System.err.println(">>>>>>");
+	}
+
+	public static void main(String[] args) {
+		if (args.length != 2) {
+			System.err.println("Need 2 arguments");
+			System.exit(1);
+		}
+		try {
+			RawText a = new RawText(new java.io.File(args[0]));
+			RawText b = new RawText(new java.io.File(args[1]));
+			MyersDiff diff = new MyersDiff(a, b);
+			System.out.println(diff.getEdits().toString());
+		} catch (Exception e) {
+			e.printStackTrace();
+		}
+	}
+}
-- 
1.6.4.297.gcb4cc

[JGIT PATCH 3/5] Add a test class for Myers' diff algorithm

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .../tst/org/spearce/jgit/diff/MyersDiffTest.java   |  103 ++++++++++++++++++++
 1 files changed, 103 insertions(+), 0 deletions(-)
 create mode 100644 org.spearce.jgit.test/tst/org/spearce/jgit/diff/MyersDiffTest.java

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/diff/MyersDiffTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/diff/MyersDiffTest.java
new file mode 100644
index 0000000..0d62790
--- /dev/null
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/diff/MyersDiffTest.java
@@ -0,0 +1,103 @@
+/*
+ * Copyright (C) 2009, Johannes E. Schindelin
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Git Development Community nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.spearce.jgit.diff;
+
+import junit.framework.TestCase;
+
+public class MyersDiffTest extends TestCase {
+	public void testAtEnd() {
+		assertDiff("HELLO", "HELL", " -4,1 +4,0");
+	}
+
+	public void testAtStart() {
+		assertDiff("Git", "JGit", " -0,0 +0,1");
+	}
+
+	public void testSimple() {
+		assertDiff("HELLO WORLD", "LOW",
+			" -0,3 +0,0 -5,1 +2,0 -7,4 +3,0");
+		// is ambiguous, could be this, too:
+		// " -0,2 +0,0 -3,1 +1,0 -5,1 +2,0 -7,4 +3,0"
+	}
+
+	public void assertDiff(String a, String b, String edits) {
+		MyersDiff diff = new MyersDiff(toCharArray(a), toCharArray(b));
+System.err.println("edits: " + diff.getEdits());
+		assertEquals(edits, toString(diff.getEdits()));
+	}
+
+	private static String toString(EditList list) {
+		StringBuilder builder = new StringBuilder();
+		for (Edit e : list)
+			builder.append(" -" + e.beginA
+					+ "," + (e.endA - e.beginA)
+				+ " +" + e.beginB + "," + (e.endB - e.beginB));
+		return builder.toString();
+	}
+
+	private static CharArray toCharArray(String s) {
+		return new CharArray(s);
+	}
+
+	protected static String toString(Sequence seq, int begin, int end) {
+		CharArray a = (CharArray)seq;
+		return new String(a.array, begin, end - begin);
+	}
+
+	protected static String toString(CharArray a, CharArray b,
+			int x, int k) {
+		return "(" + x + "," + (k + x)
+			+ (x < 0 ? '<' :
+					(x >= a.array.length ?
+					 '>' : a.array[x]))
+			+ (k + x < 0 ? '<' :
+					(k + x >= b.array.length ?
+					 '>' : b.array[k + x]))
+			+ ")";
+	}
+
+	private static class CharArray implements Sequence {
+		char[] array;
+		public CharArray(String s) { array = s.toCharArray(); }
+		public int size() { return array.length; }
+		public boolean equals(int i, Sequence other, int j) {
+			CharArray o = (CharArray)other;
+			return array[i] == o.array[j];
+		}
+	}
+}
-- 
1.6.4.297.gcb4cc

[JGIT PATCH 4/5] Prepare RawText for diff-index and diff-files

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .../src/org/spearce/jgit/diff/RawText.java         |   28 +++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/diff/RawText.java b/org.spearce.jgit/src/org/spearce/jgit/diff/RawText.java
index 9886d36..15d1c12 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/diff/RawText.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/diff/RawText.java
@@ -38,6 +38,8 @@
 
 package org.spearce.jgit.diff;
 
+import java.io.File;
+import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.OutputStream;
 
@@ -81,6 +83,18 @@ public RawText(final byte[] input) {
 		hashes = computeHashes();
 	}
 
+	/**
+	 * Create a new sequence from a file.
+	 * <p>
+	 * The entire file contents are used.
+	 *
+	 * @param file
+	 *            the text file.
+	 */
+	public RawText(File file) throws IOException {
+		this(readFile(file));
+	}
+
 	public int size() {
 		// The line map is always 2 entries larger than the number of lines in
 		// the file. Index 0 is padded out/unused. The last index is the total
@@ -181,4 +195,16 @@ protected int hashLine(final byte[] raw, int ptr, final int end) {
 			hash = (hash << 5) ^ (raw[ptr] & 0xff);
 		return hash;
 	}
-}
\ No newline at end of file
+
+	private static byte[] readFile(File file) throws IOException {
+		byte[] result = new byte[(int)file.length()];
+		FileInputStream in = new FileInputStream(file);
+		for (int off = 0; off < result.length; ) {
+			int read = in.read(result, off, result.length - off);
+			if (read < 0)
+				throw new IOException("Early EOF");
+			off += read;
+		}
+		return result;
+	}
+}
-- 
1.6.4.297.gcb4cc

[JGIT PATCH 5/5] Add the "jgit diff" command

[Johannes Schindelin]

This commit contains fixes provided by Christian Halstrick.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .../services/org.spearce.jgit.pgm.TextBuiltin      |    1 +
 .../src/org/spearce/jgit/pgm/Diff.java             |  133 ++++++++++++++++++++
 .../src/org/spearce/jgit/diff/DiffFormatter.java   |    2 +-
 3 files changed, 135 insertions(+), 1 deletions(-)
 create mode 100644 org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Diff.java

diff --git a/org.spearce.jgit.pgm/src/META-INF/services/org.spearce.jgit.pgm.TextBuiltin b/org.spearce.jgit.pgm/src/META-INF/services/org.spearce.jgit.pgm.TextBuiltin
index 3a8cc09..107ef38 100644
--- a/org.spearce.jgit.pgm/src/META-INF/services/org.spearce.jgit.pgm.TextBuiltin
+++ b/org.spearce.jgit.pgm/src/META-INF/services/org.spearce.jgit.pgm.TextBuiltin
@@ -1,6 +1,7 @@
 org.spearce.jgit.pgm.Branch
 org.spearce.jgit.pgm.Clone
 org.spearce.jgit.pgm.Daemon
+org.spearce.jgit.pgm.Diff
 org.spearce.jgit.pgm.DiffTree
 org.spearce.jgit.pgm.Fetch
 org.spearce.jgit.pgm.Glog
diff --git a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Diff.java b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Diff.java
new file mode 100644
index 0000000..56e352d
--- /dev/null
+++ b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Diff.java
@@ -0,0 +1,133 @@
+/*
+ * Copyright (C) 2009, Johannes E. Schindelin
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Git Development Community nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.spearce.jgit.pgm;
+
+import java.io.IOException;
+import java.io.PrintStream;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.kohsuke.args4j.Argument;
+import org.kohsuke.args4j.ExampleMode;
+import org.kohsuke.args4j.Option;
+
+import org.spearce.jgit.diff.DiffFormatter;
+import org.spearce.jgit.diff.MyersDiff;
+import org.spearce.jgit.diff.RawText;
+
+import org.spearce.jgit.lib.Constants;
+import org.spearce.jgit.lib.FileMode;
+import org.spearce.jgit.lib.ObjectId;
+import org.spearce.jgit.lib.Ref;
+
+import org.spearce.jgit.pgm.opt.CmdLineParser;
+import org.spearce.jgit.pgm.opt.PathTreeFilterHandler;
+
+import org.spearce.jgit.treewalk.AbstractTreeIterator;
+import org.spearce.jgit.treewalk.TreeWalk;
+
+import org.spearce.jgit.treewalk.filter.AndTreeFilter;
+import org.spearce.jgit.treewalk.filter.TreeFilter;
+
+@Command(common = true, usage = "Show diffs")
+class Diff extends TextBuiltin {
+	@Argument(index = 0, metaVar = "tree-ish", required = true)
+	void tree_0(final AbstractTreeIterator c) {
+		trees.add(c);
+	}
+
+	@Argument(index = 1, metaVar = "tree-ish", required = true)
+	private final List<AbstractTreeIterator> trees = new ArrayList<AbstractTreeIterator>();
+
+	@Option(name = "--", metaVar = "path", multiValued = true, handler = PathTreeFilterHandler.class)
+	private TreeFilter pathFilter = TreeFilter.ALL;
+
+	private DiffFormatter fmt = new DiffFormatter();
+
+	@Override
+	protected void run() throws Exception {
+		final TreeWalk walk = new TreeWalk(db);
+		walk.reset();
+		walk.setRecursive(true);
+		for (final AbstractTreeIterator i : trees)
+			walk.addTree(i);
+		walk.setFilter(AndTreeFilter.create(TreeFilter.ANY_DIFF, pathFilter));
+
+		final int nTree = walk.getTreeCount();
+		while (walk.next())
+			outputDiff(System.out, walk.getPathString(),
+				walk.getObjectId(0), walk.getFileMode(0),
+				walk.getObjectId(1), walk.getFileMode(1));
+	}
+
+	protected void outputDiff(PrintStream out, String path,
+			ObjectId id1, FileMode mode1, ObjectId id2, FileMode mode2) throws IOException {
+		String name1 = "a/" + path;
+		String name2 =  "b/" + path;
+		out.println("diff --git " + name1 + " " + name2);
+		boolean isNew=false;
+		boolean isDelete=false;
+		if (id1.equals(id1.zeroId())) {
+			out.println("new file mode " + mode2);
+			isNew=true;
+		} else if (id2.equals(id2.zeroId())) {
+			out.println("deleted file mode " + mode1);
+			isDelete=true;
+		} else if (!mode1.equals(mode2)) {
+			out.println("old mode " + mode1);
+			out.println("new mode " + mode2);
+		}
+		out.println("index " + id1.abbreviate(db, 7).name()
+			+ ".." + id2.abbreviate(db, 7).name()
+			+ (mode1.equals(mode2) ? " " + mode1 : ""));
+		out.println("--- " + (isNew ?  "/dev/null" : name1));
+		out.println("+++ " + (isDelete ?  "/dev/null" : name2));
+		RawText a = getRawText(id1);
+		RawText b = getRawText(id2);
+		MyersDiff diff = new MyersDiff(a, b);
+		fmt.formatEdits(out, a, b, diff.getEdits());
+	}
+
+	private RawText getRawText(ObjectId id) throws IOException {
+		if (id.equals(id.zeroId()))
+			return new RawText(new byte[] { });
+		return new RawText(db.openBlob(id).getCachedBytes());
+	}
+}
+
diff --git a/org.spearce.jgit/src/org/spearce/jgit/diff/DiffFormatter.java b/org.spearce.jgit/src/org/spearce/jgit/diff/DiffFormatter.java
index fa86737..fda3f4c 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/diff/DiffFormatter.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/diff/DiffFormatter.java
@@ -108,7 +108,7 @@ public void format(final OutputStream out, final FileHeader head,
 		formatEdits(out, a, b, head.toEditList());
 	}
 
-	private void formatEdits(final OutputStream out, final RawText a,
+	public void formatEdits(final OutputStream out, final RawText a,
 			final RawText b, final EditList edits) throws IOException {
 		for (int curIdx = 0; curIdx < edits.size();) {
 			Edit curEdit = edits.get(curIdx);
-- 
1.6.4.297.gcb4cc

Re: [JGIT PATCH 0/5] jgit diff

[Johannes Schindelin]

Hi,

On Thu, 3 Sep 2009, Johannes Schindelin wrote:

> This patch series provides a rudimentary, working implementation of 
> "jgit diff".  It does not provide all modes of "git diff" -- by far! -- 
> but it is robust, and should provide a good starting point for further 
> work.

I forgot to mention that this is rebased to current jgit.git's master (and 
tested there, too; took 713 seconds on this here machine).

Ciao,
Dscho

Re: [JGIT PATCH 0/5] jgit diff

[Shawn O. Pearce]

Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> This patch series provides a rudimentary, working implementation of "jgit 
> diff".  It does not provide all modes of "git diff" -- by far! -- but it 
> is robust, and should provide a good starting point for further work.

I have rebased this patch series on our new JGit history, and then
opened a bug at eclipse.org to get it through the IP review process:

  https://bugs.eclipse.org/bugs/show_bug.cgi?id=291083
 
-- 
Shawn.

Re: [JGIT PATCH 0/5] jgit diff

[Johannes Schindelin]

Hi,

On Thu, 1 Oct 2009, Shawn O. Pearce wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> > This patch series provides a rudimentary, working implementation of "jgit 
> > diff".  It does not provide all modes of "git diff" -- by far! -- but it 
> > is robust, and should provide a good starting point for further work.
> 
> I have rebased this patch series on our new JGit history, and then
> opened a bug at eclipse.org to get it through the IP review process:
> 
>   https://bugs.eclipse.org/bugs/show_bug.cgi?id=291083

Great, thanks!

Ciao,
Dscho

Re: jgit diff, was Re: [JGIT] Request for help

[Christian Halstrick]

Johannes Schindelin <Johannes.Schindelin <at> gmx.de> writes:

...
> > > This is not really difficult in Java, however, it relies on a working 
> > > diff implementation (and IIRC my implementation has not yet been 
> > > integrated into JGit).
> > 
> > Speaking of... where does that stand?
> 
> Same as where I left off.  IOW it is a working implementation that saw 
> some testing, but I simply lack the time for performance tuning.

I can offer my help here. I looked at Dscho's code before, provided patches to
let it compile and run the tests (IIRC not fully successfully). I started in my
local repo to modify enhance the tests but didn't finished with that yet. I'll
try now to add some performance tests and tests derived from native Git diff 
tests. 

> 
> It should not be all that bad, though.
> 
...
> Seems I misremembered a bit.  Christian provided a patch to make it 
> compileable, but I think that I ran the script to verify that the diffs 
> are correct on jgit.git and IIRC it completed fine.

Re: [JGIT] Request for help

[Jonas Fonseca]

On Wed, Sep 2, 2009 at 21:22, Shawn O. Pearce<spearce@spearce.org> wrote:
> Yea, for the most part I think we use Eclipse, and you just have
> to import JGit's top level directory into Eclipse as it comes with
> Eclipse project files.  But I know some folks only use our Maven
> build (under jgit-maven/jgit) or use NetBeans.  I have no idea how
> to import the project into the latter or configure its unit tests
> to run.

NetBeans comes with very good support for Maven projects. Importing
JGit into NetBeans is just a matter of using the "Open Project" wizard
and locating jgit-maven/jgit. This will also configure the unit tests
to run.

BTW, what is your opinion of making it a bit easier to import and use
the Maven configuration by putting a pom.xml in the top-level
directory? The actual pom.xml file responsible for building the jgit
library can still live on in jgit-maven/ if that is preferable.

I am also thinking about "mavenizing" the .pgm subproject to make it
easier to browse and search the code from within NetBeans.

-- 
Jonas Fonseca

Re: [JGIT] Request for help

[Shawn O. Pearce]

Jonas Fonseca <jonas.fonseca@gmail.com> wrote:
> BTW, what is your opinion of making it a bit easier to import and use
> the Maven configuration by putting a pom.xml in the top-level
> directory? The actual pom.xml file responsible for building the jgit
> library can still live on in jgit-maven/ if that is preferable.
> 
> I am also thinking about "mavenizing" the .pgm subproject to make it
> easier to browse and search the code from within NetBeans.

Actually, now that we have forked out of the egit.git repository,
I want to refactor the layout of the JGit project to be more maven
like, and have a proper top-level pom to build things.

Unfortunately it seems that nobody can program a proper Maven pom
for a multi-project project unless they are one of the authors
of Maven itself.  I watched the Apache MINA team struggle with it
until the Maven guys wanted to use their code, and fixed their build.

So, this refactoring is waiting for a Maven guru to contribute
an improvement.  Unfortunately they are all busy...

So, to answer your original question, yes, we should make this
better, and patches are welcome.  My own Maven-fu is just not up
to the task.

-- 
Shawn.

Re: [JGIT] Request for help

[Jonas Fonseca]

On Thu, Sep 3, 2009 at 10:42, Shawn O. Pearce<spearce@spearce.org> wrote:
> Jonas Fonseca <jonas.fonseca@gmail.com> wrote:
>> BTW, what is your opinion of making it a bit easier to import and use
>> the Maven configuration by putting a pom.xml in the top-level
>> directory? The actual pom.xml file responsible for building the jgit
>> library can still live on in jgit-maven/ if that is preferable.
>>
>> I am also thinking about "mavenizing" the .pgm subproject to make it
>> easier to browse and search the code from within NetBeans.
>
> Actually, now that we have forked out of the egit.git repository,
> I want to refactor the layout of the JGit project to be more maven
> like, and have a proper top-level pom to build things.

What kind of module structure do you have in mind? Do you want to move
some of the modules/subdirectories?
Some refactoring of the maven setup for JGit back was done back in
April in sonatype's (a maven company) JGit clone. It is not
signed-off, but can serve as a reference.

 - http://github.com/sonatype/JGit/commit/641ae523c496f381a7673f4acfa0acdff9d3913e

The Maven layout in the sonatype clone simply uses the Eclipse project layout.

pom.xml: JGit :: Parent
 |- org.spearce.jgit/pom.xml: JGit :: Core
 |- org.spearce.jgit.pgm/pom.xml: JGit :: Programs
 `- org.spearce.jgit.test/pom.xml: JGit :: Test

However, having tests in a separate module can be both good/bad. For
example, they will not automatically get run when you only build the
Core module.

Anyway, I would like to help.

-- 
Jonas Fonseca

Re: [JGIT] Request for help

[Shawn O. Pearce]

Jonas Fonseca <jonas.fonseca@gmail.com> wrote:
> On Thu, Sep 3, 2009 at 10:42, Shawn O. Pearce<spearce@spearce.org> wrote:
> > Actually, now that we have forked out of the egit.git repository,
> > I want to refactor the layout of the JGit project to be more maven
> > like, and have a proper top-level pom to build things.
> 
> What kind of module structure do you have in mind? Do you want to move
> some of the modules/subdirectories?
> Some refactoring of the maven setup for JGit back was done back in
> April in sonatype's (a maven company) JGit clone. It is not
> signed-off, but can serve as a reference.

Yea, I was hoping they would contribute this back as patches,
but thus far they haven't.
 
> The Maven layout in the sonatype clone simply uses the Eclipse project layout.
> 
> pom.xml: JGit :: Parent
>  |- org.spearce.jgit/pom.xml: JGit :: Core
>  |- org.spearce.jgit.pgm/pom.xml: JGit :: Programs
>  `- org.spearce.jgit.test/pom.xml: JGit :: Test
> 
> However, having tests in a separate module can be both good/bad. For
> example, they will not automatically get run when you only build the
> Core module.

Yea, I know.  This is one area where Maven is just whack, by putting
the tests in the same project the Maven plugin for Eclipse puts
them into the same classpath, which means you can see test code
from project code.  Wrong.  They should be different projects so
the test classpath is isolated.

However.  This is a bug in the Eclipse plugin I think, not
necessarily with Maven's approach of trying to keep tests alongside
the code they test.  Thus we probably want:

  pom.xml: JGit :: Parent
   |- jgit-lib/pom.xml: JGit
   |     src/main/java  <-- from org.spearce.jgit/src
   |     src/test/java  <-- from org.spearce.jgit.test/src
   |
   `- jgit-pgm/pom.xml: JGit pgm
         src/main/java  <-- from org.spearce.jgit.pgm/src

IIRC there is Maven support to create proper MANIFEST.MF files for
OSGI bundles, which is what we need for the Eclipse plugin support.
That should be able to replace the META-INF/MANIFEST.MF in the top
of each of the current directories.

> Anyway, I would like to help.

Please post patches; formatted with -M.  I do want to do this, I just
don't have the patience and Maven-fu to write the new poms myself.

-- 
Shawn.

Re: [JGIT] Request for help

[Gabe McArthur]

 
Shawn O. Pearce <spearce <at> spearce.org> writes:

> 
> Please post patches; formatted with -M.  I do want to do this, I just
> don't have the patience and Maven-fu to write the new poms myself.
> 


Hey,
I'm a build engineer with a considerable amount of "Maven-fu" :).  I've actually 
generated a patch that does everything you want (and a bit more).  I'm not that 
familiar with git's command line yet, so it's a bit tricky to get the patch 
thing right.  However, here's a rough overview of what I did:

ROOT
====
README
/bin
  bash.env   -- A script that you can source from Bash that
                will add the 'jgit' executable and the other
                scripts in this 'bin' directory to your PATH
  build.sh   -- A general build script, that hides some
                Maven complexities for initiates.
  tag.sh     -- Ok, this is the only thing that will have to 
                be re-written.  It's too tied in with git commands for
                me to fully extract what it's supposed to do.
/docs
  LICENSE
  SUBMITTING_PATCHES
  TODO
pom.xml      -- A considerable amount of build logic has been
                centralized here.  It references 3 sub-module
                projects, listed below.
/sources
  /jgit-lib
    pom.xml
    /src/main/java....
    /src/test
      /java....
      /resources 
      /exttst   -- Don't know exactly where this goes, as it
                   doesn't seem to be doing much/being run 
                   currently.
  /jgit-pgm
    pom.xml     -- Does the work to do a 'jar-with-dependencies' 
                   so that org.spearce.jgit.pgm.build can be removed.
    /src/main/java....
  /jgit-exec
    pom.xml     -- Actually generates the 'jgit' executable and
                   installs it in ROOT/target/bin, so that it will
                   be on your path after sourcing 'bin/bash.env'
    /src/main/scripts/jgit

I'll try to submit a full patch later, using your conventions.

My appreciation to Shawn for pointing out this thread....
-Gabe
                

Re: [JGIT] Request for help

[Mark Struberg]

Seems this speeds up lately ;)

Gabe, please allow me a few questions:

.) why do we need the /sources directory layer? I think /jgit and /jgit-pgm would be enough.

.) imho the docs should stay in / at least the LICENSE file

.) we don't need a tag.sh any more if we work with maven. Maven now has the maven-scm-provider-gitexe activated by default (since early 2008), so 
mvn release:prepare
mvn release:perform
should work if we set the proper <scm> section. Any feedback or bugreporting on the maven-git integration is highly welcome btw ;)


LieGrue,
strub


--- On Fri, 9/4/09, Gabe McArthur <gabriel.mcarthur@gmail.com> wrote:

> From: Gabe McArthur <gabriel.mcarthur@gmail.com>
> Subject: Re: [JGIT] Request for help
> To: git@vger.kernel.org
> Date: Friday, September 4, 2009, 7:00 AM
>  
> Shawn O. Pearce <spearce <at> spearce.org>
> writes:
> 
> > 
> > Please post patches; formatted with -M.  I do
> want to do this, I just
> > don't have the patience and Maven-fu to write the new
> poms myself.
> > 
> 
> 
> Hey,
> I'm a build engineer with a considerable amount of
> "Maven-fu" :).  I've actually 
> generated a patch that does everything you want (and a bit
> more).  I'm not that 
> familiar with git's command line yet, so it's a bit tricky
> to get the patch 
> thing right.  However, here's a rough overview of what
> I did:
> 
> ROOT
> ====
> README
> /bin
>   bash.env   -- A script that you can
> source from Bash that
>                
> will add the 'jgit' executable and the other
>                
> scripts in this 'bin' directory to your PATH
>   build.sh   -- A general build script,
> that hides some
>                
> Maven complexities for initiates.
>   tag.sh     -- Ok, this is the
> only thing that will have to 
>                 be
> re-written.  It's too tied in with git commands for
>                 me
> to fully extract what it's supposed to do.
> /docs
>   LICENSE
>   SUBMITTING_PATCHES
>   TODO
> pom.xml      -- A considerable amount of
> build logic has been
>                
> centralized here.  It references 3 sub-module
>                
> projects, listed below.
> /sources
>   /jgit-lib
>     pom.xml
>     /src/main/java....
>     /src/test
>       /java....
>       /resources 
>       /exttst   -- Don't know
> exactly where this goes, as it
>                
>    doesn't seem to be doing much/being run 
>                
>    currently.
>   /jgit-pgm
>     pom.xml     -- Does the
> work to do a 'jar-with-dependencies' 
>                
>    so that org.spearce.jgit.pgm.build can be
> removed.
>     /src/main/java....
>   /jgit-exec
>     pom.xml     -- Actually
> generates the 'jgit' executable and
>                
>    installs it in ROOT/target/bin, so that it
> will
>                
>    be on your path after sourcing
> 'bin/bash.env'
>     /src/main/scripts/jgit
> 
> I'll try to submit a full patch later, using your
> conventions.
> 
> My appreciation to Shawn for pointing out this thread....
> -Gabe
>                 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe
> git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


      

Re: [JGIT] Request for help

[Jonas Fonseca]

On Fri, Sep 4, 2009 at 03:33, Mark Struberg<struberg@yahoo.de> wrote:
> .) we don't need a tag.sh any more if we work with maven. Maven now has the maven-scm-provider-gitexe activated by default (since early 2008), so
> mvn release:prepare
> mvn release:perform
> should work if we set the proper <scm> section. Any feedback or bugreporting on the maven-git integration is highly welcome btw ;)

If tag_jgit.sh goes away, it could be nice to add a document showing
how releasing/tagging it's done the maven way.

-- 
Jonas Fonseca

Re: [JGIT] Request for help

[Mark Struberg]

Hi Jonas!

See the following documentation:

http://maven.apache.org/plugins/maven-release-plugin/index.html
http://maven.apache.org/plugins/maven-release-plugin/examples/prepare-release.html
http://maven.apache.org/plugins/maven-release-plugin/examples/perform-release.html

There is also a freely available book from sonatype [1] which contains all the documemtation you need - plus fn lot more :)

LieGrue,
strub

[1] http://www.sonatype.com/book/


--- On Fri, 9/4/09, Jonas Fonseca <jonas.fonseca@gmail.com> wrote:

> From: Jonas Fonseca <jonas.fonseca@gmail.com>
> Subject: Re: [JGIT] Request for help
> To: "Mark Struberg" <struberg@yahoo.de>
> Cc: git@vger.kernel.org, "Gabe McArthur" <gabriel.mcarthur@gmail.com>
> Date: Friday, September 4, 2009, 2:22 PM
> On Fri, Sep 4, 2009 at 03:33, Mark
> Struberg<struberg@yahoo.de>
> wrote:
> > .) we don't need a tag.sh any more if we work with
> maven. Maven now has the maven-scm-provider-gitexe activated
> by default (since early 2008), so
> > mvn release:prepare
> > mvn release:perform
> > should work if we set the proper <scm> section.
> Any feedback or bugreporting on the maven-git integration is
> highly welcome btw ;)
> 
> If tag_jgit.sh goes away, it could be nice to add a
> document showing
> how releasing/tagging it's done the maven way.
> 
> -- 
> Jonas Fonseca
> 


      

Re: [JGIT] Request for help

[Jonas Fonseca]

On Fri, Sep 4, 2009 at 03:33, Mark Struberg<struberg@yahoo.de> wrote:
>> From: Gabe McArthur <gabriel.mcarthur@gmail.com>
> >
>> I'll try to submit a full patch later, using your
>> conventions.

I have a question as well:

Support for using find bug is part of the Eclipse configuration (see
org.spearce.jgit/findBugs/), and I know that there's a find bug plugin
for Maven. From looking at sonatype's JGit repositories it is not
integrated. Have you managed to include it?

-- 
Jonas Fonseca

Re: [JGIT] Request for help

[Mark Struberg]

as an old saying tells us: how to climb a mountain? step after step! ;)

I suggest we create a fresh branch based on the Shawns current version and add all the features incrementally.

1.) move the directory structure over to mavens std layout
2.) create the scm section and try releases
3.) improve site generation and documentation
tbc


LieGrue,
strub


--- On Fri, 9/4/09, Jonas Fonseca <jonas.fonseca@gmail.com> wrote:

> From: Jonas Fonseca <jonas.fonseca@gmail.com>
> Subject: Re: [JGIT] Request for help
> To: "Mark Struberg" <struberg@yahoo.de>
> Cc: git@vger.kernel.org, "Gabe McArthur" <gabriel.mcarthur@gmail.com>
> Date: Friday, September 4, 2009, 2:41 PM
> On Fri, Sep 4, 2009 at 03:33, Mark
> Struberg<struberg@yahoo.de>
> wrote:
> >> From: Gabe McArthur <gabriel.mcarthur@gmail.com>
> > >
> >> I'll try to submit a full patch later, using your
> >> conventions.
> 
> I have a question as well:
> 
> Support for using find bug is part of the Eclipse
> configuration (see
> org.spearce.jgit/findBugs/), and I know that there's a find
> bug plugin
> for Maven. From looking at sonatype's JGit repositories it
> is not
> integrated. Have you managed to include it?
> 
> -- 
> Jonas Fonseca
> 


      

Re: [JGIT] Request for help

[Shawn O. Pearce]

Nasser Grainawi <nasser@codeaurora.org> wrote:
> Should PatchId be a class on its own, or just a method within the Patch  
> class?

Hmm, maybe a method on Patch is reasonable.

-- 
Shawn.

Re: [JGIT] Request for help

[Nasser Grainawi]

Shawn O. Pearce wrote:
> Nasser Grainawi <nasser@codeaurora.org> wrote:
>> Should PatchId be a class on its own, or just a method within the Patch  
>> class?
> 
> Hmm, maybe a method on Patch is reasonable.
> 

Going down this route, I'd add a few things to Patch.
patchId would be a private field (of type ObjectId?)
getPatchId would be a public method that returns patchId
and then likely a private method (computePatchId?) that actually 
generates the patchId

This way any method in Patch that would potentially change a Patch 
object's patch-id would call computePatchId before it returns.

Thoughts?

Re: [JGIT] Request for help

[Shawn O. Pearce]

Nasser Grainawi <nasser@codeaurora.org> wrote:
> Shawn O. Pearce wrote:
>> Hmm, maybe a method on Patch is reasonable.
>
> Going down this route, I'd add a few things to Patch.
> patchId would be a private field (of type ObjectId?)
> getPatchId would be a public method that returns patchId
> and then likely a private method (computePatchId?) that actually  
> generates the patchId

Sure, but getPatchId can compute it on demand on the first call,
and anyone who modifies the Patch would just need to clear out
the cached patchId value so the next call (if it ever comes) to
getPatchId would force it to recompute.

Most users of Patch won't want the patchId, so there is no reason
to compute it.

-- 
Shawn.

Re: [JGIT] Request for help

[Nasser Grainawi]

Shawn O. Pearce wrote:
> Nasser Grainawi <nasser@codeaurora.org> wrote:
>> Shawn O. Pearce wrote:
>>> Hmm, maybe a method on Patch is reasonable.
>> Going down this route, I'd add a few things to Patch.
>> patchId would be a private field (of type ObjectId?)
>> getPatchId would be a public method that returns patchId
>> and then likely a private method (computePatchId?) that actually  
>> generates the patchId
> 
> Sure, but getPatchId can compute it on demand on the first call,
> and anyone who modifies the Patch would just need to clear out
> the cached patchId value so the next call (if it ever comes) to
> getPatchId would force it to recompute.
> 
> Most users of Patch won't want the patchId, so there is no reason
> to compute it.
> 
Works for me, that's even easier. I'll get to work on implementing this,
but between (re-)learning Java and our legal dept, don't know when I'll
have a finished product to share...

I'll continue to post questions as I get them.
Thanks everyone for their help thus far :)