From 59f9d206c954b5633f5978723bd0a2e7db31c2e8 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Tue, 6 Aug 2019 18:31:35 +0200 Subject: [PATCH] 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 --- .../tst/org/eclipse/jgit/pgm/BlameTest.java | 35 ++++++ .../src/org/eclipse/jgit/pgm/Blame.java | 28 +---- .../eclipse/jgit/api/BlameCommandTest.java | 36 +++++- .../eclipse/jgit/internal/JGitText.properties | 1 + .../org/eclipse/jgit/api/BlameCommand.java | 72 +---------- .../eclipse/jgit/blame/BlameGenerator.java | 117 +++++++++++++++++- .../src/org/eclipse/jgit/blame/Candidate.java | 64 +++++++++- .../org/eclipse/jgit/internal/JGitText.java | 1 + 8 files changed, 256 insertions(+), 98 deletions(-) diff --git a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/BlameTest.java b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/BlameTest.java index e806872c1..732c54e33 100644 --- a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/BlameTest.java +++ b/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 void testBlameNonExistingInSubdir() throws Exception { 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")); + } } diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Blame.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Blame.java index b67b04c5b..f85d8d49a 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Blame.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Blame.java @@ -50,7 +50,6 @@ 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.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 @@ protected void run() { } 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 @@ && sameCommit(blame.getSourceCommit(line + 1), c)) { } while (++line < end && sameCommit(blame.getSourceCommit(line), c)); } - } catch (NoWorkTreeException | IOException e) { + } catch (NoWorkTreeException | NoHeadException | IOException e) { throw die(e.getMessage(), e); } } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/BlameCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/BlameCommandTest.java index 7e73084e8..4d1375a2f 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/BlameCommandTest.java +++ b/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 @@ 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 void testConflictingMerge2() throws Exception { } } + @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)) { diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index 882b766f4..27d3e6aaa 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/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 diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/BlameCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/BlameCommand.java index 0c765b01c..a69aa70c6 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/BlameCommand.java +++ b/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.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 BlameResult call() throws GitAPIException { 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(); - } - } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java b/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java index 9cec64567..d0aa292df 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java +++ b/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_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.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.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 BlameGenerator push(String description, RawText contents) 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 getHeads(Repository repo, ObjectId head) + throws NoWorkTreeException, IOException { + List mergeIds = repo.readMergeHeads(); + if (mergeIds == null || mergeIds.isEmpty()) { + return Collections.singletonList(revPool.parseCommit(head)); + } + List 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. *

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java b/org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java index 457d1d2ce..3ef494398 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java +++ b/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 @@ public String toString() { } } + /** + * A {@link Candidate} to blame a working tree file in conflict state. + *

+ * 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 not consider a previously pushed + * Candidate as its parent. + *

+ */ + static final class HeadCandidate extends Candidate { + + private List parents; + + HeadCandidate(Repository repo, PathFilter path, + List 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. *

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 65c962946..0cea60fd9 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -518,6 +518,7 @@ public static JGitText get() { /***/ public String nonCommitToHeads; /***/ public String noPathAttributesFound; /***/ public String noSuchRef; + /***/ public String noSuchRefKnown; /***/ public String noSuchSubmodule; /***/ public String notABoolean; /***/ public String notABundle;