Re: [EGIT PATCH] Show diffs for changed files under a new or deleted directory.

Re: [EGIT PATCH] Show diffs for changed files under a new or deleted directory.

[Robin Rosenberg]

söndagen den 17 februari 2008 skrev du:
> When a directory is added or removed in a commit the files under it 
> were not being shown in the structure compare viewer. This fixes it by
> adding diff nodes for all the files under this directory.
> 
> This patch also fixes some files showing as being removed in the
> structure compare when they were actually being added.
> 
> Signed-off-by: Roger C. Soares <rogersoares@intelinet.com.br>
> ---
> Hi Robin,
> 
> What is "TODO: Git ordering" supposed to change?

Should really read FIXME. It's a known bug that I forgot about at this place.

The git data structures has a nasty ordering of things that makes sense in 
the index but nowhere else. This is an example. say a.b and a=b is a file and 
a is a tree, then the 'git' order is:

a.b
a
a=b

i.e. the tree 'a' is sorted as if there is a '/' at then end. For full path
names that is no problem and it is actually trivial, but when you order
things by partial names it is much messier and quite hard to test also. There
is a bug in the IndexTreeWalker and probably the differenser and you found it.
It seems most likely you fixed it for that particular case, but it could still 
be broken for another.

I'd love to see a unit test for your code since, even if it works, it is very easy
to break again.

See also the topic "[EGIT Patches] Sort order from hell fixes". Unfortunately
the patches did *not* fix the ordering, it just moved the problem to another
set of input data. Patch 1/2 is ok, I think, but 2/2 is just bogus.

Here is one of the cases I'm struggling with now. I discovered it when trying
to commit things and the dialog shows allmost all files as modified. Another
user got no files listed. We probably should compute the DiffNode structure
using the IndexDiff too to make sure everyone does it right, once I (or we)
make it work. That would also give us the missing Index/workdir vs HEAD diff
feature too.

I've amended the existing tests since the original ones only tested the
"interesting" properties and missed some surprised so currently I'm trying to
make these tests work. We need more tests than this though since these tests
pass with known bad code.

-- robin

/*
 *  Copyright (C) 2007 David Watson <dwatson@mimvista.com>
 *
 *  This library is free software; you can redistribute it and/or
 *  modify it under the terms of the GNU Lesser General Public
 *  License, version 2.1, as published by the Free Software Foundation.
 *
 *  This library is distributed in the hope that it will be useful,
 *  but WITHOUT ANY WARRANTY; without even the implied warranty of
 *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 *  Lesser General Public License for more details.
 *
 *  You should have received a copy of the GNU Lesser General Public
 *  License along with this library; if not, write to the Free Software
 *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301
 */

package org.spearce.jgit.lib;

import java.io.File;
import java.io.IOException;

public class IndexDiffTest extends RepositoryTestCase {
	public void testAdded() throws IOException {
		GitIndex index = new GitIndex(db);
		writeTrashFile("file1", "file1");
		writeTrashFile("dir/subfile", "dir/subfile");
		Tree tree = new Tree(db);

		index.add(trash, new File(trash, "file1"));
		index.add(trash, new File(trash, "dir/subfile"));
		IndexDiff diff = new IndexDiff(tree, index);
		diff.diff();
		assertEquals(2, diff.getAdded().size());
		assertTrue(diff.getAdded().contains("file1"));
		assertTrue(diff.getAdded().contains("dir/subfile"));
		assertEquals(0, diff.getChanged().size());
		assertEquals(0, diff.getModified().size());
		assertEquals(0, diff.getRemoved().size());
	}

	public void testRemoved() throws IOException {
		GitIndex index = new GitIndex(db);
		writeTrashFile("file2", "file2");
		writeTrashFile("dir/file3", "dir/file3");

		Tree tree = new Tree(db);
		tree.addFile("file2");
		tree.addFile("dir/file3");
		assertEquals(2, tree.memberCount());
		tree.findBlobMember("file2").setId(new ObjectId("30d67d4672d5c05833b7192cc77a79eaafb5c7ad"));
		Tree tree2 = (Tree) tree.findTreeMember("dir");
		tree2.findBlobMember("file3").setId(new ObjectId("873fb8d667d05436d728c52b1d7a09528e6eb59b"));
		tree2.setId(new ObjectWriter(db).writeTree(tree2));
		tree.setId(new ObjectWriter(db).writeTree(tree));

		IndexDiff diff = new IndexDiff(tree, index);
		diff.diff();
		assertEquals(2, diff.getRemoved().size());
		assertTrue(diff.getRemoved().contains("file2"));
		assertTrue(diff.getRemoved().contains("dir/file3"));
		assertEquals(0, diff.getChanged().size());
		assertEquals(0, diff.getModified().size());
		assertEquals(0, diff.getAdded().size());
	}

