Make blame work correctly on merge conflicts

When a conflicting file was blamed, JGit would not identify lines
coming from the merge parents. The main cause for this was that
Blame and BlameCommand simply added the first DirCacheEntry found
for a file to its queue of candidates (blobs or commits) to consider.
In case of a conflict this typically is the merge base commit, and
comparing a auto-merged contents against that base would yield
incorrect results.

Such cases have to be handled specially. The candidate to be
considered by the blame must use the working tree contents, but
at the same time behave like a merge commit/candidate with HEAD
and the MERGE_HEADs as parents. Canonical git does something very
similar, see [1].

Implement that and add tests.

I first did this for the JGit pgm Blame command. When I then tried
to do the same in BlameCommand, I noticed that the latter also
included some fancy but incomplete CR-LF handling. In order to
be able to use the new BlameGenerator.prepareHead() also in
BlameCommand this CR-LF handling was also moved into BlameGenerator
and corrected in doing so.

(Just considering the git config settings was not good enough,
CR-LF behavior can also be influenced by .gitattributes, and even
by whether the file in the index has CR-LF. To correctly determine
CR-LF handling for check-in one needs to do a TreeWalk with at
least a FileTreeIterator and a DirCacheIterator.)

[1] https://github.com/git/git/blob/v2.22.0/blame.c#L174

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

org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/BlameTest.java

@@ -42,8 +42,13 @@
  */
 package org.eclipse.jgit.pgm;
 
+import static org.junit.Assert.assertTrue;
+
 import org.eclipse.jgit.api.Git;
+import org.eclipse.jgit.api.MergeResult;
 import org.eclipse.jgit.lib.CLIRepositoryTestCase;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.revwalk.RevCommit;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -119,4 +124,34 @@ public class BlameTest extends CLIRepositoryTestCase {
 		thrown.expectMessage("no such path 'sub/does_not_exist.txt' in HEAD");
 		execute("git blame sub/does_not_exist.txt");
 	}
+
+	@Test
+	public void testBlameMergeConflict() throws Exception {
+		try (Git git = new Git(db)) {
+			writeTrashFile("file", "Origin\n");
+			git.add().addFilepattern("file").call();
+			git.commit().setMessage("initial commit").call();
+			git.checkout().setCreateBranch(true)
+					.setName("side").call();
+			writeTrashFile("file",
+					"Conflicting change from side branch\n");
+			git.add().addFilepattern("file").call();
+			RevCommit side = git.commit().setMessage("side commit").call();
+			git.checkout().setName(Constants.MASTER).call();
+			writeTrashFile("file", "Change on master branch\n");
+			git.add().addFilepattern("file").call();
+			git.commit().setMessage("Commit conflict on master").call();
+			MergeResult result = git.merge()
+					.include("side", side).call();
+			assertTrue("Expected conflict on 'file'",
+					result.getConflicts().containsKey("file"));
+		}
+		String[] expected = {
+				"         (Not Committed Yet                            1) <<<<<<< HEAD",
+				"7a918de5 (GIT_COMMITTER_NAME 2009-08-15 20:12:58 -0330 2) Change on master branch",
+				"         (Not Committed Yet                            3) =======",
+				"beb52f68 (GIT_COMMITTER_NAME 2009-08-15 20:12:58 -0330 4) Conflicting change from side branch",
+				"         (Not Committed Yet                            5) >>>>>>> side" };
+		assertArrayOfLinesEquals(expected, execute("git blame file"));
+	}
 }

org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Blame.java

@@ -50,7 +50,6 @@ import static java.lang.Integer.valueOf;
 import static java.lang.Long.valueOf;
 import static org.eclipse.jgit.lib.Constants.OBJECT_ID_STRING_LENGTH;
 
-import java.io.File;
 import java.io.IOException;
 import java.text.MessageFormat;
 import java.text.SimpleDateFormat;
@@ -60,11 +59,11 @@ import java.util.List;
 import java.util.Map;
 import java.util.regex.Pattern;
 
+import org.eclipse.jgit.api.errors.NoHeadException;
 import org.eclipse.jgit.blame.BlameGenerator;
 import org.eclipse.jgit.blame.BlameResult;
 import org.eclipse.jgit.diff.RawText;
 import org.eclipse.jgit.diff.RawTextComparator;
