From 640f984b54a76947bf192529a17647648f54bdb2 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 8 Oct 2015 15:53:20 +0200 Subject: [PATCH 1/2] FileInfo: Include size delta Reviewers are interested in the size of a change that was done to a file. This is why for non-binary files the number of inserted/deleted lines is shown. For binary files there is no indication of how big the change was. With this change we compute for the file size delta in bytes and return it in the FileInfo. The client can use this information to show the file size increase/decrease in the file list (not done in this change). Please note that a field was added to PatchListEntry and hence the diff cache must be flushed once. Change-Id: I0252387099c724251b71480c36b52b7f8d46e713 Signed-off-by: Edwin Kempin --- Documentation/rest-api-changes.txt | 29 ++++++++---- .../gerrit/extensions/common/FileInfo.java | 1 + .../gerrit/server/change/FileInfoJson.java | 2 + .../gerrit/server/patch/PatchListEntry.java | 46 +++++++++++-------- .../gerrit/server/patch/PatchListLoader.java | 41 +++++++++++++---- 5 files changed, 83 insertions(+), 36 deletions(-) diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index cbd748c533..eb775c0d41 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -405,29 +405,36 @@ default. Optional fields are: }, "files": { "gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeCache.java": { - "lines_deleted": 8 + "lines_deleted": 8, + "size_delta": -412 }, "gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeDetailCache.java": { - "lines_inserted": 1 + "lines_inserted": 1, + "size_delta": 23 }, "gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java": { "lines_inserted": 11, - "lines_deleted": 19 + "lines_deleted": 19, + "size_delta": -298 }, "gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeTable.java": { "lines_inserted": 23, - "lines_deleted": 20 + "lines_deleted": 20, + "size_delta": 132 }, "gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/StarCache.java": { "status": "D", - "lines_deleted": 139 + "lines_deleted": 139, + "size_delta": -5512 }, "gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/StarredChanges.java": { "status": "A", - "lines_inserted": 204 + "lines_inserted": 204, + "size_delta": 8345 }, "gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/Screen.java": { - "lines_deleted": 9 + "lines_deleted": 9, + "size_delta": -343 } } } @@ -3254,11 +3261,13 @@ sorted by file path. { "/COMMIT_MSG": { "status": "A", - "lines_inserted": 7 + "lines_inserted": 7, + "size_delta": 551 }, "gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java": { "lines_inserted": 5, - "lines_deleted": 3 + "lines_deleted": 3, + "size_delta": 98 } } ---- @@ -4199,6 +4208,8 @@ Not set for binary files or if no lines were inserted. |`lines_deleted` |optional| Number of deleted lines. + Not set for binary files or if no lines were deleted. +|`size_delta` || +Number of bytes by which the file size increased/decreased. |============================= [[fix-input]] diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/FileInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/FileInfo.java index 58f549466b..00d0c183f0 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/FileInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/FileInfo.java @@ -20,4 +20,5 @@ public class FileInfo { public String oldPath; public Integer linesInserted; public Integer linesDeleted; + public long sizeDelta; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileInfoJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileInfoJson.java index 9ddb0ed04a..84f8a042e3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileInfoJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileInfoJson.java @@ -63,6 +63,7 @@ public class FileInfoJson { d.status = e.getChangeType() != Patch.ChangeType.MODIFIED ? e.getChangeType().getCode() : null; d.oldPath = e.getOldName(); + d.sizeDelta = e.getSizeDelta(); if (e.getPatchType() == Patch.PatchType.BINARY) { d.binary = true; } else { @@ -76,6 +77,7 @@ public class FileInfoJson { // when the file was rewritten and too little content survived. Write // a single record with data from both sides. d.status = Patch.ChangeType.REWRITE.getCode(); + d.sizeDelta = o.sizeDelta; if (o.binary != null && o.binary) { d.binary = true; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListEntry.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListEntry.java index e7c56be7d8..d0069ecfd4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListEntry.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListEntry.java @@ -18,10 +18,12 @@ import static com.google.gerrit.server.ioutil.BasicSerialization.readBytes; import static com.google.gerrit.server.ioutil.BasicSerialization.readEnum; import static com.google.gerrit.server.ioutil.BasicSerialization.readString; import static com.google.gerrit.server.ioutil.BasicSerialization.readVarInt32; +import static com.google.gerrit.server.ioutil.BasicSerialization.readFixInt64; import static com.google.gerrit.server.ioutil.BasicSerialization.writeBytes; import static com.google.gerrit.server.ioutil.BasicSerialization.writeEnum; import static com.google.gerrit.server.ioutil.BasicSerialization.writeString; import static com.google.gerrit.server.ioutil.BasicSerialization.writeVarInt32; +import static com.google.gerrit.server.ioutil.BasicSerialization.writeFixInt64; import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.Patch.ChangeType; @@ -49,7 +51,7 @@ public class PatchListEntry { static PatchListEntry empty(final String fileName) { return new PatchListEntry(ChangeType.MODIFIED, PatchType.UNIFIED, null, - fileName, EMPTY_HEADER, Collections. emptyList(), 0, 0); + fileName, EMPTY_HEADER, Collections. emptyList(), 0, 0, 0); } private final ChangeType changeType; @@ -60,8 +62,9 @@ public class PatchListEntry { private final List edits; private final int insertions; private final int deletions; + private final long sizeDelta; - PatchListEntry(final FileHeader hdr, List editList) { + PatchListEntry(FileHeader hdr, List editList, long sizeDelta) { changeType = toChangeType(hdr); patchType = toPatchType(hdr); @@ -106,12 +109,12 @@ public class PatchListEntry { } insertions = ins; deletions = del; + this.sizeDelta = sizeDelta; } - private PatchListEntry(final ChangeType changeType, - final PatchType patchType, final String oldName, final String newName, - final byte[] header, final List edits, final int insertions, - final int deletions) { + private PatchListEntry(ChangeType changeType, PatchType patchType, + String oldName, String newName, byte[] header, List edits, + int insertions, int deletions, long sizeDelta) { this.changeType = changeType; this.patchType = patchType; this.oldName = oldName; @@ -120,6 +123,7 @@ public class PatchListEntry { this.edits = edits; this.insertions = insertions; this.deletions = deletions; + this.sizeDelta = sizeDelta; } int weigh() { @@ -166,6 +170,10 @@ public class PatchListEntry { return deletions; } + public long getSizeDelta() { + return sizeDelta; + } + public List getHeaderLines() { final IntList m = RawParseUtils.lineMap(header, 0, header.length); final List headerLines = new ArrayList<>(m.size() - 1); @@ -190,7 +198,7 @@ public class PatchListEntry { return p; } - void writeTo(final OutputStream out) throws IOException { + void writeTo(OutputStream out) throws IOException { writeEnum(out, changeType); writeEnum(out, patchType); writeString(out, oldName); @@ -198,6 +206,7 @@ public class PatchListEntry { writeBytes(out, header); writeVarInt32(out, insertions); writeVarInt32(out, deletions); + writeFixInt64(out, sizeDelta); writeVarInt32(out, edits.size()); for (final Edit e : edits) { @@ -208,17 +217,18 @@ public class PatchListEntry { } } - static PatchListEntry readFrom(final InputStream in) throws IOException { - final ChangeType changeType = readEnum(in, ChangeType.values()); - final PatchType patchType = readEnum(in, PatchType.values()); - final String oldName = readString(in); - final String newName = readString(in); - final byte[] hdr = readBytes(in); - final int ins = readVarInt32(in); - final int del = readVarInt32(in); + static PatchListEntry readFrom(InputStream in) throws IOException { + ChangeType changeType = readEnum(in, ChangeType.values()); + PatchType patchType = readEnum(in, PatchType.values()); + String oldName = readString(in); + String newName = readString(in); + byte[] hdr = readBytes(in); + int ins = readVarInt32(in); + int del = readVarInt32(in); + long sizeDelta = readFixInt64(in); - final int editCount = readVarInt32(in); - final Edit[] editArray = new Edit[editCount]; + int editCount = readVarInt32(in); + Edit[] editArray = new Edit[editCount]; for (int i = 0; i < editCount; i++) { int beginA = readVarInt32(in); int endA = readVarInt32(in); @@ -228,7 +238,7 @@ public class PatchListEntry { } return new PatchListEntry(changeType, patchType, oldName, newName, hdr, - toList(editArray), ins, del); + toList(editArray), ins, del, sizeDelta); } private static List toList(Edit[] l) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java index 5cc8b87585..4c84a189a6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java @@ -15,6 +15,8 @@ package com.google.gerrit.server.patch; +import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; + import com.google.common.base.Function; import com.google.common.base.Throwables; import com.google.common.collect.FluentIterable; @@ -59,6 +61,7 @@ import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevObject; import org.eclipse.jgit.revwalk.RevTree; import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.treewalk.TreeWalk; import org.eclipse.jgit.util.TemporaryBuffer; import org.eclipse.jgit.util.io.DisabledOutputStream; import org.slf4j.Logger; @@ -194,8 +197,12 @@ public class PatchListLoader implements Callable { DiffEntry diffEntry = diffEntries.get(i); if (paths == null || paths.contains(diffEntry.getNewPath()) || paths.contains(diffEntry.getOldPath())) { + FileHeader fh = toFileHeader(key, df, diffEntry); - entries.add(newEntry(aTree, fh)); + long sizeDelta = + getFileSize(repo, reader, diffEntry.getNewPath(), bTree) + - getFileSize(repo, reader, diffEntry.getOldPath(), aTree); + entries.add(newEntry(aTree, fh, sizeDelta)); } } return new PatchList(a, b, againstParent, @@ -203,6 +210,15 @@ public class PatchListLoader implements Callable { } } + private static long getFileSize(Repository repo, ObjectReader reader, + String path, RevTree t) throws IOException { + try (TreeWalk tw = TreeWalk.forPath(reader, path, t)) { + return tw != null + ? repo.open(tw.getObjectId(0), OBJ_BLOB).getSize() + : 0; + } + } + private FileHeader toFileHeader(PatchListKey key, final DiffFormatter diffFormatter, final DiffEntry diffEntry) throws IOException { @@ -267,32 +283,39 @@ public class PatchListLoader implements Callable { Text bText = Text.forCommit(reader, bCommit); byte[] rawHdr = hdr.toString().getBytes("UTF-8"); - RawText aRawText = new RawText(aText.getContent()); - RawText bRawText = new RawText(bText.getContent()); + byte[] aContent = aText.getContent(); + byte[] bContent = bText.getContent(); + long sizeDelta = bContent.length - aContent.length; + RawText aRawText = new RawText(aContent); + RawText bRawText = new RawText(bContent); EditList edits = new HistogramDiff().diff(cmp, aRawText, bRawText); FileHeader fh = new FileHeader(rawHdr, edits, PatchType.UNIFIED); - return new PatchListEntry(fh, edits); + return new PatchListEntry(fh, edits, sizeDelta); } - private PatchListEntry newEntry(RevTree aTree, FileHeader fileHeader) { + private PatchListEntry newEntry(RevTree aTree, FileHeader fileHeader, + long sizeDelta) { final FileMode oldMode = fileHeader.getOldMode(); final FileMode newMode = fileHeader.getNewMode(); if (oldMode == FileMode.GITLINK || newMode == FileMode.GITLINK) { - return new PatchListEntry(fileHeader, Collections. emptyList()); + return new PatchListEntry(fileHeader, Collections. emptyList(), + sizeDelta); } if (aTree == null // want combined diff || fileHeader.getPatchType() != PatchType.UNIFIED || fileHeader.getHunks().isEmpty()) { - return new PatchListEntry(fileHeader, Collections. emptyList()); + return new PatchListEntry(fileHeader, Collections. emptyList(), + sizeDelta); } List edits = fileHeader.toEditList(); if (edits.isEmpty()) { - return new PatchListEntry(fileHeader, Collections. emptyList()); + return new PatchListEntry(fileHeader, Collections. emptyList(), + sizeDelta); } else { - return new PatchListEntry(fileHeader, edits); + return new PatchListEntry(fileHeader, edits, sizeDelta); } } From 3b38262eae5b2fb3426b48d444d007bd67884425 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 9 Oct 2015 14:17:21 +0200 Subject: [PATCH 2/2] ChangeScreen: Show file size increase/decrease for binary files Reviewers are interested in the size of a change that was done to a file. This is why for non-binary files the number of inserted/deleted lines is shown. So far for binary files there was no indication of how big the change was, but now the file size increase/decrease is shown. Change-Id: Ie595754af1536ce430206c2553a7656f526d7769 Signed-off-by: Edwin Kempin --- .../google/gerrit/client/info/FileInfo.java | 9 ++++ .../com/google/gerrit/client/FormatUtil.java | 16 ++++++ .../gerrit/client/change/FileTable.java | 3 ++ .../google/gerrit/client/FormatUtilTest.java | 49 +++++++++++++++++++ 4 files changed, 77 insertions(+) create mode 100644 gerrit-gwtui/src/test/java/com/google/gerrit/client/FormatUtilTest.java diff --git a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/FileInfo.java b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/FileInfo.java index b21078ec1d..d95f9efb33 100644 --- a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/FileInfo.java +++ b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/FileInfo.java @@ -30,6 +30,15 @@ public class FileInfo extends JavaScriptObject { public final native boolean binary() /*-{ return this.binary || false; }-*/; public final native String status() /*-{ return this.status; }-*/; + + // JSNI methods cannot have 'long' as a parameter type or a return type and + // it's suggested to use double in this case: + // http://www.gwtproject.org/doc/latest/DevGuideCodingBasicsJSNI.html#important + public final long sizeDelta() { + return (long)_sizeDelta(); + } + private final native double _sizeDelta() /*-{ return this.size_delta || 0; }-*/; + public final native int _row() /*-{ return this._row }-*/; public final native void _row(int r) /*-{ this._row = r }-*/; diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/FormatUtil.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/FormatUtil.java index 80aa9cc643..08fe75d62f 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/FormatUtil.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/FormatUtil.java @@ -17,6 +17,7 @@ package com.google.gerrit.client; import com.google.gerrit.client.info.AccountInfo; import com.google.gerrit.client.info.AccountPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; +import com.google.gwt.i18n.client.NumberFormat; import java.util.Date; @@ -107,4 +108,19 @@ public class FormatUtil { private static AccountFormatter createAccountFormatter() { return new AccountFormatter(Gerrit.info().user().anonymousCowardName()); } + + public static String formatBytes(long bytes) { + if (bytes == 0) { + return "+/- 0 B"; + } + + if (Math.abs(bytes) < 1024) { + return (bytes > 0 ? "+" : "") + bytes + " B"; + } + + int exp = (int) (Math.log(Math.abs(bytes)) / Math.log(1024)); + return (bytes > 0 ? "+" : "") + + NumberFormat.getFormat("#.0").format(bytes / Math.pow(1024, exp)) + + " " + "KMGTPE".charAt(exp - 1) + "iB"; + } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java index ad61446bdb..932cda48a3 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java @@ -15,6 +15,7 @@ package com.google.gerrit.client.change; import com.google.gerrit.client.Dispatcher; +import com.google.gerrit.client.FormatUtil; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.VoidResult; import com.google.gerrit.client.changes.ChangeApi; @@ -750,6 +751,8 @@ public class FileTable extends FlowPanel { .append(info.linesDeleted()); } } + } else if (info.binary()) { + sb.append(FormatUtil.formatBytes(info.sizeDelta())); } sb.closeTd(); } diff --git a/gerrit-gwtui/src/test/java/com/google/gerrit/client/FormatUtilTest.java b/gerrit-gwtui/src/test/java/com/google/gerrit/client/FormatUtilTest.java new file mode 100644 index 0000000000..6ea09fd55f --- /dev/null +++ b/gerrit-gwtui/src/test/java/com/google/gerrit/client/FormatUtilTest.java @@ -0,0 +1,49 @@ +// Copyright (C) 2015 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.client; + +import static org.junit.Assert.assertEquals; + +import com.googlecode.gwt.test.GwtModule; +import com.googlecode.gwt.test.GwtTest; + +import static com.google.gerrit.client.FormatUtil.formatBytes; + +import org.junit.Ignore; +import org.junit.Test; + +@GwtModule("com.google.gerrit.GerritGwtUI") +@Ignore +public class FormatUtilTest extends GwtTest { + @Test + public void testFormatBytes() { + assertEquals("+/- 0 B", formatBytes(0)); + assertEquals("+27 B", formatBytes(27)); + assertEquals("+999 B", formatBytes(999)); + assertEquals("+1000 B", formatBytes(1000)); + assertEquals("+1023 B", formatBytes(1023)); + assertEquals("+1.0 KiB", formatBytes(1024)); + assertEquals("+1.7 KiB", formatBytes(1728)); + assertEquals("+108.0 KiB", formatBytes(110592)); + assertEquals("+6.8 MiB", formatBytes(7077888)); + assertEquals("+432.0 MiB", formatBytes(452984832)); + assertEquals("+27.0 GiB", formatBytes(28991029248L)); + assertEquals("+1.7 TiB", formatBytes(1855425871872L)); + assertEquals("+8.0 EiB", formatBytes(9223372036854775807L)); + + assertEquals("-27 B", formatBytes(-27)); + assertEquals("-1.7 MiB", formatBytes(-1728)); + } +}