Fix showing selected refs or commits in history view

When the history view shows only the current branch ("Show all
branches and tags" _off_), selecting some other branch in the git
repositories view might not show the selected branch. It would be
shown only if that branch happened to be merged into the currently
checked out branch. The same could be observed when using "Show In
History" on a commit in the results of a "Git Search": the commit
would only be selected if it was contained in the branch currently
checked out.

Change this by always considering such a selected branch or commit
as an additional start point for the RevWalk. This enables the user
to make the history view show exactly two branches (plus all those
merged into these two), and also correctly selects commits not on
the current branch.

Bug: 360868
Change-Id: Ib2e071f40c57b06a0ce0a51defac3fe30f481e03
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

3 files changed, 130 insertions, 33 deletions

diff --git a/org.eclipse.egit.ui.test/src/org/eclipse/egit/ui/test/history/HistoryViewTest.java b/org.eclipse.egit.ui.test/src/org/eclipse/egit/ui/test/history/HistoryViewTest.java
index 3e95720b0c..268ce7b830 100644
--- a/org.eclipse.egit.ui.test/src/org/eclipse/egit/ui/test/history/HistoryViewTest.java
+++ b/org.eclipse.egit.ui.test/src/org/eclipse/egit/ui/test/history/HistoryViewTest.java
@@ -14,6 +14,7 @@ package org.eclipse.egit.ui.test.history;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
@@ -25,12 +26,16 @@ import org.eclipse.core.resources.IFile;
 import org.eclipse.core.resources.IFolder;
 import org.eclipse.core.resources.IProject;
 import org.eclipse.core.resources.ResourcesPlugin;
+import org.eclipse.core.runtime.Adapters;
 import org.eclipse.core.runtime.jobs.Job;
+import org.eclipse.egit.core.RepositoryUtil;
+import org.eclipse.egit.ui.Activator;
 import org.eclipse.egit.ui.JobFamilies;
-import org.eclipse.egit.ui.common.LocalRepositoryTestCase;
 import org.eclipse.egit.ui.internal.UIText;
+import org.eclipse.egit.ui.internal.repository.RepositoriesView;
 import org.eclipse.egit.ui.test.ContextMenuHelper;
 import org.eclipse.egit.ui.test.TestUtil;
+import org.eclipse.egit.ui.view.repositories.GitRepositoriesViewTestBase;
 import org.eclipse.jface.dialogs.IDialogConstants;
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.lib.Constants;
@@ -38,6 +43,7 @@ import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.swt.widgets.Table;
 import org.eclipse.swt.widgets.TableItem;
 import org.eclipse.swtbot.eclipse.finder.widgets.SWTBotView;
 import org.eclipse.swtbot.swt.finder.SWTBot;
@@ -56,7 +62,7 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 
 @RunWith(SWTBotJunit4ClassRunner.class)
-public class HistoryViewTest extends LocalRepositoryTestCase {
+public class HistoryViewTest extends GitRepositoriesViewTestBase {
 	private static final String SECONDFOLDER = "secondFolder";
 
 	private static final String ADDEDFILE = "another.txt";
@@ -68,7 +74,7 @@ public class HistoryViewTest extends LocalRepositoryTestCase {
 	private File repoFile;
 
 	@Before
-	public void setup() throws Exception {
+	public void setupTests() throws Exception {
 		repoFile = createProjectAndCommitToRepository();
 		IProject prj = ResourcesPlugin.getWorkspace().getRoot()
 				.getProject(PROJ1);
@@ -81,6 +87,9 @@ public class HistoryViewTest extends LocalRepositoryTestCase {
 		addAndCommit(addedFile, ADDEDMESSAGE);
 		// TODO count the commits
 		commitCount = 3;
+		RepositoryUtil repositoryUtil = Activator.getDefault()
+				.getRepositoryUtil();
+		repositoryUtil.addConfiguredRepository(repoFile);
 	}
 
 	@Test
@@ -228,6 +237,15 @@ public class HistoryViewTest extends LocalRepositoryTestCase {
 				getHistoryViewTable(PROJ1).getTableItem(0).getText(1));
 	}
 
+	private SWTBotTable getHistoryViewTable() throws Exception {
+		SWTBot historyView = getHistoryViewBot();
+		Job.getJobManager().join(JobFamilies.GENERATE_HISTORY, null);
+		historyView.getDisplay().syncExec(() -> {
+			// Join UI update triggered by GenerateHistoryJob
+		});
+		return historyView.table();
+	}
+
 	/**
 	 * @param path
 	 *            must be length 2 or three (folder or file)
@@ -252,17 +270,7 @@ public class HistoryViewTest extends LocalRepositoryTestCase {
 		explorerItem.select();
 		ContextMenuHelper.clickContextMenuSync(projectExplorerTree, "Team",
 				"Show in History");
-		// join GenerateHistoryJob
-		Job.getJobManager().join(JobFamilies.GENERATE_HISTORY, null);
-		// join UI update triggered by GenerateHistoryJob
-		projectExplorerTree.widget.getDisplay().syncExec(new Runnable() {
-			@Override
-			public void run() {
-				// empty
-			}
-		});
-
-		return getHistoryViewBot().table();
+		return getHistoryViewTable();
 	}
 
 	private SWTBotTable getFileDiffTable() throws Exception {
@@ -280,6 +288,55 @@ public class HistoryViewTest extends LocalRepositoryTestCase {
 	}
 
 	@Test
+	public void testSelectBranch() throws Exception {
+		toggleShowAllBranchesButton(false);
+		SWTBotTable commitTable = getHistoryViewTable(PROJ1);
+		assertEquals("Unexpected number of commits", commitCount,
+				commitTable.rowCount());
+		// Current branch is "master". Create a new commit on a new branch, then
+		// switch back to master.
+		try (Git git = new Git(lookupRepository(repoFile))) {
+			git.checkout().setCreateBranch(true).setName("otherBranch").call();
+			TestUtil.waitForDecorations();
+			touchAndSubmit("Updated");
+			ObjectId otherCommit = git.getRepository().resolve("otherBranch");
+			Ref master = git.checkout().setName(Constants.MASTER).call();
+			assertNotNull("Branch is null", master.getLeaf().getObjectId());
+			assertNotEquals("Branch not switched", otherCommit,
+					master.getLeaf().getObjectId());
+			TestUtil.waitForDecorations();
+			// History table should not show otherCommit
+			commitTable = getHistoryViewTable();
+			assertEquals("Unexpected number of commits", commitCount,
+					commitTable.rowCount());
+			// Open git repo view, select "otherBranch".
+			SWTBotView view = TestUtil.showView(RepositoriesView.VIEW_ID);
+			TestUtil.joinJobs(JobFamilies.REPO_VIEW_REFRESH);
+			TestUtil.waitForDecorations();
+			SWTBotTree tree = view.bot().tree();
+			SWTBotTreeItem localBranches = myRepoViewUtil
+					.getLocalBranchesItem(tree, repoFile);
+			TestUtil.expandAndWait(localBranches).getNode("otherBranch")
+					.select();
+			ContextMenuHelper.clickContextMenuSync(tree, "Show In", "History");
+			// History table should show both branches
+			commitTable = getHistoryViewTable();
+			assertEquals("Unexpected number of commits", commitCount + 1,
+					commitTable.rowCount());
+			Table swtTable = commitTable.widget;
+			ObjectId[] firstId = { null };
+			swtTable.getDisplay().syncExec(() -> {
+				Object obj = swtTable.getItem(0).getData();
+				RevCommit c = Adapters.adapt(obj, RevCommit.class);
+				if (c != null) {
+					firstId[0] = c.getId();
+				}
+			});
+			assertEquals("Unexpected commit in table", otherCommit, firstId[0]);
+		}
+	}
+
+	@Test
 	public void testAddBranch() throws Exception {
 		Repository repo = lookupRepository(repoFile);
 		assertNull(repo.resolve(Constants.R_HEADS + "NewBranch"));
@@ -341,17 +398,21 @@ public class HistoryViewTest extends LocalRepositoryTestCase {
 
 	@Test
 	public void testShowAllBranches() throws Exception {
-		toggleShowAllBranchesButton(true);
-		final SWTBotTable table = getHistoryViewTable(PROJ1);
-		int commits = getHistoryViewTable(PROJ1).rowCount();
-		checkoutLine(table, 1);
-
-		toggleShowAllBranchesButton(false);
-		assertEquals("Wrong number of commits", commits - 1,
-				getHistoryViewTable(PROJ1).rowCount());
-		toggleShowAllBranchesButton(true);
-		assertEquals("Wrong number of commits", commits,
-				getHistoryViewTable(PROJ1).rowCount());
+		try {
+			toggleShowAllBranchesButton(true);
+			final SWTBotTable table = getHistoryViewTable(PROJ1);
+			int commits = getHistoryViewTable(PROJ1).rowCount();
+			checkoutLine(table, 1);
+
+			toggleShowAllBranchesButton(false);
+			assertEquals("Wrong number of commits", commits - 1,
+					getHistoryViewTable(PROJ1).rowCount());
+			toggleShowAllBranchesButton(true);
+			assertEquals("Wrong number of commits", commits,
+					getHistoryViewTable(PROJ1).rowCount());
+		} finally {
+			toggleShowAllBranchesButton(false);
+		}
 	}
 
 	@Test
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 589d849b87..3a9c6ec711 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
@@ -809,6 +809,9 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener,
 	/** Repository of the last input*/
 	private Repository currentRepo;
 
+	/** ObjectId of the ref or commit of the last input, if any. */
+	private ObjectId selectedObj;
+
 	private boolean currentShowAllBranches;
 
 	private boolean currentShowAdditionalRefs;
@@ -1661,6 +1664,8 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener,
 		}
 		renameTracker.reset(null);
 		Job.getJobManager().cancel(JobFamilies.HISTORY_DIFF);
+		currentRepo = null;
+		selectedObj = null;
 		super.dispose();
 	}
 
@@ -1685,6 +1690,7 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener,
 
 	private void clearHistoryPage() {
 		currentRepo = null;
+		selectedObj = null;
 		name = ""; //$NON-NLS-1$
 		input = null;
 		commentViewer.setInput(null);
@@ -1957,7 +1963,13 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener,
 
 			setErrorMessage(null);
 			try {
-				initAndStartRevWalk(false);
+				ObjectId id = null;
+				if (ref != null) {
+					id = ref.getLeaf().getObjectId();
+				} else if (selection != null) {
+					id = selection.getId();
+				}
+				initAndStartRevWalk(false, id);
 			} catch (IllegalStateException e) {
 				Activator.handleError(e.getMessage(), e, true);
 				return false;
@@ -2291,7 +2303,13 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener,
 		warningComposite.getParent().layout(true);
 	}
 
-	void initAndStartRevWalk(boolean forceNewWalk) throws IllegalStateException {
+	private void initAndStartRevWalk(boolean forceNewWalk) {
+		initAndStartRevWalk(forceNewWalk, selectedObj);
+	}
+
+	private void initAndStartRevWalk(boolean forceNewWalk,
+			ObjectId newSelectedObj)
+			throws IllegalStateException {
 		try {
 			if (trace)
 				GitTraceLocation.getTrace().traceEntry(
@@ -2309,6 +2327,7 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener,
 			if (headId == null) {
 				currentHeadId = null;
 				currentFetchHeadId = null;
+				selectedObj = null;
 				currentRepo = db;
 				clearViewers();
 				return;
@@ -2324,7 +2343,13 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener,
 				currentRepo = db;
 			}
 
-			if (forceNewWalk || repoChanged
+			boolean objChanged = false;
+			if (newSelectedObj != null && newSelectedObj != selectedObj) {
+				objChanged = !newSelectedObj.equals(selectedObj);
+			}
+			selectedObj = newSelectedObj;
+
+			if (forceNewWalk || repoChanged || objChanged
 					|| shouldRedraw(headId, fetchHeadId, paths)) {
 				releaseGenerateHistoryJob();
 
@@ -2501,7 +2526,7 @@ public class GitHistoryPage extends HistoryPage implements RefsChangedListener,
 			AnyObjectId fetchHeadId) {
 		currentHeadId = headId;
 		currentFetchHeadId = fetchHeadId;
-		SWTWalk walk = new GitHistoryWalk(db, headId);
+		SWTWalk walk = new GitHistoryWalk(db, headId, selectedObj);
 		try {
 			if (store
 					.getBoolean(UIPreferences.RESOURCEHISTORY_SHOW_ADDITIONAL_REFS))
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitHistoryWalk.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitHistoryWalk.java
index ec5fd7c7d5..57ccd8e79f 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitHistoryWalk.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GitHistoryWalk.java
@@ -21,6 +21,7 @@ import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefDatabase;
 import org.eclipse.jgit.lib.Repository;
@@ -39,9 +40,12 @@ class GitHistoryWalk extends SWTWalk {
 
 	private final AnyObjectId headId;
 
-	GitHistoryWalk(Repository repository, AnyObjectId headId) {
+	private final ObjectId toShow;
+
+	GitHistoryWalk(Repository repository, AnyObjectId headId, ObjectId toShow) {
 		super(repository);
 		this.headId = headId;
+		this.toShow = toShow;
 	}
 
 	@Override
@@ -85,6 +89,9 @@ class GitHistoryWalk extends SWTWalk {
 				markUninteresting(db, Constants.R_NOTES);
 			}
 			markStart(parseCommit(headId));
+			if (toShow != null) {
+				markStart(toShow);
+			}
 		} catch (IOException e) {
 			throw new IOException(MessageFormat.format(
 					UIText.GitHistoryPage_errorSettingStartPoints,
@@ -111,11 +118,10 @@ class GitHistoryWalk extends SWTWalk {
 		}
 	}
 
-	private void markStartRef(Ref ref)
+	private void markStart(ObjectId id)
 			throws IOException, IncorrectObjectTypeException {
 		try {
-			RevObject refTarget = parseAny(ref.getLeaf().getObjectId());
-			RevObject peeled = peel(refTarget);
+			RevObject peeled = peel(parseAny(id));
 			if (peeled instanceof RevCommit) {
 				markStart((RevCommit) peeled);
 			}
@@ -126,6 +132,11 @@ class GitHistoryWalk extends SWTWalk {
 		}
 	}
 
+	private void markStartRef(Ref ref)
+			throws IOException, IncorrectObjectTypeException {
+		markStart(ref.getLeaf().getObjectId());
+	}
+
 	private void markUninteresting(RefDatabase db, String prefix)
 			throws IOException, IncorrectObjectTypeException {
 		for (Ref ref : db.getRefsByPrefix(prefix)) {