Refactor GitHistoryPage

Explicitly clear contained views (CommitFileDiffViewer and
CommitMessageViewer) by setting fields to 'null'. When used, throw an
IllegalStateException if a field is 'null'. This prevents strange
effects caused by outdated references that are hard to debug.

Call 'showHead', 'showRef' and 'showTag' only after the commit graph
has been initialized/updated (initAndStartRevWalk).

Change-Id: I9c5789729debc68070bd2aea35523b21e3659950
Signed-off-by: Philipp Thun <philipp.thun@sap.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

4 files changed, 67 insertions, 22 deletions

diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitFileDiffViewer.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitFileDiffViewer.java
index 71f460151a..fc0df9c1d7 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitFileDiffViewer.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitFileDiffViewer.java
@@ -186,8 +186,9 @@ class CommitFileDiffViewer extends TableViewer {
 				final IStructuredSelection iss = (IStructuredSelection) s;
 				for (Iterator<FileDiff> it = iss.iterator(); it.hasNext();) {
 					String relativePath = it.next().getPath();
-					String path = new Path(db.getWorkTree().getAbsolutePath())
-							.append(relativePath).toOSString();
+					String path = new Path(getRepository().getWorkTree()
+							.getAbsolutePath()).append(relativePath)
+							.toOSString();
 					openFileInEditor(path);
 				}
 			}
@@ -330,9 +331,9 @@ class CommitFileDiffViewer extends TableViewer {
 			IWorkbenchPage page = window.getActivePage();
 			IFileRevision rev = CompareUtils.getFileRevision(d.getPath(), d
 					.getChange().equals(ChangeType.DELETE) ? d.getCommit()
-					.getParent(0) : d.getCommit(), db, d.getChange().equals(
-					ChangeType.DELETE) ? d.getBlobs()[0] : d.getBlobs()[d
-					.getBlobs().length - 1]);
+					.getParent(0) : d.getCommit(), getRepository(), d
+					.getChange().equals(ChangeType.DELETE) ? d.getBlobs()[0]
+					: d.getBlobs()[d.getBlobs().length - 1]);
 			if (rev != null)
 				EgitUiEditorUtils.openEditor(page, rev,
 						new NullProgressMonitor());
@@ -361,7 +362,7 @@ class CommitFileDiffViewer extends TableViewer {
 
 		if (d.getBlobs().length == 2 && !d.getChange().equals(ChangeType.ADD))
 			base = CompareUtils.getFileRevisionTypedElement(p, c.getParent(0),
-					db, d.getBlobs()[0]);
+					getRepository(), d.getBlobs()[0]);
 		else
 			// Initial import
 			base = new GitCompareFileRevisionEditorInput.EmptyTypedElement(""); //$NON-NLS-1$