	public void testModified() throws IOException {
		GitIndex index = new GitIndex(db);
		
		
		index.add(trash, writeTrashFile("file2", "file2"));
		index.add(trash, writeTrashFile("dir/file3", "dir/file3"));
		
		writeTrashFile("dir/file3", "changed");

		Tree tree = new Tree(db);
		tree.addFile("file2").setId(new ObjectId("0123456789012345678901234567890123456789"));
		tree.addFile("dir/file3").setId(new ObjectId("0123456789012345678901234567890123456789"));
		assertEquals(2, tree.memberCount());

		Tree tree2 = (Tree) tree.findTreeMember("dir");
		tree2.setId(new ObjectWriter(db).writeTree(tree2));
		tree.setId(new ObjectWriter(db).writeTree(tree));
		IndexDiff diff = new IndexDiff(tree, index);
		diff.diff();
		assertEquals(2, diff.getChanged().size());
		assertTrue(diff.getChanged().contains("file2"));
		assertTrue(diff.getChanged().contains("dir/file3"));
		assertEquals(1, diff.getModified().size());
		assertTrue(diff.getModified().contains("dir/file3"));
		assertEquals(0, diff.getAdded().size());
		assertEquals(0, diff.getRemoved().size());
		assertEquals(0, diff.getMissing().size());
	}

	public void testUnchangedSimple() throws IOException {
		GitIndex index = new GitIndex(db);

		index.add(trash, writeTrashFile("a.b", "a.b"));
		index.add(trash, writeTrashFile("a.c", "a.c"));
		index.add(trash, writeTrashFile("a=c", "a=c"));
		index.add(trash, writeTrashFile("a=d", "a=d"));

		Tree tree = new Tree(db);
		// got the hash id'd from the data using echo -n a.b|git hash-object -t blob --stdin
		tree.addFile("a.b").setId(new ObjectId("f6f28df96c2b40c951164286e08be7c38ec74851"));
		tree.addFile("a.c").setId(new ObjectId("6bc0e647512d2a0bef4f26111e484dc87df7f5ca"));
		tree.addFile("a=c").setId(new ObjectId("06022365ddbd7fb126761319633bf73517770714"));
		tree.addFile("a=d").setId(new ObjectId("fa6414df3da87840700e9eeb7fc261dd77ccd5c2"));

		tree.setId(new ObjectWriter(db).writeTree(tree));

		IndexDiff diff = new IndexDiff(tree, index);
		diff.diff();
		assertEquals(0, diff.getChanged().size());
		assertEquals(0, diff.getAdded().size());
		assertEquals(0, diff.getRemoved().size());
		assertEquals(0, diff.getMissing().size());
		assertEquals(0, diff.getModified().size());
	}

	/**
	 * This test has both files and directories that involve
	 * the tricky ordering used by Git.
	 *
	 * @throws IOException
	 */
	public void testUnchangedComplex() throws IOException {
		GitIndex index = new GitIndex(db);

		index.add(trash, writeTrashFile("a.b", "a.b"));
		index.add(trash, writeTrashFile("a.c", "a.c"));
		index.add(trash, writeTrashFile("a/b.b/b", "a/b.b/b"));
		index.add(trash, writeTrashFile("a/b", "a/b"));
		index.add(trash, writeTrashFile("a/c", "a/c"));
		index.add(trash, writeTrashFile("a=c", "a=c"));
		index.add(trash, writeTrashFile("a=d", "a=d"));

		Tree tree = new Tree(db);
		// got the hash id'd from the data using echo -n a.b|git hash-object -t blob --stdin
		tree.addFile("a.b").setId(new ObjectId("f6f28df96c2b40c951164286e08be7c38ec74851"));
		tree.addFile("a.c").setId(new ObjectId("6bc0e647512d2a0bef4f26111e484dc87df7f5ca"));
		tree.addFile("a/b.b/b").setId(new ObjectId("8d840bd4e2f3a48ff417c8e927d94996849933fd"));
		tree.addFile("a/b").setId(new ObjectId("db89c972fc57862eae378f45b74aca228037d415"));
		tree.addFile("a/c").setId(new ObjectId("52ad142a008aeb39694bafff8e8f1be75ed7f007"));
		tree.addFile("a=c").setId(new ObjectId("06022365ddbd7fb126761319633bf73517770714"));
		tree.addFile("a=d").setId(new ObjectId("fa6414df3da87840700e9eeb7fc261dd77ccd5c2"));

		Tree tree3 = (Tree) tree.findTreeMember("a/b.b");
		tree3.setId(new ObjectWriter(db).writeTree(tree3));
		Tree tree2 = (Tree) tree.findTreeMember("a");
		tree2.setId(new ObjectWriter(db).writeTree(tree2));
		tree.setId(new ObjectWriter(db).writeTree(tree));

		IndexDiff diff = new IndexDiff(tree, index);
		diff.diff();
		assertEquals(0, diff.getChanged().size());
		assertEquals(0, diff.getAdded().size());
		assertEquals(0, diff.getRemoved().size());
		assertEquals(0, diff.getMissing().size());
		assertEquals(0, diff.getModified().size());
	}
}

