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.

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.

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.

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.

In T262944#6467291, @hashar wrote:

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