commit | 9078129f78b7e1c900b9a614c44bd420ee300777 | [log] [tgz] |
---|---|---|
author | Alice Kober-Sotzek <aliceks@google.com> | Fri Nov 29 18:36:42 2019 +0100 |
committer | David Pursehouse <dpursehouse@collab.net> | Tue Jan 21 10:02:05 2020 +0900 |
tree | cf7dfe39764832d2038eade0c2bdd261c8c77dfa | |
parent | a6cf72adc026371bf88bc63fee3a318de24e2284 [diff] |
Switch off intraline diffs for some cases (invalid cache key sharing) This change switches off intraline diffs for COMMIT_MSG, MERGE_LIST, and submodule commit entries. There are some situations in which we use the exactly same IntraLineDiff cache key, though the resulting diff is different: 1) Diff between patch set A and B of change C for COMMIT_MSG. 2) Diff between patch set A and B of change C for MERGE_LIST. 3) Diff between patch set X and Y of change Z for a submodule commit, which updates the submodule repository (on same host) from commit A (= patch set A) to commit B (= patch set B). Depending on which diff is requested first, its result gets stored inside of the IntraLineDiff cache. When the other diffs are requested, they get the result of the first request even though that is a wrong diff for them. What makes the situation worse is that those wrong results from the IntraLineDiff cache also replace the regular (non-intraline) edits/diffs of the whole file. Users can observe this bug either by not being able to open such a wrong diff (due to an internal server error caused by trying to access content beyond its limits due to an edit in a higher line) or by seeing a wrong diff. According to design principles, cache keys should contain all values necessary to produce the cache result. For the IntraLineDiff cache, this principle is violated. That fact is directly apparent by the existence of the IntraLineDiffArgs which are passed in along the IntraLineDiffKey. There have probably been good reasons why those parameters were not included in the key (e.g. edits containing lots of data), which is also the reason why we can't directly include them in the key now. Instead, we'd need to totally rework the cache so that we can use a better cache key. We have ideas for this but it will take quite some effort and hence is not suited as a quick fix. Of course, we could now try to find a parameter we could additionally include in the key to fix the bug we identified. On the other hand, this would mean that we just paper over the real issue. In addition, we would need to be very careful as we don't have any tests for diffs relating to submodules and hardly any for COMMIT_MSG or MERGE_LIST, especially not for intraline diffs. Furthermore, we would need to flush the IntraLineDiff cache, which we typically try to avoid. As quick and easy mitigation, we decide to disable the intraline diffing for COMMIT_MSG, MERGE_LIST, and submodule commits. For COMMIT_MSG and MERGE_LIST, this effectively changes only the behavior when diffing patch sets against each other as otherwise those magic files are considered/shown as full file additions (which don't have intraline diffs). That's also the reason this bug doesn't occur in those situations: The IntralineDiff cache isn't called for them. One might argue that we could simply switch off the intraline diffing for two of the cases and keep it enabled for the remaining one as then only one type of request would use the cache key. As we can't know for sure that there isn't a further case with that same key without extremely deeply looking at the code, it's safer to switch off intraline diffing for all of them. Writing test cases for the error situations is tricky. Due to this, we only add one test which surfaces the issue when run in the intraline mode of the test class. Future test contributions especially for submodule commits would be welcome. Bug: Issue 7969 Bug: Issue 9296 Change-Id: Iaaf3a0b3cd86e80033c4472d0d5662b76daa3509
Gerrit is a code review and project management tool for Git based projects.
Gerrit makes reviews easier by showing changes in a side-by-side display, and allowing inline comments to be added by any reviewer.
Gerrit simplifies Git based project maintainership by permitting any authorized user to submit changes to the master Git repository, rather than requiring all approved changes to be merged in by hand by the project maintainer.
For information about how to install and use Gerrit, refer to the documentation.
Our canonical Git repository is located on googlesource.com. There is a mirror of the repository on Github.
Please report bugs on the issue tracker.
Gerrit is the work of hundreds of contributors. We appreciate your help!
Please read the contribution guidelines.
Note that we do not accept Pull Requests via the Github mirror.
The Developer Mailing list is repo-discuss on Google Groups.
Gerrit is provided under the Apache License 2.0.
Install Bazel and run the following:
git clone --recurse-submodules https://gerrit.googlesource.com/gerrit cd gerrit && bazel build release
The instruction how to configure GerritForge/BinTray repositories is here
On Debian/Ubuntu run:
apt-get update & apt-get install gerrit=<version>-<release>
NOTE: release is a counter that starts with 1 and indicates the number of packages that have been released with the same version of the software.
On CentOS/RedHat run:
yum clean all && yum install gerrit-<version>[-<release>]
On Fedora run:
dnf clean all && dnf install gerrit-<version>[-<release>]
Docker images of Gerrit are available on DockerHub
To run a CentOS 7 based Gerrit image:
docker run -p 8080:8080 gerritforge/gerrit-centos7[:version]
To run a Ubuntu 15.04 based Gerrit image:
docker run -p 8080:8080 gerritforge/gerrit-ubuntu15.04[:version]
NOTE: release is optional. Last released package of the version is installed if the release number is omitted.