[EGIT PATCH 0/5] Quickdiff improvements

[EGIT PATCH 0/5] Quickdiff improvements

[Robin Rosenberg]

I noticed Eclipse could hang for long period, especially
during rebase. This series fixes that.

-- robin

Robin Rosenberg (5):
  Make the equals method work for AnyObjectId, not just ObjectId
  quickdiff - Don't add GitDocument as repository listener more than
    once
  Move dcocument to repository mapping to GitDocument
  Update Quickdiff tracing statements
  Cache resolved ids in quickdiff document for faster update

 .../egit/ui/internal/decorators/GitDocument.java   |  104 +++++++++++++++++---
 .../internal/decorators/GitQuickDiffProvider.java  |   10 +--
 .../src/org/spearce/jgit/lib/AnyObjectId.java      |    4 +-
 .../src/org/spearce/jgit/lib/Repository.java       |    2 +
 .../src/org/spearce/jgit/revwalk/RevObject.java    |    2 +-
 5 files changed, 97 insertions(+), 25 deletions(-)

[EGIT PATCH 1/5] Make the equals method work for AnyObjectId, not just ObjectId

[Robin Rosenberg]

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---

This has nothing to do with the series, just trying to squeeze it in... The
quickdiff fixes works fine with our without this patch, but it seems reasonable.

 .../src/org/spearce/jgit/lib/AnyObjectId.java      |    4 ++--
 .../src/org/spearce/jgit/revwalk/RevObject.java    |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/AnyObjectId.java b/org.spearce.jgit/src/org/spearce/jgit/lib/AnyObjectId.java