[EGIT PATCH] Show diffs for changed files under a new or deleted directory.

[Roger C. Soares]

When a directory is added or removed in a commit the files under it 
were not being shown in the structure compare viewer. This fixes it by
adding diff nodes for all the files under this directory.

This patch also fixes some files showing as being removed in the
structure compare when they were actually being added.

Signed-off-by: Roger C. Soares <rogersoares@intelinet.com.br>
---
Hi Robin,

What is "TODO: Git ordering" supposed to change?

Please evaluate.

[]s,
Roger.


 .../GitCompareFileRevisionEditorInput.java         |   79 +++++++++++++++++---
 1 files changed, 68 insertions(+), 11 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/GitCompareFileRevisionEditorInput.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/GitCompareFileRevisionEditorInput.java
index d3bc92a..d72093b 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/GitCompareFileRevisionEditorInput.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/GitCompareFileRevisionEditorInput.java
@@ -122,30 +122,65 @@ public class GitCompareFileRevisionEditorInput extends CompareEditorInput {
 			while (li<lc.length && ri<rc.length) {
 				ITypedElement ln = lc[li];
 				ITypedElement rn = rc[ri];
+				int compareTo = ln.getName().compareTo(rn.getName());
 				// TODO: Git ordering!
-				if (ln.getName().compareTo(rn.getName()) < 0) {
-					diffNode.add(new DiffNode(Differencer.ADDITION,null, ln, null));
-					++li;
-				}
-				if (ln.getName().compareTo(rn.getName()) > 0) {
-					diffNode.add(new DiffNode(Differencer.DELETION,null, null, rn));
-					++ri;
-				}
-				if (ln.getName().compareTo(rn.getName()) == 0) {
+				if (compareTo == 0) {
 					if (!ln.equals(rn))
 						diffNode.add(compare(ln,rn));
 					++li;
 					++ri;
+				} else if (compareTo < 0) {
+					DiffNode childDiffNode = new DiffNode(Differencer.ADDITION, null, ln, null);
+					diffNode.add(childDiffNode);
+					if (ln.getType().equals(ITypedElement.FOLDER_TYPE)) {
+						ITypedElement[] children = (ITypedElement[])((IStructureComparator)ln).getChildren();
+						if(children != null && children.length > 0) {
+							for (ITypedElement child : children) {
+								childDiffNode.add(addDirectoryFiles(child, Differencer.ADDITION));
+							}
+						}
+					}
+					++li;
+				} else {
+					DiffNode childDiffNode = new DiffNode(Differencer.DELETION, null, null, rn);
+					diffNode.add(childDiffNode);
+					if (rn.getType().equals(ITypedElement.FOLDER_TYPE)) {
+						ITypedElement[] children = (ITypedElement[])((IStructureComparator)rn).getChildren();
+						if(children != null && children.length > 0) {
+							for (ITypedElement child : children) {
+								childDiffNode.add(addDirectoryFiles(child, Differencer.DELETION));
+							}
+						}
+					}
+					++ri;
 				}
 			}
 			while (li<lc.length) {
 				ITypedElement ln = lc[li];
-				diffNode.add(new DiffNode(Differencer.ADDITION,null, ln, null));
+				DiffNode childDiffNode = new DiffNode(Differencer.ADDITION, null, ln, null);
+				diffNode.add(childDiffNode);
+				if (ln.getType().equals(ITypedElement.FOLDER_TYPE)) {
+					ITypedElement[] children = (ITypedElement[])((IStructureComparator)ln).getChildren();
+					if(children != null && children.length > 0) {
+						for (ITypedElement child : children) {
+							childDiffNode.add(addDirectoryFiles(child, Differencer.ADDITION));
+						}
+					}
+				}
 				++li;
 			}
 			while (ri<rc.length) {
 				ITypedElement rn = rc[ri];
-				diffNode.add(new DiffNode(Differencer.ADDITION,null, null, rn));
+				DiffNode childDiffNode = new DiffNode(Differencer.DELETION, null, null, rn);
+				diffNode.add(childDiffNode);
+				if (rn.getType().equals(ITypedElement.FOLDER_TYPE)) {
+					ITypedElement[] children = (ITypedElement[])((IStructureComparator)rn).getChildren();
+					if(children != null && children.length > 0) {
+						for (ITypedElement child : children) {
+							childDiffNode.add(addDirectoryFiles(child, Differencer.DELETION));
+						}
+					}
+				}
 				++ri;
 			}
 			return diffNode;
@@ -154,6 +189,28 @@ public class GitCompareFileRevisionEditorInput extends CompareEditorInput {
 		}
 	}
 
+	private DiffNode addDirectoryFiles(ITypedElement elem, int diffType) {
+		ITypedElement l = null;
+		ITypedElement r = null;
+		if (diffType == Differencer.DELETION) {
+			r = elem;
+		} else {
+			l = elem;
+		}
+
+		if (elem.getType().equals(ITypedElement.FOLDER_TYPE)) {
+			DiffNode diffNode = null;
+			diffNode = new DiffNode(null,Differencer.CHANGE,null,l,r);
+			ITypedElement[] children = (ITypedElement[])((IStructureComparator)elem).getChildren();
+			for (ITypedElement child : children) {
+				diffNode.add(addDirectoryFiles(child, diffType));
+			}
+			return diffNode;
+		} else {
+			return new DiffNode(diffType, null, l, r);
+		}
+	}
+
 	private void initLabels(ICompareInput input) {
 		CompareConfiguration cc = getCompareConfiguration();
 		if(left != null && left instanceof GitResourceNode) {
-- 
1.5.3.8

Re: [EGIT PATCH] Show diffs for changed files under a new or deleted directory.

[Roger C. Soares]

Hi Robin,

Robin Rosenberg escreveu:
> i.e. the tree 'a' is sorted as if there is a '/' at then end. For full path
> names that is no problem and it is actually trivial, but when you order
> things by partial names it is much messier and quite hard to test also. There
> is a bug in the IndexTreeWalker and probably the differenser and you found it.
> It seems most likely you fixed it for that particular case, but it could still 
> be broken for another.
>   
Thanks for your explanation, it makes more sense now.

I didn't look in detail yet the parts where we need to interact with the 
index and the workspace, but just thinking about it now. If we are using 
an algorithm which requires that both trees are sorted using the same 
order, and one of them can't be trusted to follow such order, them 
sorting the trees the way we want looks like a simple solution...

I intend to go back to this after I finish some more patches I'm working 
on to add more info into the revision detail viewer. So, lets go in 
steps and look at one distinct thing at a time. My patch doesn't change 
sorting or ordering of things and isn't very complicated either, it just 
adds missing functionality.

So, if in the old tree we had:

file1

and in the new tree we have:

file1
dir1/file2
dir1/file3

The compare viewer must show:

+ dir1/file2
+ dir1/file3

The current code only notices that dir1 was added and shows:

+ dir1

which confuses people reviewing code because they can't see that file2 
and file3 were added. What the patch does is take the next step and, if 
the added or removed node is a directory and it has children, add all 
the children to the compare viewer using the same Differencer from the 
directory.

The Differencer I changed was actually a bug. If you say that the files 
left in the left pane are Differencer.ADDITION, then the ones left on 
the right pane _must_ be Differencer.DELETION. It just doesn't make 
sense to say that files not on the old tree but on the new tree are 
addition, and at the same time that files on the old tree but not on the 
new tree are also addition.

There is another check that must be added. If in a commit I delete a 
directory and add a file with the same name of the directory and 
vice-versa. This case is still failing, it shows an alert with an empty 
message. But as I said, I intend to deal with it later, it is a more 
uncommon case and nobody here complained about it yet... :)

I think it should be like this for all cases, even when comparing with 
the index or the workspace, so if there is something else I'm missing 
please let me know.


> I'd love to see a unit test for your code since, even if it works, it is very easy
> to break again.
>   
Ok, I'll get a look on the test cases and probably write something when 
I get back into these compare issues. That if somebody else doesn't do 
it before me, of course :)

[]s,
Roger.

Re: [EGIT PATCH] Show diffs for changed files under a new or deleted directory.

[Robin Rosenberg]

måndagen den 18 februari 2008 skrev du:
> The Differencer I changed was actually a bug. If you say that the files 
> left in the left pane are Differencer.ADDITION, then the ones left on 
> the right pane _must_ be Differencer.DELETION. It just doesn't make 
> sense to say that files not on the old tree but on the new tree are 
> addition, and at the same time that files on the old tree but not on the 
> new tree are also addition.
Ok, I see. I'll test it a little and push soon.
> > I'd love to see a unit test for your code since, even if it works, it is very easy
> > to break again.
> >   
> Ok, I'll get a look on the test cases and probably write something when 
> I get back into these compare issues. That if somebody else doesn't do 
> it before me, of course :)

Not very likely within a short time frame, though I want do the excersize. Unfortunately I have too many open branches right now to add yet another. The original author (cough, cough) didn't do it. I was so happy and pleased, at them time, to see it work at all so easiliy :) . I've written some tests for other functions, so I guess it's mostly about setting up some more infrastructure to make testing reasonably simple.

-- robin