patchShow diffs for changed files under a new or deleted directory.

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

söndagen den 17 februari 2008 skrev du:

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());
	}
}

Hi Robin,

Robin Rosenberg escreveu:

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.

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.

måndagen den 18 februari 2008 skrev du:

Ok, I see. I'll test it a little and push soon.

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