index e2f70ca..2e3a43e 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/AnyObjectId.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/AnyObjectId.java
@@ -248,12 +248,12 @@ public int hashCode() {
 	 *            the other id to compare to. May be null.
 	 * @return true only if both ObjectIds have identical bits.
 	 */
-	public boolean equals(final ObjectId other) {
+	public boolean equals(final AnyObjectId other) {
 		return other != null ? equals(this, other) : false;
 	}
 
 	public boolean equals(final Object o) {
-		return equals((ObjectId) o);
+		return equals((AnyObjectId) o);
 	}
 
 	/**
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
index e8fb29f..1a13d0a 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
@@ -75,7 +75,7 @@ public final ObjectId getId() {
 	}
 
 	@Override
-	public final boolean equals(final ObjectId o) {
+	public final boolean equals(final AnyObjectId o) {
 		return this == o;
 	}
 
-- 
1.6.2.1.345.g89fb

[EGIT PATCH 2/5] quickdiff - Don't add GitDocument as repository listener more than once

[Robin Rosenberg]

Doing so was very costly and led to sessions being locked
for a long time while refreshing the reference document used
by Eclipse's quickdiff feature.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../egit/ui/internal/decorators/GitDocument.java   |    4 +++-
 .../src/org/spearce/jgit/lib/Repository.java       |    2 ++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java
index a9c0c7d..69e9295 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java
@@ -35,6 +35,9 @@ static GitDocument create(final IResource resource) throws IOException {
 		if (RepositoryProvider.getProvider(resource.getProject()) instanceof GitProvider) {
 			ret = new GitDocument(resource);
 			ret.populate();
+			final Repository repository = ret.getRepository();
+			if (repository != null)
+				repository.addRepositoryChangedListener(ret);
 		}
 		return ret;
 	}
@@ -52,7 +55,6 @@ void populate() throws IOException {
 			return;
 		final String gitPath = mapping.getRepoRelativePath(resource);
 		final Repository repository = getRepository();
-		repository.addRepositoryChangedListener(this);
 		String baseline = GitQuickDiffProvider.baseline.get(repository);
 		if (baseline == null)
 			baseline = Constants.HEAD;
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
index cfd92b8..0f396ed 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
@@ -1132,6 +1132,7 @@ public File getWorkDir() {
 	 * @param l
 	 */
 	public void addRepositoryChangedListener(final RepositoryListener l) {
+		assert !listeners.contains(l);
 		listeners.add(l);
 	}
 
@@ -1150,6 +1151,7 @@ public void removeRepositoryChangedListener(final RepositoryListener l) {
 	 * @param l
 	 */
 	public static void addAnyRepositoryChangedListener(final RepositoryListener l) {
+		assert !allListeners.contains(l);
 		allListeners.add(l);
 	}
 
-- 
1.6.2.1.345.g89fb

[EGIT PATCH 3/5] Move dcocument to repository mapping to GitDocument

[Robin Rosenberg]

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../egit/ui/internal/decorators/GitDocument.java   |   23 ++++++++++++++++++-
 .../internal/decorators/GitQuickDiffProvider.java  |    8 +------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java
index 69e9295..8c82a55 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (C) 2008, Robin Rosenberg <robin.rosenberg@dewire.com>
+ * Copyright (C) 2008, 2009 Robin Rosenberg <robin.rosenberg@dewire.com>
  *
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
@@ -8,6 +8,8 @@
 package org.spearce.egit.ui.internal.decorators;
 
 import java.io.IOException;
+import java.util.Map;
+import java.util.WeakHashMap;
 
 import org.eclipse.core.resources.IEncodedStorage;
 import org.eclipse.core.resources.IProject;
@@ -29,6 +31,7 @@
 
 class GitDocument extends Document implements RepositoryListener {
 	private final IResource resource;
+	static Map<GitDocument,Repository> doc2repo = new WeakHashMap<GitDocument, Repository>();
 
 	static GitDocument create(final IResource resource) throws IOException {
 		GitDocument ret = null;
@@ -44,7 +47,7 @@ static GitDocument create(final IResource resource) throws IOException {
 
 	private GitDocument(IResource resource) {
 		this.resource = resource;
-		GitQuickDiffProvider.doc2repo.put(this, getRepository());
+		GitDocument.doc2repo.put(this, getRepository());
 	}
 
 	void populate() throws IOException {
@@ -95,6 +98,7 @@ void populate() throws IOException {
 	}
 
 	void dispose() {
+		doc2repo.remove(this);
 		Repository repository = getRepository();
 		if (repository != null)
 			repository.removeRepositoryChangedListener(this);
@@ -119,4 +123,19 @@ private Repository getRepository() {
 			return mapping.getRepository();
 		return null;
 	}
+
+	/**
+	 * A change occurred to a repository. Update any GitDocument instances
+	 * referring to such repositories.
+	 * 
+	 * @param repository Repository which changed 
+	 * @throws IOException
+	 */
+	static void refreshRelevant(final Repository repository) throws IOException {
+		for (Map.Entry<GitDocument, Repository> i : doc2repo.entrySet()) {
+			if (i.getValue() == repository) {
+				i.getKey().populate();
+			}
+		}
+	}
 }
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitQuickDiffProvider.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitQuickDiffProvider.java
index 88f5ea0..6c71f3c 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitQuickDiffProvider.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitQuickDiffProvider.java
@@ -37,7 +37,6 @@
 	private IResource resource;
 
 	static Map<Repository,String> baseline = new WeakHashMap<Repository,String>();
-	static Map<GitDocument,Repository> doc2repo = new WeakHashMap<GitDocument, Repository>();
 
 	/**
 	 * Create the GitQuickDiffProvider instance
@@ -48,7 +47,6 @@ public GitQuickDiffProvider() {
 
 	public void dispose() {
 		Activator.trace("(GitQuickDiffProvider) dispose");
-		doc2repo.remove(document);
 		if (document != null)
 			document.dispose();
 	}
@@ -96,11 +94,7 @@ public void setId(String id) {
 	 */
 	public static void setBaselineReference(final Repository repository, final String baseline) throws IOException {
 		GitQuickDiffProvider.baseline.put(repository, baseline);
-		for (Map.Entry<GitDocument, Repository> i : doc2repo.entrySet()) {
-			if (i.getValue() == repository) {
-				i.getKey().populate();
-			}
-		}
+		GitDocument.refreshRelevant(repository);
 	}
 
 }
-- 
1.6.2.1.345.g89fb

[EGIT PATCH 4/5] Update Quickdiff tracing statements

[Robin Rosenberg]

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../egit/ui/internal/decorators/GitDocument.java   |    9 ++++++---
 .../internal/decorators/GitQuickDiffProvider.java  |    2 ++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java
index 8c82a55..347e6fc 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java
@@ -34,6 +34,7 @@
 	static Map<GitDocument,Repository> doc2repo = new WeakHashMap<GitDocument, Repository>();
 
 	static GitDocument create(final IResource resource) throws IOException {
+		Activator.trace("(GitDocument) create: " + resource);
 		GitDocument ret = null;
 		if (RepositoryProvider.getProvider(resource.getProject()) instanceof GitProvider) {
 			ret = new GitDocument(resource);
@@ -51,6 +52,7 @@ private GitDocument(IResource resource) {
 	}
 
 	void populate() throws IOException {
+		Activator.trace("(GitDocument) populate: " + resource);
 		set("");
 		final IProject project = resource.getProject();
 		RepositoryMapping mapping = RepositoryMapping.getMapping(project);
@@ -70,7 +72,7 @@ void populate() throws IOException {
 		}
 		TreeEntry blobEnry = baselineTree.findBlobMember(gitPath);
 		if (blobEnry != null) {
-			Activator.trace("(GitQuickDiffProvider) compareTo: " + baseline);
+			Activator.trace("(GitDocument) compareTo: " + baseline);
 			ObjectLoader loader = repository.openBlob(blobEnry.getId());
 			byte[] bytes = loader.getBytes();
 			String charset;
@@ -91,13 +93,14 @@ void populate() throws IOException {
 			// to the content. We don't do that here.
 			String s = new String(bytes, charset);
 			set(s);
-			Activator.trace("(GitQuickDiffProvider) has reference doc, size=" + s.length() + " bytes");
+			Activator.trace("(GitDocument) has reference doc, size=" + s.length() + " bytes");
 		} else {
-			Activator.trace("(GitQuickDiffProvider) no revision.");
+			Activator.trace("(GitDocument) no revision.");
 		}
 	}
 
 	void dispose() {
+		Activator.trace("(GitDocument) dispose: " + resource);
 		doc2repo.remove(this);
 		Repository repository = getRepository();
 		if (repository != null)
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitQuickDiffProvider.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitQuickDiffProvider.java
index 6c71f3c..91efb56 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitQuickDiffProvider.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitQuickDiffProvider.java
@@ -42,6 +42,7 @@
 	 * Create the GitQuickDiffProvider instance
 	 */
 	public GitQuickDiffProvider() {
+		Activator.trace("(GitQuickDiffProvider) constructor");
 		// Empty
 	}
 
@@ -77,6 +78,7 @@ public boolean isEnabled() {
 	}
 
 	public void setActiveEditor(ITextEditor editor) {
+		Activator.trace("(GitQuickDiffProvider) setActiveEditor: " + editor.getTitle());
 		IEditorInput editorInput = editor.getEditorInput();
 		resource = ResourceUtil.getResource(editorInput);
 	}
-- 
1.6.2.1.345.g89fb

[EGIT PATCH 5/5] Cache resolved ids in quickdiff document for faster update

[Robin Rosenberg]

We do this by caching the commit, tree and blob ids and can then
very quickly decide whether a change in baseline actually results in a
changed version of the reference blob used for quickdiff.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../egit/ui/internal/decorators/GitDocument.java   |   70 +++++++++++++++++---
 1 files changed, 60 insertions(+), 10 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java
index 347e6fc..59a738c 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitDocument.java
@@ -20,8 +20,11 @@
 import org.spearce.egit.core.GitProvider;
 import org.spearce.egit.core.project.RepositoryMapping;
 import org.spearce.egit.ui.Activator;
+import org.spearce.jgit.lib.AnyObjectId;
+import org.spearce.jgit.lib.Commit;
 import org.spearce.jgit.lib.Constants;
 import org.spearce.jgit.lib.IndexChangedEvent;
+import org.spearce.jgit.lib.ObjectId;
 import org.spearce.jgit.lib.ObjectLoader;
 import org.spearce.jgit.lib.RefsChangedEvent;
 import org.spearce.jgit.lib.Repository;
@@ -31,6 +34,11 @@
 
 class GitDocument extends Document implements RepositoryListener {
 	private final IResource resource;
+
+	private AnyObjectId lastCommit;
+	private AnyObjectId lastTree;
+	private AnyObjectId lastBlob;
+
 	static Map<GitDocument,Repository> doc2repo = new WeakHashMap<GitDocument, Repository>();
 
 	static GitDocument create(final IResource resource) throws IOException {
@@ -51,29 +59,68 @@ private GitDocument(IResource resource) {
 		GitDocument.doc2repo.put(this, getRepository());
 	}
 
+	private void setResolved(final AnyObjectId commit, final AnyObjectId tree, final AnyObjectId blob, final String value) {
+		lastCommit = commit != null ? commit.copy() : null;
+		lastTree = tree != null ? tree.copy() : null;
+		lastBlob = blob != null ? blob.copy() : null;
+		set(value);
+		if (blob != null)
+			Activator.trace("(GitDocument) resolved " + resource + " to " + lastBlob + " in " + lastCommit + "/" + lastTree);
+		else
+			Activator.trace("(GitDocument) unresolved " + resource);
+	}
+
 	void populate() throws IOException {
 		Activator.trace("(GitDocument) populate: " + resource);
-		set("");
 		final IProject project = resource.getProject();
 		RepositoryMapping mapping = RepositoryMapping.getMapping(project);
-		if (mapping == null)
+		if (mapping == null) {
+			setResolved(null, null, null, "");
 			return;
+		}
 		final String gitPath = mapping.getRepoRelativePath(resource);
-		final Repository repository = getRepository();
+		final Repository repository = mapping.getRepository();
 		String baseline = GitQuickDiffProvider.baseline.get(repository);
 		if (baseline == null)
 			baseline = Constants.HEAD;
-		Tree baselineTree = repository.mapTree(baseline);
-		if (baselineTree == null) {
+		ObjectId commitId = repository.resolve(baseline);
+		if (commitId != null) {
+			if (commitId.equals(lastCommit)) {
+				Activator.trace("(GitDocument) already resolved"); 
+				return;
+			}
+		} else {
 			Activator.logError("Could not resolve quickdiff baseline "
 					+ baseline + " corresponding to " + resource + " in "
 					+ repository, new Throwable());
+			setResolved(null, null, null, "");
+			return;
+		}
+		Commit baselineCommit = repository.mapCommit(commitId);
+		if (baselineCommit == null) {
+			Activator.logError("Could not load commit " + commitId + " for "
+					+ baseline + " corresponding to " + resource + " in "
+					+ repository, new Throwable());
+			setResolved(null, null, null, "");
+			return;
+		}
+		ObjectId treeId = baselineCommit.getTreeId();
+		if (treeId.equals(lastTree)) {
+			Activator.trace("(GitDocument) already resolved"); 
+			return;
+		}
+		Tree baselineTree = baselineCommit.getTree();
+		if (baselineTree == null) {
+			Activator.logError("Could not load tree " + treeId + " for "
+					+ baseline + " corresponding to " + resource + " in "
+					+ repository, new Throwable());
+			setResolved(null, null, null, "");
 			return;
 		}
-		TreeEntry blobEnry = baselineTree.findBlobMember(gitPath);
-		if (blobEnry != null) {
+		TreeEntry blobEntry = baselineTree.findBlobMember(gitPath);
+		if (blobEntry != null && !blobEntry.getId().equals(lastBlob)) {
 			Activator.trace("(GitDocument) compareTo: " + baseline);
-			ObjectLoader loader = repository.openBlob(blobEnry.getId());
+			ObjectLoader loader = repository.openBlob(blobEntry.getId());
 			byte[] bytes = loader.getBytes();
 			String charset;
 			// Get the encoding for the current version. As a matter of
@@ -92,10 +139,13 @@ void populate() throws IOException {
 			// Finally we could consider validating the content with respect
 			// to the content. We don't do that here.
 			String s = new String(bytes, charset);
-			set(s);
+			setResolved(commitId, baselineTree.getId(), blobEntry.getId(), s);
 			Activator.trace("(GitDocument) has reference doc, size=" + s.length() + " bytes");
 		} else {
-			Activator.trace("(GitDocument) no revision.");
+			if (blobEntry == null)
+				setResolved(null, null, null, "");
+			else
+				Activator.trace("(GitDocument) already resolved"); 
 		}
 	}
 
-- 
1.6.2.1.345.g89fb

Re: [EGIT PATCH 5/5] Cache resolved ids in quickdiff document for faster update

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> We do this by caching the commit, tree and blob ids and can then
> very quickly decide whether a change in baseline actually results in a
> changed version of the reference blob used for quickdiff.
...
> @@ -31,6 +34,11 @@
>  
>  class GitDocument extends Document implements RepositoryListener {
>  	private final IResource resource;
> +
> +	private AnyObjectId lastCommit;
> +	private AnyObjectId lastTree;
> +	private AnyObjectId lastBlob;

Should have been "ObjectId"; I amended the patch.

> +		Commit baselineCommit = repository.mapCommit(commitId);
> +		Tree baselineTree = baselineCommit.getTree();
> +		TreeEntry blobEntry = baselineTree.findBlobMember(gitPath);

Arrrgh.  We're still using Commit/Tree/TreeEntry to read file paths?

I'm applying this as-is, but we really need to start to transition
away from them.  I wanted to start deleting the mapCommit and its
friends from the Repository class.

-- 
Shawn.

Re: [EGIT PATCH 5/5] Cache resolved ids in quickdiff document for faster update

[Robin Rosenberg]

söndag 05 april 2009 22:36:04 skrev "Shawn O. Pearce" <spearce@spearce.org>:
> Arrrgh.  We're still using Commit/Tree/TreeEntry to read file paths?
>
> I'm applying this as-is, but we really need to start to transition
> away from them.  I wanted to start deleting the mapCommit and its
> friends from the Repository class.

Yeah, but the new API is more awkward and error-prone to use. The old
API is quite straightforward. I will try harder next time.

-- robin

Re: [EGIT PATCH 5/5] Cache resolved ids in quickdiff document for faster update

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> s?ndag 05 april 2009 22:36:04 skrev "Shawn O. Pearce" <spearce@spearce.org>:
> > Arrrgh.  We're still using Commit/Tree/TreeEntry to read file paths?
> >
> > I'm applying this as-is, but we really need to start to transition
> > away from them.  I wanted to start deleting the mapCommit and its
> > friends from the Repository class.
> 
> Yeah, but the new API is more awkward and error-prone to use. The old
> API is quite straightforward.

I see.  Then I failed.

What can we do to reduce that?

> I will try harder next time.

Shouldn't be necessary.  :-)

-- 
Shawn.

Re: [EGIT PATCH 2/5] quickdiff - Don't add GitDocument as repository listener more than once

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> Doing so was very costly and led to sessions being locked
> for a long time while refreshing the reference document used
> by Eclipse's quickdiff feature.
...
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> @@ -1132,6 +1132,7 @@ public File getWorkDir() {
>  	 * @param l
>  	 */
>  	public void addRepositoryChangedListener(final RepositoryListener l) {
> +		assert !listeners.contains(l);
>  		listeners.add(l);
>  	}
>  
> @@ -1150,6 +1151,7 @@ public void removeRepositoryChangedListener(final RepositoryListener l) {
>  	 * @param l
>  	 */
>  	public static void addAnyRepositoryChangedListener(final RepositoryListener l) {
> +		assert !allListeners.contains(l);
>  		allListeners.add(l);
>  	}

That's a race condition.  The two collections are Vectors so another
thread can add the listener between your assertion point and the
add call.

Also, if this really is considered to be very bad behavior on the
part of the application, maybe these should be real tests with
exceptions thrown, so they can't be disabled when assertions are
turned off in the JRE?

-- 
Shawn.

Re: [EGIT PATCH 2/5] quickdiff - Don't add GitDocument as repository listener more than once

[Robin Rosenberg]

söndag 05 april 2009 22:24:00 skrev "Shawn O. Pearce" <spearce@spearce.org>:
> Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> > Doing so was very costly and led to sessions being locked
> > for a long time while refreshing the reference document used
> > by Eclipse's quickdiff feature.
> ...
> > --- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> > +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> > @@ -1132,6 +1132,7 @@ public File getWorkDir() {
> >  	 * @param l
> >  	 */
> >  	public void addRepositoryChangedListener(final RepositoryListener l) {
> > +		assert !listeners.contains(l);
> >  		listeners.add(l);
> >  	}
> >  
> > @@ -1150,6 +1151,7 @@ public void removeRepositoryChangedListener(final RepositoryListener l) {
> >  	 * @param l
> >  	 */
> >  	public static void addAnyRepositoryChangedListener(final RepositoryListener l) {
> > +		assert !allListeners.contains(l);
> >  		allListeners.add(l);
> >  	}
> 
> That's a race condition.  The two collections are Vectors so another
> thread can add the listener between your assertion point and the
> add call.

That would be another programming error, to add the same listener from different threads. Hopefully it would be caught on the next run. Adding synchronized might be too much since I cannot disable it by disabling assert.

> Also, if this really is considered to be very bad behavior on the
> part of the application, maybe these should be real tests with
> exceptions thrown, so they can't be disabled when assertions are
> turned off in the JRE?

It's usually not terribly bad, but what happens isn't well defined. If it happens it's a programming error. Especially now that the API bans it.

Asserts are for detecting programming errors during program test. If the tests are good enough there will never be a reason for this assert to fail and so it won't be necessary in deployed code. We could probably have lots more of these
declarations of assumptions, most of which would be less obvious than this simple test.

In particular asserts are for detecting bad conditions the programmer controls (or should control). Anything that can be the result of user actions or depending on the runtime environment should have non assert tests.

-- robin