@@ -369,8 +370,8 @@ class CommitFileDiffViewer extends TableViewer {
 		if (d.getChange().equals(ChangeType.DELETE))
 			next = new GitCompareFileRevisionEditorInput.EmptyTypedElement(""); //$NON-NLS-1$
 		else
-			next = CompareUtils.getFileRevisionTypedElement(p, c, db, d
-					.getBlobs()[1]);
+			next = CompareUtils.getFileRevisionTypedElement(p, c,
+					getRepository(), d.getBlobs()[1]);
 
 		in = new GitCompareFileRevisionEditorInput(next, base, null);
 		CompareUtils.openInCompare(site.getWorkbenchWindow().getActivePage(),
@@ -379,9 +380,17 @@ class CommitFileDiffViewer extends TableViewer {
 	}
 
 	TreeWalk getTreeWalk() {
+		if (walker == null)
+			throw new IllegalStateException("TreeWalk has not been set"); //$NON-NLS-1$
 		return walker;
 	}
 
+	private Repository getRepository() {
+		if (db == null)
+			throw new IllegalStateException("Repository has not been set"); //$NON-NLS-1$
+		return db;
+	}
+
 	void setTreeWalk(Repository repository, TreeWalk walk) {
 		db = repository;
 		walker = walk;
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitMessageViewer.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitMessageViewer.java
index e46434cec1..ba4b42ff66 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitMessageViewer.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitMessageViewer.java
@@ -298,6 +298,12 @@ class CommitMessageViewer extends TextViewer implements
 		this.db = repository;
 	}
 
+	private Repository getRepository() {
+		if (db == null)
+			throw new IllegalStateException("Repository has not been set"); //$NON-NLS-1$
+		return db;
+	}
+
 	private void format() {
 		if (commit == null) {
 			setDocument(new Document(
@@ -315,7 +321,7 @@ class CommitMessageViewer extends TextViewer implements
 				.getAdapter(IWorkbenchSiteProgressService.class);
 		if (siteService == null)
 			return;
-		FormatJob.FormatRequest formatRequest = new FormatJob.FormatRequest(db,
+		FormatJob.FormatRequest formatRequest = new FormatJob.FormatRequest(getRepository(),
 				commit, fill, currentDiffs, SYS_LINKCOLOR, SYS_DARKGRAY,
 				SYS_HUNKHEADER_COLOR, SYS_LINES_ADDED_COLOR,
 				SYS_LINES_REMOVED_COLOR);
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/FileDiffContentProvider.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/FileDiffContentProvider.java
index 8711c71ab6..50e034e458 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/FileDiffContentProvider.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/FileDiffContentProvider.java
@@ -30,8 +30,13 @@ public class FileDiffContentProvider implements IStructuredContentProvider {
 
 	public void inputChanged(final Viewer newViewer, final Object oldInput,
 			final Object newInput) {
-		walk = ((CommitFileDiffViewer) newViewer).getTreeWalk();
-		commit = (RevCommit) newInput;
+		if (newInput != null) {
+			walk = ((CommitFileDiffViewer) newViewer).getTreeWalk();
+			commit = (RevCommit) newInput;
+		} else {
+			walk = null;
+			commit = null;
+		}
 		diff = null;
 	}
 
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitHistoryPage.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitHistoryPage.java
index 8b9524b0ea..35a3c9bf60 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitHistoryPage.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitHistoryPage.java
@@ -822,9 +822,19 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener {
 		currentRepo = null;
 		name = ""; //$NON-NLS-1$
 		input = null;
+		clearCommentViewer();
+		clearFileViewer();
+		setInput(null);
+	}
+
+	private void clearCommentViewer() {
+		commentViewer.setRepository(null);
 		commentViewer.setInput(null);
+	}
+
+	private void clearFileViewer() {
+		fileViewer.setTreeWalk(null, null);
 		fileViewer.setInput(null);
-		setInput(null);
 	}
 
 	@Override
@@ -921,44 +931,52 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener {
 				return false;
 			}
 
+			boolean showHead = false;
+			boolean showRef = false;
+			boolean showTag = false;
+			Repository repo = null;
+			Ref ref = null;
 			if (o instanceof IResource) {
 				RepositoryMapping mapping = RepositoryMapping
 						.getMapping((IResource) o);
 				if (mapping != null) {
-					Repository repo = mapping.getRepository();
+					repo = mapping.getRepository();
 					input = new HistoryPageInput(repo,
 							new IResource[] { (IResource) o });
-					showHead(repo);
+					showHead = true;
 				}
 			} else if (o instanceof RepositoryTreeNode) {
 				RepositoryTreeNode repoNode = (RepositoryTreeNode) o;
-				Repository repo = repoNode.getRepository();
+				repo = repoNode.getRepository();
 				switch (repoNode.getType()) {
 				case FILE:
 					File file = ((FileNode) repoNode).getObject();
 					input = new HistoryPageInput(repo, new File[] { file });
-					showHead(repo);
+					showHead = true;
 					break;
 				case FOLDER:
 					File folder = ((FolderNode) repoNode).getObject();
 					input = new HistoryPageInput(repo, new File[] { folder });
-					showHead(repo);
+					showHead = true;
 					break;
 				case REF:
 					input = new HistoryPageInput(repo);
-					showRef(((RefNode) repoNode).getObject(), repo);
+					ref = ((RefNode) repoNode).getObject();
+					showRef = true;
 					break;
 				case ADDITIONALREF:
 					input = new HistoryPageInput(repo);
-					showRef(((AdditionalRefNode) repoNode).getObject(), repo);
+					ref = ((AdditionalRefNode) repoNode).getObject();
+					showRef = true;
 					break;
 				case TAG:
 					input = new HistoryPageInput(repo);
-					showTag(((TagNode) repoNode).getObject(), repo);
+					ref = ((TagNode) repoNode).getObject();
+					showTag = true;
 					break;
 				default:
 					input = new HistoryPageInput(repo);
-					showHead(repo);
+					showHead = true;
 					break;
 				}
 			} else if (o instanceof HistoryPageInput)
@@ -969,7 +987,7 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener {
 				if (resource != null) {
 					RepositoryMapping mapping = RepositoryMapping
 							.getMapping(resource);
-					Repository repo = mapping.getRepository();
+					repo = mapping.getRepository();
 					input = new HistoryPageInput(repo,
 							new IResource[] { resource });
 				}
@@ -1012,6 +1030,13 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener {
 				return false;
 			}
 
+			if (showHead)
+				showHead(repo);
+			if (showRef)
+				showRef(ref, repo);
+			if (showTag)
+				showTag(ref, repo);
+
 			return true;
 		} finally {
 			if (trace)