-import org.eclipse.jgit.dircache.DirCache;
 import org.eclipse.jgit.errors.NoWorkTreeException;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
@@ -186,28 +185,7 @@ class Blame extends TextBuiltin {
 				}
 				generator.push(null, rev);
 			} else {
-				ObjectId head = db.resolve(Constants.HEAD);
-				if (head == null) {
-					throw die(MessageFormat.format(CLIText.get().noSuchRef,
-							Constants.HEAD));
-				}
-				generator.push(null, head);
-				if (!db.isBare()) {
-					DirCache dc = db.readDirCache();
-					int entry = dc.findEntry(file);
-					if (0 <= entry) {
-						generator.push(null, dc.getEntry(entry).getObjectId());
-					} else {
-						throw die(MessageFormat.format(
-								CLIText.get().noSuchPathInRef, file,
-								Constants.HEAD));
-					}
-
-					File inTree = new File(db.getWorkTree(), file);
-					if (db.getFS().isFile(inTree)) {
-						generator.push(null, new RawText(inTree));
-					}
-				}
+				generator.prepareHead();
 			}
 
 			blame = BlameResult.create(generator);
@@ -280,7 +258,7 @@ class Blame extends TextBuiltin {
 				} while (++line < end
 						&& sameCommit(blame.getSourceCommit(line), c));
 			}
-		} catch (NoWorkTreeException | IOException e) {
+		} catch (NoWorkTreeException | NoHeadException | IOException e) {
 			throw die(e.getMessage(), e);
 		}
 	}

org.eclipse.jgit.test/tst/org/eclipse/jgit/api/BlameCommandTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011, GitHub Inc.
+ * Copyright (C) 2011, 2019 GitHub Inc.
  * and other copyright owners as documented in the project's IP log.
  *
  * This program and the accompanying materials are made available
@@ -44,6 +44,7 @@ package org.eclipse.jgit.api;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
 import java.io.File;
@@ -461,6 +462,39 @@ public class BlameCommandTest extends RepositoryTestCase {
 		}
 	}
 
