Page MenuHomePhabricator

Do not expand large files (like package-lock.json) by default when `Expand All` is clicked
Open, Needs TriagePublic

Description

I've noticed that github doesn't expand large files (like package-lock.json) by default, while gerrit does.

For example, see the same commit in github (3fb16c3) and gerrit (618014).

Videos: github, gerrit.

My usual workflow (in gerrit) is to open a patch and click Expand All. It works great, unless there's a package-lock.json file. Then the page becomes unresponsive for a while (while the huge package-lock.json is rendered), and any files under it are not rendered, even after package-lock.json is.

Maybe it makes sense to expand some files, but package-lock.json is not supposed to be edited by humans, so it's very unlikely that you would actually want to review the file.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 15 2020, 3:35 PM

Is it the size of the file or the size of the diff that's a problem? I'm going to assume the size of the diff, so that a one-line change to a huge file is still OK, but a diff that changes every line in a 1000-line file is not.

Good point. I guess I don't want files with a lot of diffs rendered by default. If it's a big file, with just one line changed, that would render fine.

Krinkle added a subscriber: Krinkle.EditedSep 15 2020, 4:23 PM

The GitHub approach would imho be somewhat flawed if applied to current Gerrit as it quite often ends up omitting useful diffs as well, it's a bit too aggressive.

On the other hand, their interface quite different overall and does lend itself to this approach as it still offers a way to "Load the diff" by pressing on the placeholder button.

Placeholder diff:

Something like this doesn't currently exist afaik, but could be submitted as a feature request upstream to Gerrit. They're quite responsive in general and are doing a lot of UX work recently.

Right now when Gerrit doesn't display a diff (like for PNG files), it just refuses with no option to bypass.

The native .gitattributes config for Git in each repo can be used to register additional file patterns as "binary"-like which in practice just means to not include them in git show, git log -p and git grep and is commonly used for source maps and minified/derivative files like this.

Having said that, we actually have this set in gitattributes currently, but an example commit in Gerrit shows that it is still displayed by default just the same. That would be needed to support first as well. I suspect currently Gerrit probably hardcodes its binary file detection instead of checking the currrent repo config.

Krinkle added a comment.EditedSep 15 2020, 4:30 PM

In terms of workflow, I wonder how developers usually review diffs. Personally, I don't use "Expand All", regardless of this issue. It's quite slow to load (despite it trying very hard to be lazy and efficient), and even then its still only a stripped down version of the full diff capabilities. The inline diff is too limited for me to do proper code reviews.

I generally open the first file and then navigate left or right with [ or ]. This seems to load much quicker, doesn't feel buggy, and you scan or skip over any files as needed.

hashar added a subscriber: hashar.Sep 16 2020, 5:21 PM

For the .gitattributes support, I am guessing the logic to determine whether a file is binary comes from jgit so one would have to dig into that. That can probably filed up to Gerrit upstream (respect .gitattributes).

The issue is that EXPAND ALL does what is requested: expands every single diff, much like a local git show will end up with pages and page of diff due to the large diff introduced in the lock file. A workaround is to collapse the file large diff, else fall back to file per file review.

The issue is that EXPAND ALL does what is requested: expands every single diff, much like a local git show […]

The git show output adheres to .gitattributes config. Patterns identified as binary are omitted from the diff in favour of a placeholder saying that it was added, removed, or changed.

mediawiki$ git show 9cf6be92d86a1317366dc8d28e6f98e0c475bb21

diff --git a/resources/lib/ooui/oojs-ui-widgets.js.map.json b/resources/lib/ooui/oojs-ui-widgets.js.map.json
index acf2647f78..3fb2465798 100644
Binary files a/resources/lib/ooui/oojs-ui-widgets.js.map.json and b/resources/lib/ooui/oojs-ui-widgets.js.map.json differ

diff --git a/resources/lib/ooui/oojs-ui-wikimediaui.js b/resources/lib/ooui/oojs-ui-wikimediaui.js
index 03b8798fd2..0e40f34c12 100644
--- a/resources/lib/ooui/oojs-ui-wikimediaui.js
+++ b/resources/lib/ooui/oojs-ui-wikimediaui.js
@@ -1,12 +1,12 @@
 /*!
- * OOUI v0.40.1
+ * OOUI v0.40.3