)]}'
{
  "commit": "9078129f78b7e1c900b9a614c44bd420ee300777",
  "tree": "cf7dfe39764832d2038eade0c2bdd261c8c77dfa",
  "parents": [
    "a6cf72adc026371bf88bc63fee3a318de24e2284"
  ],
  "author": {
    "name": "Alice Kober-Sotzek",
    "email": "aliceks@google.com",
    "time": "Fri Nov 29 18:36:42 2019 +0100"
  },
  "committer": {
    "name": "David Pursehouse",
    "email": "dpursehouse@collab.net",
    "time": "Tue Jan 21 10:02:05 2020 +0900"
  },
  "message": "Switch off intraline diffs for some cases (invalid cache key sharing)\n\nThis change switches off intraline diffs for COMMIT_MSG, MERGE_LIST, and\nsubmodule commit entries.\n\nThere are some situations in which we use the exactly same IntraLineDiff\ncache key, though the resulting diff is different:\n1) Diff between patch set A and B of change C for COMMIT_MSG.\n2) Diff between patch set A and B of change C for MERGE_LIST.\n3) Diff between patch set X and Y of change Z for a submodule commit,\nwhich updates the submodule repository (on same host) from commit A\n(\u003d patch set A) to commit B (\u003d patch set B).\n\nDepending on which diff is requested first, its result gets stored\ninside of the IntraLineDiff cache. When the other diffs are requested,\nthey get the result of the first request even though that is a wrong\ndiff for them. What makes the situation worse is that those wrong\nresults from the IntraLineDiff cache also replace the regular\n(non-intraline) edits/diffs of the whole file.\n\nUsers can observe this bug either by not being able to open such a wrong\ndiff (due to an internal server error caused by trying to access content\nbeyond its limits due to an edit in a higher line) or by seeing a wrong\ndiff.\n\nAccording to design principles, cache keys should contain all values\nnecessary to produce the cache result. For the IntraLineDiff cache,\nthis principle is violated. That fact is directly apparent by the\nexistence of the IntraLineDiffArgs which are passed in along the\nIntraLineDiffKey. There have probably been good reasons why those\nparameters were not included in the key (e.g. edits containing lots\nof data), which is also the reason why we can\u0027t directly include them\nin the key now. Instead, we\u0027d need to totally rework the cache so that\nwe can use a better cache key. We have ideas for this but it will take\nquite some effort and hence is not suited as a quick fix.\n\nOf course, we could now try to find a parameter we could additionally\ninclude in the key to fix the bug we identified. On the other hand,\nthis would mean that we just paper over the real issue. In addition,\nwe would need to be very careful as we don\u0027t have any tests for diffs\nrelating to submodules and hardly any for COMMIT_MSG or MERGE_LIST,\nespecially not for intraline diffs. Furthermore, we would need to\nflush the IntraLineDiff cache, which we typically try to avoid.\n\nAs quick and easy mitigation, we decide to disable the intraline diffing\nfor COMMIT_MSG, MERGE_LIST, and submodule commits. For COMMIT_MSG and\nMERGE_LIST, this effectively changes only the behavior when diffing\npatch sets against each other as otherwise those magic files are\nconsidered/shown as full file additions (which don\u0027t have intraline\ndiffs). That\u0027s also the reason this bug doesn\u0027t occur in those\nsituations: The IntralineDiff cache isn\u0027t called for them.\n\nOne might argue that we could simply switch off the intraline diffing\nfor two of the cases and keep it enabled for the remaining one as then\nonly one type of request would use the cache key. As we can\u0027t know for\nsure that there isn\u0027t a further case with that same key without\nextremely deeply looking at the code, it\u0027s safer to switch off intraline\ndiffing for all of them.\n\nWriting test cases for the error situations is tricky. Due to this,\nwe only add one test which surfaces the issue when run in the intraline\nmode of the test class. Future test contributions especially for\nsubmodule commits would be welcome.\n\nBug: Issue 7969\nBug: Issue 9296\nChange-Id: Iaaf3a0b3cd86e80033c4472d0d5662b76daa3509\n",
  "tree_diff": [
    {
      "type": "modify",
      "old_id": "d0b1b66cc6874feb75990c20ee1402d59012ee0d",
      "old_mode": 33188,
      "old_path": "java/com/google/gerrit/server/patch/PatchScriptBuilder.java",
      "new_id": "c2caa01b752c58f2e322abc127ebfe4230208ec2",
      "new_mode": 33188,
      "new_path": "java/com/google/gerrit/server/patch/PatchScriptBuilder.java"
    },
    {
      "type": "modify",
      "old_id": "c85e8e4fa63b67fe00f506818796f7abce7fef40",
      "old_mode": 33188,
      "old_path": "javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java",
      "new_id": "c27d637edf8aacd9f75799a779d5470226a82b91",
      "new_mode": 33188,
      "new_path": "javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java"
    }
  ]
}