+	@Test
+	public void testUnresolvedMergeConflict() throws Exception {
+		try (Git git = new Git(db)) {
+			RevCommit base = commitFile("file.txt", "Origin\n", "master");
+
+			RevCommit master = commitFile("file.txt",
+					"Change on master branch\n", "master");
+
+			git.checkout().setName("side").setCreateBranch(true)
+					.setStartPoint(base).call();
+			RevCommit side = commitFile("file.txt",
+					"Conflicting change on side\n", "side");
+
+			checkoutBranch("refs/heads/master");
+			MergeResult result = git.merge().include(side).call();
+
+			// The merge results in a conflict, which we do not resolve
+			assertTrue("Expected a conflict",
+					result.getConflicts().containsKey("file.txt"));
+
+			BlameCommand command = new BlameCommand(db);
+			command.setFilePath("file.txt");
+			BlameResult lines = command.call();
+
+			assertEquals(5, lines.getResultContents().size());
+			assertNull(lines.getSourceCommit(0));
+			assertEquals(master, lines.getSourceCommit(1));
+			assertNull(lines.getSourceCommit(2));
+			assertEquals(side, lines.getSourceCommit(3));
+			assertNull(lines.getSourceCommit(4));
+		}
+	}
+
 	@Test
 	public void testWhitespaceMerge() throws Exception {
 		try (Git git = new Git(db)) {

org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties

@@ -457,6 +457,7 @@ nonBareLinkFilesNotSupported=Link files are not supported with nonbare repos
 nonCommitToHeads=Cannot point a branch to a non-commit object
 noPathAttributesFound=No Attributes found for {0}.
 noSuchRef=no such ref
+noSuchRefKnown=no such ref: {0}
 noSuchSubmodule=no such submodule {0}
 notABoolean=Not a boolean: {0}
 notABundle=not a bundle

org.eclipse.jgit/src/org/eclipse/jgit/api/BlameCommand.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011, GitHub Inc.
+ * Copyright (C) 2011, 2019 GitHub Inc.
  * and other copyright owners as documented in the project's IP log.
  *
  * This program and the accompanying materials are made available
@@ -42,11 +42,7 @@
  */
 package org.eclipse.jgit.api;
 
-import java.io.File;
-import java.io.FileInputStream;
-import java.io.FileNotFoundException;
 import java.io.IOException;
-import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -56,17 +52,10 @@ import org.eclipse.jgit.api.errors.JGitInternalException;
 import org.eclipse.jgit.blame.BlameGenerator;
 import org.eclipse.jgit.blame.BlameResult;
 import org.eclipse.jgit.diff.DiffAlgorithm;
-import org.eclipse.jgit.diff.RawText;
 import org.eclipse.jgit.diff.RawTextComparator;
-import org.eclipse.jgit.dircache.DirCache;
 import org.eclipse.jgit.lib.AnyObjectId;
-import org.eclipse.jgit.lib.Constants;
-import org.eclipse.jgit.lib.CoreConfig.AutoCRLF;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.treewalk.WorkingTreeOptions;
-import org.eclipse.jgit.util.IO;
-import org.eclipse.jgit.util.io.AutoLFInputStream;
 
 /**
  * Blame command for building a {@link org.eclipse.jgit.blame.BlameResult} for a
@@ -221,68 +210,11 @@ public class BlameCommand extends GitCommand<BlameResult> {
 			else if (startCommit != null)
 				gen.push(null, startCommit);
 			else {
-				gen.push(null, repo.resolve(Constants.HEAD));
-				if (!repo.isBare()) {
-					DirCache dc = repo.readDirCache();
-					int entry = dc.findEntry(path);
-					if (0 <= entry)
-						gen.push(null, dc.getEntry(entry).getObjectId());
-
-					File inTree = new File(repo.getWorkTree(), path);
-					if (repo.getFS().isFile(inTree)) {
-						RawText rawText = getRawText(inTree);
-						gen.push(null, rawText);
-					}
-				}
+				gen.prepareHead();
 			}
 			return gen.computeBlameResult();
 		} catch (IOException e) {
 			throw new JGitInternalException(e.getMessage(), e);
 		}
 	}
-
-	private RawText getRawText(File inTree) throws IOException,
-			FileNotFoundException {
-		RawText rawText;
-
-		WorkingTreeOptions workingTreeOptions = getRepository().getConfig()
-				.get(WorkingTreeOptions.KEY);
-		AutoCRLF autoCRLF = workingTreeOptions.getAutoCRLF();
-		switch (autoCRLF) {
-		case FALSE:
-		case INPUT:
-			// Git used the repo format on checkout, but other tools
-			// may change the format to CRLF. We ignore that here.
-			rawText = new RawText(inTree);
-			break;
-		case TRUE:
-			try (AutoLFInputStream in = new AutoLFInputStream(
-					new FileInputStream(inTree), true)) {
-				// Canonicalization should lead to same or shorter length
-				// (CRLF to LF), so the file size on disk is an upper size bound
-				rawText = new RawText(toByteArray(in, (int) inTree.length()));
-			}
-			break;
-		default:
-			throw new IllegalArgumentException(
-					"Unknown autocrlf option " + autoCRLF); //$NON-NLS-1$
-		}
-		return rawText;
-	}
-
-	private static byte[] toByteArray(InputStream source, int upperSizeLimit)
-			throws IOException {
-		byte[] buffer = new byte[upperSizeLimit];
-		try {
-			int read = IO.readFully(source, buffer, 0);
-			if (read == upperSizeLimit) {
-				return buffer;
-			}
-			byte[] copy = new byte[read];
-			System.arraycopy(buffer, 0, copy, 0, read);
-			return copy;
-		} finally {
-			source.close();
-		}
-	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011, Google Inc.
+ * Copyright (C) 2011, 2019 Google Inc.
  * and other copyright owners as documented in the project's IP log.
  *
  * This program and the accompanying materials are made available
@@ -48,11 +48,17 @@ import static org.eclipse.jgit.lib.FileMode.TYPE_FILE;
 import static org.eclipse.jgit.lib.FileMode.TYPE_MASK;
 
 import java.io.IOException;
+import java.io.InputStream;
+import java.text.MessageFormat;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.List;
 
 import org.eclipse.jgit.annotations.Nullable;
+import org.eclipse.jgit.api.errors.NoHeadException;
 import org.eclipse.jgit.blame.Candidate.BlobCandidate;
+import org.eclipse.jgit.blame.Candidate.HeadCandidate;
 import org.eclipse.jgit.blame.Candidate.ReverseCandidate;
 import org.eclipse.jgit.blame.ReverseWalk.ReverseCommit;
 import org.eclipse.jgit.diff.DiffAlgorithm;
@@ -63,8 +69,13 @@ import org.eclipse.jgit.diff.HistogramDiff;
 import org.eclipse.jgit.diff.RawText;
 import org.eclipse.jgit.diff.RawTextComparator;
 import org.eclipse.jgit.diff.RenameDetector;
+import org.eclipse.jgit.dircache.DirCache;
+import org.eclipse.jgit.dircache.DirCacheEntry;
+import org.eclipse.jgit.dircache.DirCacheIterator;
+import org.eclipse.jgit.errors.NoWorkTreeException;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.lib.AnyObjectId;
+import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.MutableObjectId;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectLoader;
@@ -74,9 +85,12 @@ import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevFlag;
 import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.treewalk.FileTreeIterator;
 import org.eclipse.jgit.treewalk.TreeWalk;
+import org.eclipse.jgit.treewalk.TreeWalk.OperationType;
 import org.eclipse.jgit.treewalk.filter.PathFilter;
 import org.eclipse.jgit.treewalk.filter.TreeFilter;
+import org.eclipse.jgit.util.IO;
 
 /**
  * Generate author information for lines based on a provided file.
@@ -312,6 +326,107 @@ public class BlameGenerator implements AutoCloseable {
 		return this;
 	}
 
+	/**
+	 * Pushes HEAD, index, and working tree as appropriate for blaming the file
+	 * given in the constructor {@link #BlameGenerator(Repository, String)}
+	 * against HEAD. Includes special handling in case the file is in conflict
+	 * state from an unresolved merge conflict.
+	 *
+	 * @return {@code this}
+	 * @throws NoHeadException
+	 *             if the repository has no HEAD
+	 * @throws IOException
+	 *             if an error occurs
+	 * @since 5.6
+	 */
+	public BlameGenerator prepareHead() throws NoHeadException, IOException {
+		Repository repo = getRepository();
+		ObjectId head = repo.resolve(Constants.HEAD);
+		if (head == null) {
+			throw new NoHeadException(MessageFormat
+					.format(JGitText.get().noSuchRefKnown, Constants.HEAD));
+		}
+		if (repo.isBare()) {
+			return push(null, head);
+		}
+		DirCache dc = repo.readDirCache();
+		try (TreeWalk walk = new TreeWalk(repo)) {
+			walk.setOperationType(OperationType.CHECKIN_OP);
+			FileTreeIterator iter = new FileTreeIterator(repo);
+			int fileTree = walk.addTree(iter);
+			int indexTree = walk.addTree(new DirCacheIterator(dc));
+			iter.setDirCacheIterator(walk, indexTree);
+			walk.setFilter(resultPath);
+			walk.setRecursive(true);
+			if (!walk.next()) {
+				return this;
+			}
+			DirCacheIterator dcIter = walk.getTree(indexTree,
+					DirCacheIterator.class);
+			if (dcIter == null) {
+				// Not found in index
+				return this;
+			}
+			iter = walk.getTree(fileTree, FileTreeIterator.class);
+			if (iter == null || !isFile(iter.getEntryRawMode())) {
+				return this;
+			}
+			RawText inTree;
+			long filteredLength = iter.getEntryContentLength();
+			try (InputStream stream = iter.openEntryStream()) {
+				inTree = new RawText(getBytes(iter.getEntryFile().getPath(),
+						stream, filteredLength));
+			}
+			DirCacheEntry indexEntry = dcIter.getDirCacheEntry();
+			if (indexEntry.getStage() == DirCacheEntry.STAGE_0) {
+				push(null, head);
+				push(null, indexEntry.getObjectId());
+				push(null, inTree);
+			} else {
+				// Create a special candidate using the working tree file as
+				// blob and HEAD and the MERGE_HEADs as parents.
+				HeadCandidate c = new HeadCandidate(getRepository(), resultPath,
+						getHeads(repo, head));
+				c.sourceText = inTree;
+				c.regionList = new Region(0, 0, inTree.size());
+				remaining = inTree.size();
+				push(c);
+			}
+		}
+		return this;
+	}
+
+	private List<RevCommit> getHeads(Repository repo, ObjectId head)
+			throws NoWorkTreeException, IOException {
+		List<ObjectId> mergeIds = repo.readMergeHeads();
+		if (mergeIds == null || mergeIds.isEmpty()) {
+			return Collections.singletonList(revPool.parseCommit(head));
+		}
+		List<RevCommit> heads = new ArrayList<>(mergeIds.size() + 1);
+		heads.add(revPool.parseCommit(head));
+		for (ObjectId id : mergeIds) {
+			heads.add(revPool.parseCommit(id));
+		}
+		return heads;
+	}
+
+	private static byte[] getBytes(String path, InputStream in, long maxLength)
+			throws IOException {
+		if (maxLength > Integer.MAX_VALUE) {
+			throw new IOException(
+					MessageFormat.format(JGitText.get().fileIsTooLarge, path));
+		}
+		int max = (int) maxLength;
+		byte[] buffer = new byte[max];
+		int read = IO.readFully(in, buffer, 0);
+		if (read == max) {
+			return buffer;
+		}
+		byte[] copy = new byte[read];
+		System.arraycopy(buffer, 0, copy, 0, read);
+		return copy;
+	}
+
 	/**
 	 * Push a candidate object onto the generator's traversal stack.
 	 * <p>

org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011, Google Inc.
+ * Copyright (C) 2011, 2019 Google Inc.
  * and other copyright owners as documented in the project's IP log.
  *
  * This program and the accompanying materials are made available
@@ -44,12 +44,14 @@
 package org.eclipse.jgit.blame;
 
 import java.io.IOException;
+import java.util.List;
 
 import org.eclipse.jgit.blame.ReverseWalk.ReverseCommit;
 import org.eclipse.jgit.diff.Edit;
 import org.eclipse.jgit.diff.EditList;
 import org.eclipse.jgit.diff.RawText;
 import org.eclipse.jgit.errors.MissingObjectException;
+import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectLoader;
@@ -391,6 +393,66 @@ class Candidate {
 		}
 	}
 
+	/**
+	 * A {@link Candidate} to blame a working tree file in conflict state.
+	 * <p>
+	 * Contrary to {@link BlobCandidate}, it expects to be given the parent
+	 * commits (typically HEAD and the MERGE_HEADs) and behaves like a merge
+	 * commit during blame. It does <em>not</em> consider a previously pushed
+	 * Candidate as its parent.
+	 * </p>
+	 */
+	static final class HeadCandidate extends Candidate {
+
+		private List<RevCommit> parents;
+
+		HeadCandidate(Repository repo, PathFilter path,
+				List<RevCommit> parents) {
+			super(repo, null, path);
+			this.parents = parents;
+		}
+
+		@Override
+		void beginResult(RevWalk rw) {
+			// Blob candidates have nothing to prepare.
+		}
+
+		@Override
+		int getParentCount() {
+			return parents.size();
+		}
+
+		@Override
+		RevCommit getParent(int idx) {
+			return parents.get(idx);
+		}
+
+		@Override
+		boolean has(RevFlag flag) {
+			return true; // Pretend flag was added; sourceCommit is null.
+		}
+
+		@Override
+		void add(RevFlag flag) {
+			// Do nothing, sourceCommit is null.
+		}
+
+		@Override
+		void remove(RevFlag flag) {
+			// Do nothing, sourceCommit is null.
+		}
+
+		@Override
+		int getTime() {
+			return Integer.MAX_VALUE;
+		}
+
+		@Override
+		PersonIdent getAuthor() {
+			return new PersonIdent(JGitText.get().blameNotCommittedYet, ""); //$NON-NLS-1$
+		}
+	}
+
 	/**
 	 * Candidate loaded from a file source, and not a commit.
 	 * <p>

org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java

@@ -518,6 +518,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String nonCommitToHeads;
 	/***/ public String noPathAttributesFound;
 	/***/ public String noSuchRef;
+	/***/ public String noSuchRefKnown;
 	/***/ public String noSuchSubmodule;
 	/***/ public String notABoolean;
 	/***/ public String notABundle;