BlameServlet: Honor .git-blame-ignore-revs Skip bulk reformatting and other mechanical revisions during blame, attributing lines to the last meaningful change. If .git-blame-ignore-revs exists at the root of the repository in the tree of the revision being blamed, its listed revisions are ignored when computing blame. If the file is missing, too large or cannot be read, blame ignores no revisions instead of failing the request. Both the HTML and the JSON blame views honor the file. BlameCacheImpl supports ignored revisions and includes them in its cache key. Change-Id: I9290cb31e3793e1e72e210b9c841a8508fe92046
diff --git a/java/com/google/gitiles/blame/BlameServlet.java b/java/com/google/gitiles/blame/BlameServlet.java index 179b5d6..a950ccb 100644 --- a/java/com/google/gitiles/blame/BlameServlet.java +++ b/java/com/google/gitiles/blame/BlameServlet.java
@@ -19,6 +19,7 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; import com.google.gitiles.BaseServlet; import com.google.gitiles.BlobSoyData; @@ -32,6 +33,8 @@ import com.google.gitiles.Renderer; import com.google.gitiles.ViewFilter; import com.google.gitiles.blame.cache.BlameCache; +import com.google.gitiles.blame.cache.IgnoreRevFileReader; +import com.google.gitiles.blame.cache.IgnoreRevFileReader.IgnoreRevsFile; import com.google.gitiles.blame.cache.Region; import com.google.gson.GsonBuilder; import com.google.gson.reflect.TypeToken; @@ -177,13 +180,48 @@ lastCommit = currCommit; } - List<Region> regions = cache.get(repo, lastCommit, view.getPathPart()); + List<Region> regions = + cache.get( + repo, + lastCommit, + view.getPathPart(), + getIgnoreIds(rw, currCommit)); if (regions.isEmpty()) { throw new GitilesRequestFailureException(FailureReason.BLAME_REGION_NOT_FOUND); } return new RegionResult(regions, lastCommitBlobId); } + /** + * Returns the revisions listed in {@code .git-blame-ignore-revs}, read from the tree of the + * blamed commit. + */ + private static ImmutableSet<ObjectId> getIgnoreIds(RevWalk rw, RevCommit commit) + throws IOException { + IgnoreRevsFile file; + try { + file = IgnoreRevFileReader.read(rw, commit); + } catch (IOException e) { + log.warn("Cannot read {} at {}", IgnoreRevFileReader.DEFAULT_PATH, commit.name(), e); + return ImmutableSet.of(); + } + if (file.tooLarge()) { + log.warn( + "{} at {} is larger than {} bytes, not ignoring any revisions", + IgnoreRevFileReader.DEFAULT_PATH, + commit.name(), + IgnoreRevFileReader.MAX_SIZE); + return ImmutableSet.of(); + } else if (file.truncated()) { + log.info( + "Ignoring only the first {} revisions listed in {} at {}", + IgnoreRevFileReader.MAX_REVS, + IgnoreRevFileReader.DEFAULT_PATH, + commit.name()); + } + return file.ids(); + } + private static @Nullable ObjectId resolveBlob(GitilesView view, RevWalk rw, ObjectId commitId) throws IOException { try {
diff --git a/javatests/com/google/gitiles/blame/BlameServletTest.java b/javatests/com/google/gitiles/blame/BlameServletTest.java index d05b082..58a872c 100644 --- a/javatests/com/google/gitiles/blame/BlameServletTest.java +++ b/javatests/com/google/gitiles/blame/BlameServletTest.java
@@ -31,6 +31,7 @@ public class BlameServletTest extends ServletTest { private static final String NAME = "J. Author"; private static final String EMAIL = "jauthor@example.com"; + private static final String IGNORE_REVS_FILE = ".git-blame-ignore-revs"; private static class RegionJsonData { int start; @@ -73,6 +74,113 @@ assertThat(r2.author.time).isEqualTo(c2Time); } + @Test + public void blameJsonWithIgnoreRevsFile() throws Exception { + RevCommit c1 = repo.update("master", repo.commit().add("foo", "foo\nbar\n")); + RevCommit c2 = + repo.update("master", repo.commit().tick(10).parent(c1).add("foo", "Foo\nBar\n")); + RevCommit c3 = + repo.update( + "master", + repo.commit() + .tick(10) + .parent(c2) + .add(IGNORE_REVS_FILE, "# Reformat foo\n" + c2.name() + "\n")); + + List<RegionJsonData> regions = + getBlameJson("/repo/+blame/" + c3.name() + "/foo").get("regions"); + assertThat(regions).hasSize(1); + assertThat(regions.get(0).start).isEqualTo(1); + assertThat(regions.get(0).count).isEqualTo(2); + assertThat(regions.get(0).commit).isEqualTo(c1.name()); + } + + @Test + public void blameJsonWithMissingIgnoreRevsFile() throws Exception { + RevCommit c1 = repo.update("master", repo.commit().add("foo", "foo\nbar\n")); + RevCommit c2 = + repo.update("master", repo.commit().tick(10).parent(c1).add("foo", "Foo\nBar\n")); + + List<RegionJsonData> regions = + getBlameJson("/repo/+blame/" + c2.name() + "/foo").get("regions"); + assertThat(regions).hasSize(1); + assertThat(regions.get(0).commit).isEqualTo(c2.name()); + } + + @Test + public void blameJsonWhenCommitPredatesIgnoreRevsFile() throws Exception { + RevCommit c1 = repo.update("master", repo.commit().add("foo", "foo\nbar\n")); + RevCommit c2 = + repo.update("master", repo.commit().tick(10).parent(c1).add("foo", "Foo\nBar\n")); + repo.update( + "master", + repo.commit() + .tick(10) + .parent(c2) + .add(IGNORE_REVS_FILE, "# Reformat foo\n" + c2.name() + "\n")); + + // Blame at c2, which predates the commit that added IGNORE_REVS_FILE to master. + // The file is read from c2, so c2 is not ignored. + List<RegionJsonData> regions = + getBlameJson("/repo/+blame/" + c2.name() + "/foo").get("regions"); + assertThat(regions).hasSize(1); + assertThat(regions.get(0).commit).isEqualTo(c2.name()); + } + + @Test + public void blameJsonByBranchNameWithIgnoreRevsFile() throws Exception { + RevCommit c1 = repo.update("master", repo.commit().add("foo", "foo\nbar\n")); + RevCommit c2 = + repo.update("master", repo.commit().tick(10).parent(c1).add("foo", "Foo\nBar\n")); + repo.update( + "master", + repo.commit() + .tick(10) + .parent(c2) + .add(IGNORE_REVS_FILE, "# Reformat foo\n" + c2.name() + "\n")); + + // Blame by branch name, which reads the file from the branch tip commit. + List<RegionJsonData> regions = getBlameJson("/repo/+blame/master/foo").get("regions"); + assertThat(regions).hasSize(1); + assertThat(regions.get(0).start).isEqualTo(1); + assertThat(regions.get(0).count).isEqualTo(2); + assertThat(regions.get(0).commit).isEqualTo(c1.name()); + } + + @Test + public void blameJsonWithOversizedIgnoreRevsFile() throws Exception { + RevCommit c1 = repo.update("master", repo.commit().add("foo", "foo\nbar\n")); + RevCommit c2 = + repo.update("master", repo.commit().tick(10).parent(c1).add("foo", "Foo\nBar\n")); + String oversized = (c2.name() + "\n").repeat(30_000); + repo.update( + "master", + repo.commit() + .tick(10) + .parent(c2) + .add(IGNORE_REVS_FILE, oversized)); + + // Oversized ignore file is skipped, so c2 is not ignored. + List<RegionJsonData> regions = + getBlameJson("/repo/+blame/master/foo").get("regions"); + assertThat(regions).hasSize(1); + assertThat(regions.get(0).commit).isEqualTo(c2.name()); + } + + @Test + public void blameHtmlWithIgnoreRevsFile() throws Exception { + RevCommit c1 = repo.update("master", repo.commit().add("foo", "foo\nbar\n")); + RevCommit c2 = + repo.update("master", repo.commit().tick(10).parent(c1).add("foo", "Foo\nBar\n")); + RevCommit c3 = + repo.update( + "master", repo.commit().tick(10).parent(c2).add(IGNORE_REVS_FILE, c2.name() + "\n")); + + String html = buildHtml("/repo/+blame/" + c3.name() + "/foo", false); + assertThat(html).contains(c1.name()); + assertThat(html).doesNotContain(c2.name()); + } + private Map<String, List<RegionJsonData>> getBlameJson(String path) throws Exception { return buildJson(new TypeToken<Map<String, List<RegionJsonData>>>() {}, path); }