Comments posted on a change aren't displayed in Gerrit UI [318079520]

Verified

Bug

Status Update

No update yet.

Description

ma...@gmail.com

created issue #1

Dec 30, 2023 01:34PM

GerritHub/GitHub username: msohn

GerritHub/GitHub projects names: eclipse-jgit/jgit

Full URL exhibiting the problem:

https://eclipse.gerrithub.io/c/eclipse-jgit/jgit/+/194140

Expected behavior: I posted 3 comments on this change and they should be displayed

Observed behavior:

3 empty comments are shown

Timestamp when the error occurred (include timezone): 2023-12-30 13:59 CET, 14:02 CET, 14:08 CET

Has this worked before? If yes, when? This always worked.

If applicable: screenshot of erroneous UI element:

https://imgur.com/a/vQa4yHU

If applicable: JavaScript console log (chrome: F12 > Console):

```
initResin
gr-app.js:24586 Runtime Info
gr-app.js:24586 Gerrit UI (PolyGerrit)
gr-app.js:24586 Gerrit Server Version: 3.9.1-4-gb871735b5a
5Failed to decode downloaded font: <URL>
194140:1 Failed to decode downloaded font:

https://eclipse.gerrithub.io/c/eclipse-jgit/jgit/fonts/roboto/Roboto-Regular.ttf
194140:1 Failed to decode downloaded font:

https://eclipse.gerrithub.io/c/eclipse-jgit/jgit/fonts/roboto/Roboto-Medium.ttf
194140:1 Failed to decode downloaded font:

https://eclipse.gerrithub.io/c/eclipse-jgit/jgit/fonts/robotomono/RobotoMono-Regular.ttf
194140:1 Failed to decode downloaded font:

https://eclipse.gerrithub.io/c/eclipse-jgit/jgit/fonts/roboto/Roboto-Italic.ttf
5OTS parsing error: invalid sfntVersion: 1008813135
194140:1 OTS parsing error: invalid sfntVersion: 1008813135
194140:1 OTS parsing error: invalid sfntVersion: 1008813135
194140:1 OTS parsing error: invalid sfntVersion: 1008813135
194140:1 OTS parsing error: invalid sfntVersion: 1008813135
194140:1 OTS parsing error: invalid sfntVersion: 1008813135
gr-app.js:24586

GET

https://eclipse.gerrithub.io/accounts/0/detail 404 (Not Found)
_fetchWithXsrfToken @ gr-app.js:24586
fetch @ gr-app.js:24586
(anonymous) @ gr-app.js:4121
schedule @ gr-app.js:24586
(anonymous) @ gr-app.js:24586
next @ gr-app.js:24586
(anonymous) @ gr-app.js:24586
schedule @ gr-app.js:24586
schedule @ gr-app.js:4121
fetch @ gr-app.js:4121
fetchRawJSON @ gr-app.js:4121
fetchJSON @ gr-app.js:4121
fetchCacheURL @ gr-app.js:4121
_fetchSharedCacheURL @ gr-app.js:24586
getAccountDetails @ gr-app.js:24586
getAccount @ gr-app.js:4566
fillDetails @ gr-app.js:4566
updated @ gr-app.js:4649
_$AE @ gr-app.js:1747
performUpdate @ gr-app.js:1747
scheduleUpdate @ gr-app.js:1747
_$EP @ gr-app.js:1747
await in _$EP (async)
requestUpdate @ gr-app.js:1747
_$Ev @ gr-app.js:1747
uh @ gr-app.js:1747
ep @ gr-app.js:1747
yS @ gr-app.js:4697
u @ gr-app.js:1747
g @ gr-app.js:1747
_$AI @ gr-app.js:1747
T @ gr-app.js:1747
_$AI @ gr-app.js:1747
p @ gr-app.js:1747
g @ gr-app.js:1747
_$AI @ gr-app.js:1747
p @ gr-app.js:1747
g @ gr-app.js:1747
_$AI @ gr-app.js:1747
p @ gr-app.js:1747
g @ gr-app.js:1747
_$AI @ gr-app.js:1747
Qh @ gr-app.js:1747
update @ gr-app.js:1747
performUpdate @ gr-app.js:1747
scheduleUpdate @ gr-app.js:1747
_$EP @ gr-app.js:1747
await in _$EP (async)
requestUpdate @ gr-app.js:1747
set @ gr-app.js:1747
j @ gr-app.js:1747
_$AI @ gr-app.js:1747
p @ gr-app.js:1747
g @ gr-app.js:1747
_$AI @ gr-app.js:1747
p @ gr-app.js:1747
g @ gr-app.js:1747
_$AI @ gr-app.js:1747
p @ gr-app.js:1747
g @ gr-app.js:1747
_$AI @ gr-app.js:1747
p @ gr-app.js:1747
g @ gr-app.js:1747
_$AI @ gr-app.js:1747
Qh @ gr-app.js:1747
update @ gr-app.js:1747
performUpdate @ gr-app.js:1747
scheduleUpdate @ gr-app.js:1747
_$EP @ gr-app.js:1747
await in _$EP (async)
requestUpdate @ gr-app.js:1747
set @ gr-app.js:1747
(anonymous) @ gr-app.js:20808
update @ gr-app.js:2459
(anonymous) @ gr-app.js:2459
t.__tryOrUnsub @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t.next @ gr-app.js:2459
t.nextInfiniteTimeWindow @ gr-app.js:2459
next @ gr-app.js:2459
t.__tryOrUnsub @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t.next @ gr-app.js:2459
t.nextInfiniteTimeWindow @ gr-app.js:2459
next @ gr-app.js:2459
t.__tryOrUnsub @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t.next @ gr-app.js:2459
t.nextInfiniteTimeWindow @ gr-app.js:2459
next @ gr-app.js:2459
t.__tryOrUnsub @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t.next @ gr-app.js:2459
t.next @ gr-app.js:2459
setState @ gr-app.js:2459
modifyState @ gr-app.js:10991
reloadComments @ gr-app.js:10991
await in reloadComments (async)
reloadAllComments @ gr-app.js:10991
(anonymous) @ gr-app.js:10991
t.__tryOrUnsub @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t._subscribe @ gr-app.js:2459
e._trySubscribe @ gr-app.js:2459
t._trySubscribe @ gr-app.js:2459
e.subscribe @ gr-app.js:2459
(anonymous) @ gr-app.js:2459
e.subscribe @ gr-app.js:2459
aT @ gr-app.js:10991
(anonymous) @ gr-app.js:24586
(anonymous) @ gr-app.js:24586
get @ gr-app.js:2459
n @ gr-app.js:2459
Lf.showFindingsTab @ gr-app.js:20808
hostConnected @ gr-app.js:2459
(anonymous) @ gr-app.js:1747
connectedCallback @ gr-app.js:1747
connectedCallback @ gr-app.js:1747
connectedCallback @ gr-app.js:20808
k @ gr-app.js:1747
$ @ gr-app.js:1747
g @ gr-app.js:1747
_$AI @ gr-app.js:1747
p @ gr-app.js:1747
g @ gr-app.js:1747
_$AI @ gr-app.js:1747
Qh @ gr-app.js:1747
update @ gr-app.js:1747
performUpdate @ gr-app.js:1747
scheduleUpdate @ gr-app.js:1747
_$EP @ gr-app.js:1747
await in _$EP (async)
requestUpdate @ gr-app.js:1747
set @ gr-app.js:1747
(anonymous) @ gr-app.js:24436
update @ gr-app.js:2459
(anonymous) @ gr-app.js:2459
t.__tryOrUnsub @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t.next @ gr-app.js:2459
t.nextInfiniteTimeWindow @ gr-app.js:2459
next @ gr-app.js:2459
t.__tryOrUnsub @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t._next @ gr-app.js:2459
t.next @ gr-app.js:2459
t.next @ gr-app.js:2459
t.next @ gr-app.js:2459
setState @ gr-app.js:2459
setState @ gr-app.js:24364
handleChangeRoute @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
Promise.then (async)
(anonymous) @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
loadUserMiddleware @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
(anonymous) @ gr-app.js:24364
r @ gr-app.js:24364
dispatch @ gr-app.js:24364
replace @ gr-app.js:24364
start @ gr-app.js:24364
startRouter @ gr-app.js:24364
start @ gr-app.js:24364
connectedCallback @ gr-app.js:24436
k @ gr-app.js:1747
$ @ gr-app.js:1747
g @ gr-app.js:1747
_$AI @ gr-app.js:1747
Qh @ gr-app.js:1747
update @ gr-app.js:1747
performUpdate @ gr-app.js:1747
scheduleUpdate @ gr-app.js:1747
_$EP @ gr-app.js:1747
await in _$EP (async)
requestUpdate @ gr-app.js:1747
_$Ev @ gr-app.js:1747
uh @ gr-app.js:1747
ep @ gr-app.js:1747
JU @ gr-app.js:24586
(anonymous) @ gr-app.js:1918
XU @ gr-app.js:24586
(anonymous) @ gr-app.js:24586
syntax-worker.js:1 Deprecated as of 10.7.0. highlight(lang, code, ...args) has been deprecated.
syntax-worker.js:1 Deprecated as of 10.7.0. Please use highlight(code, options) instead.

https://github.com/highlightjs/highlight.js/issues/2277
194140:1 The resource

https://eclipse.gerrithub.io/changes/?q=change:194140 was preloaded using link preload but not used within a few seconds from the window's load event. Please make sure it has an appropriate `as` value and it is preloaded intentionally.
```

If applicable: Browser (name, version, operating system)
Chrome 120.0.6099.129 (Official Build) (arm64), MacOS 14.2

If using git command-line: output of "which git && git --version"

Comments

lu...@gmail.com <lu...@gmail.com> #2Jan 2, 2024 08:29PM

From the httpd_log, I do see the three draft comments and the publishing action:

165.1.187.207 [HTTP-2319259] - msohn [2023-12-30T12:59:02.294Z] "PUT /changes/eclipse-jgit%2Fjgit~194140/revisions/26/drafts/4aa1b660_3ea910a7 HTTP/1.1" 200 284 988 - "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36" 406 330 373559024 -
165.1.187.207 [HTTP-2323467] - msohn [2023-12-30T12:59:07.792Z] "PUT /changes/eclipse-jgit%2Fjgit~194140/revisions/26/drafts/4aa1b660_3ea910a7 HTTP/1.1" 200 285 989 - "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36" 422 340 373532728 -
165.1.187.207 [HTTP-2324129] - msohn [2023-12-30T12:59:15.263Z] "PUT /changes/eclipse-jgit%2Fjgit~194140/revisions/26/drafts/4aa1b660_3ea910a7 HTTP/1.1" 200 287 1036 - "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36" 408 330 373558568 -
165.1.187.207 [HTTP-2319259] - msohn [2023-12-30T12:59:26.113Z] "POST /changes/eclipse-jgit%2Fjgit~194140/revisions/26/review HTTP/1.1" 200 14976 3728 - "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36" 249 150 340903872 -

I also looked at the backend '/meta' ref and I do see the comments. This looks like a Gerrit Frontend issue to me.

lu...@gmail.com <lu...@gmail.com> #3Jan 2, 2024 08:58PM

I believe there are some UI issues in loading the messages, it fails in service-worker.js in:

    async init() {
        await this.loadState(),
        this.ctx.addEventListener("message", (e=>this.onMessage(e))),
        this.ctx.addEventListener("notificationclick", (e=>this.onNotificationClick(e)))
    }

I see these errors on the Web Browser's Console:

service-worker.js:1 Event handler of 'message' event must be added on the initial evaluation of worker script.
init @ service-worker.js:1
await in init (async)
(anonymous) @ service-worker.js:1
service-worker.js:1 Event handler of 'notificationclick' event must be added on the initial evaluation of worker script.
init @ service-worker.js:1
await in init (async)
(anonymous) @ service-worker.js:1

lu...@gmail.com <lu...@gmail.com> #4Jan 2, 2024 09:34PM

I actually found that the rendering of the change was absolutely fine, until the following comments were added:

    {
      "unresolved": true,
      "key": {
        "uuid": "178f1ab6_fec5fe4f",
        "filename": "org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java",
        "patchSetId": 3
      },
      "lineNbr": 233,
      "author": {
        "id": 1000000
      },
      "writtenOn": "2023-12-30T01:17:24Z",
      "side": 1,
      "message": "Ping @matthias.sohn@sap.com that\u0027s your first decision for January 2024 😊\n\nJGit is downloaded millions of times and this queue is a central point for many use-cases. We\u0027ve been using it for years with Gerrit and it works f
ine, but it would be safer to have a _safety net_ just-in-case.\n\nI agree with the 3-steps approach proposed by @david.ostrovsky@gmail.com.",
      "parentUuid": "ebef9d62_4fb99ced",
      "range": {
        "startLine": 233,
        "startChar": 2,
        "endLine": 233,
        "endChar": 93
      },
      "revId": "4566262c4845cf3071587391a7276c55f28a9de5",
      "serverId": "d5d70762-12d0-45a1-890d-524b12d3f735"
    },
    {
      "unresolved": false,
      "key": {
        "uuid": "11e7b900_7bd5e1e8",
        "filename": "org.eclipse.jgit/src/org/eclipse/jgit/revwalk/AbstractRevQueue.java",
        "patchSetId": 23
      },
      "lineNbr": 34,
      "author": {
        "id": 1000000
      },
      "writtenOn": "2023-12-30T01:17:24Z",
      "side": 1,
      "message": "+1 to @david.ostrovsky@gmail.com, never implement features that aren\u0027t used by anyone.\n\nShould Matthias decide to go with a staged rollout of this change (old priority queue + new priority queue in parallel), we can still reintroduce as default method in the interface.\n\nDeleted.",
      "parentUuid": "88c60c68_6222a64c",
      "range": {
        "startLine": 23,
        "startChar": 1,
        "endLine": 34,
        "endChar": 2
      },
      "revId": "d3877e6ae3a7d286ff614ee6ac1258d2bf7f25fa",
      "serverId": "d5d70762-12d0-45a1-890d-524b12d3f735"
    },

I am trying to see if I can reproduce the same issue on master.

lu...@gmail.com <lu...@gmail.com> #5Jan 3, 2024 12:10AM

After a thorough investigation, I have concluded that the issue is on the backend. The frontend simply filters all the threads having the 'change_message_id' associated with the messages; however, after the comment I've posted on the issue, the backend does not return the 'change_message_id' field for any of the threads anymore. When I reverted the comment in NoteDb, the problem disappeared.

Moving now on the backend side to understand what happens.

Sample of the threads payload before the NoteDb comment f62b199bd2 Update patch set 25:

{
  "author": {
    "_account_id": 1000177,
    "name": "David Ostrovsky",
    "display_name": "davido",
    "email": "david.ostrovsky@gmail.com",
    "username": "davido",
    "avatars": [
      {
        "url": "http://www.gravatar.com/avatar/4fe7eff1ca4e84c03410c29cd2bb5180.jpg?d=identicon&r=pg&s=32",
        "height": 32
      },
      {
        "url": "http://www.gravatar.com/avatar/4fe7eff1ca4e84c03410c29cd2bb5180.jpg?d=identicon&r=pg&s=56",
        "height": 56
      },
      {
        "url": "http://www.gravatar.com/avatar/4fe7eff1ca4e84c03410c29cd2bb5180.jpg?d=identicon&r=pg&s=100",
        "height": 100
      },
      {
        "url": "http://www.gravatar.com/avatar/4fe7eff1ca4e84c03410c29cd2bb5180.jpg?d=identicon&r=pg&s=120",
        "height": 120
      }
    ]
  },
  "change_message_id": "53c83b20e14398d11a3b28d78e22ac2797cbef2c",
  "unresolved": true,
  "patch_set": 3,
  "id": "dd1673d2_c6ba5b6c",
  "line": 12,
  "updated": "2022-06-14 06:13:12.000000000",
  "message": "Can you add some statistics for old/new fetch command performance for big repository to better understand motivation and justification for this change? Also a comparison with C-Git fetch would be nice.",
  "commit_id": "4566262c4845cf3071587391a7276c55f28a9de5"
}

Sample of the threads payload after the NoteDb comment f62b199bd2 Update patch set 25:

{
  "author": {
    "_account_id": 0
  },
  "unresolved": true,
  "patch_set": 3,
  "id": "dd1673d2_c6ba5b6c",
  "line": 12,
  "updated": "2022-06-14 06:13:12.000000000",
  "message": "Can you add some statistics for old/new fetch command performance for big repository to better understand motivation and justification for this change? Also a comparison with C-Git fetch would be nice.",
  "commit_id": "4566262c4845cf3071587391a7276c55f28a9de5"
}

lu...@gmail.com <lu...@gmail.com> #6Jan 3, 2024 12:22AM

It looks like the CommentsUtil.linkCommentsToChangeMessages() did not manage to associate the comments to the respective messages and therefore left the comments orphan without a reference to the message.

It is still a mystery to me how adding a new comment to a change can break the association of ALL other comments to a message. I suspect a bug in the algorithm.

jo...@gmail.com <jo...@gmail.com> #7Jan 3, 2024 12:04PM

Seems to point to change https://gerrit-review.googlesource.com/c/gerrit/+/383956

fa...@gmail.com <fa...@gmail.com> #8Jan 3, 2024 06:46PM

Seems to point to change https://gerrit-review.googlesource.com/c/gerrit/+/383956

Yes, it is indeed the offending change. The problem is the introduction of the haveSameAuthor check. In cases where changes are imported from another server the haveSameAuthor condition might not be met. The user re-mapping of users coming from another server might not have been done yet when the change is rendered.

If only one comment meets this case the code will never break out from the while loop, bringing cmItr to the max value:

for (CommentInfo comment : sortedCommentInfos) {
      // Keep advancing the change message pointer until we associate the comment to the next change
      // message in timestamp
      while (cmItr < sortedChangeMessages.size()) {
        ChangeMessage cm = sortedChangeMessages.get(cmItr);
        if (isAfter(comment, cm) || (skipAutoGeneratedMessages && isAutoGenerated(cm))) {
          cmItr += 1;
        } else {
          break;
        }
      }
      if (cmItr < changeMessages.size()) {
        comment.changeMessageId = sortedChangeMessages.get(cmItr).getKey().uuid();
      }
    }

In this case, the cmItr < changeMessages.size() condition will never be met, and the changeMessageId will never be added. This will happen for any comment subsequent to the broken one.

lu...@gmail.com <lu...@gmail.com> #9Jan 3, 2024 09:27PM

Assigned to po...@gmail.com.

Good catch @ponch, will upload your fix to eclipse.gerrithub.io for an E2E verification.

lu...@gmail.com <lu...@gmail.com> #10Jan 3, 2024 09:52PM

I've deployed the fix on eclipse.gerrithub.io and it works as expected. See the different rendering compared to review.gerrithub.io's site without the fix.

ap...@google.com <ap...@google.com> #11Jan 4, 2024 04:39PM

Project: gerrit
Branch: stable-3.9

commit a1948ebf970ef0accfbd390627ace5704703d61d
Author: Fabio Ponciroli <ponch78@gmail.com>
Date: Wed Jan 03 20:16:55 2024

Test comment linking of imported changes

When importing changes from another server, the user in the comment
might not match the one in the change message.

The same user on different Gerrit instances might have different
account Ids.
To consolidate it, an entry in the external-ids account reverse lookup
need to be added [1].

The update of external-ids is a "manual" process, and until it is done,
any reference to a "not identifiable" user will default to a
placeholder one.

If this consolidation hasn't happened yet, when rendering
the change detail page, links between comments and change messages
can be broken.

This manifests with comments not displayed at all in change
log entries. It will happen for the broken change and all the
ones following it chronologically.

This wasn't considered as part of Ia478b891 and the exit conditions.
This test demonstrates the breaking scenario.

[1]:

https://www.gerritcodereview.com/3.7.html#import-projects-changes-from-other-gerrit-servers

Bug:

Issue 318079520

Release-Notes: skip
Change-Id: I72e3af09abd63b0c758744d69bab83b8448b2c7f

M javatests/com/google/gerrit/server/restapi/change/ListChangeCommentsTest.java

https://gerrit-review.googlesource.com/400623

ap...@google.com <ap...@google.com> #12Jan 4, 2024 04:53PM

Project: gerrit
Branch: stable-3.9

commit 3a2a6be3b75fbf3a79c7f4d0f9e01d79f29cee94
Author: Fabio Ponciroli <ponch78@gmail.com>
Date: Wed Jan 03 20:25:25 2024

Fix comments rendering with imported changes

Resetting change message counter if no commments are matched.

Imported changes might lead to situations where change messages and
comments don't always match.

Only skip setting the changeMessageId for the broken message
and start checking from the beginning of the change messages.

This fixes the test added in I72e3af09.

Bug:

Issue 318079520

Release-Notes: Fix comments rendering with imported changes
Change-Id: Ia3d693ac7ea9d38578814872ef33dd442ec0fdc9

M java/com/google/gerrit/server/CommentsUtil.java
M javatests/com/google/gerrit/server/restapi/change/ListChangeCommentsTest.java

https://gerrit-review.googlesource.com/400624

ma...@gmail.com <ma...@gmail.com> #13Jan 8, 2024 07:40AM

thanks, this fix improves the situation but it seems some comments in https://eclipse.gerrithub.io/c/eclipse-jgit/jgit/+/194140 are still not displayed:

David's comment on patchset 3 from Jun 14, 2022 8:13 AM
Luca's comment on patchset 6 from Oct 13 6:37 PM
my comment on patchset 6 from Oct 14 10:42 PM
several comments from Marcin on patchsets 10, 12, 14

Message last modified on Jan 8, 2024 07:41AM

ap...@google.com <ap...@google.com> #14Jan 15, 2024 02:29PM

Project: gerrit
Branch: stable-3.9

commit e48379be9f19ef2150a32e0b8080ad15db5542ca
Author: Fabio Ponciroli <ponch78@gmail.com>
Date: Mon Jan 08 19:19:25 2024

Avoid to always scan comments from the beginning

This change Ia3d693ac resets the search of comments from
the beginning of the array when something goes wrong.

Starting from the last successful match is enough.
No need to always start from the beginning.

Bug:

Issue 318079520

Release-Notes: skip
Change-Id: I5e159d12bf0fab847c5b494d96972fd7669e0935

M java/com/google/gerrit/server/CommentsUtil.java

https://gerrit-review.googlesource.com/401297

ma...@gmail.com <ma...@gmail.com> #15Jan 15, 2024 02:33PM

Verified by ma...@gmail.com.

thanks, with this patch applied on eclipse.gerrithub.io the problem seems to be fixed. I can now see all comments of the change.

Thanks for the fix :-)

Issue 318079520

Description

Issue summary

Comments

lu...@gmail.com <lu...@gmail.com> #2Jan 2, 2024 08:29PM

lu...@gmail.com <lu...@gmail.com> #3Jan 2, 2024 08:58PM

lu...@gmail.com <lu...@gmail.com> #4Jan 2, 2024 09:34PM

lu...@gmail.com <lu...@gmail.com> #5Jan 3, 2024 12:10AM

lu...@gmail.com <lu...@gmail.com> #6Jan 3, 2024 12:22AM

jo...@gmail.com <jo...@gmail.com> #7Jan 3, 2024 12:04PM

fa...@gmail.com <fa...@gmail.com> #8Jan 3, 2024 06:46PM

lu...@gmail.com <lu...@gmail.com> #9Jan 3, 2024 09:27PM

lu...@gmail.com <lu...@gmail.com> #10Jan 3, 2024 09:52PM

ap...@google.com <ap...@google.com> #11Jan 4, 2024 04:39PM

ap...@google.com <ap...@google.com> #12Jan 4, 2024 04:53PM

ma...@gmail.com <ma...@gmail.com> #13Jan 8, 2024 07:40AM

ap...@google.com <ap...@google.com> #14Jan 15, 2024 02:29PM

ma...@gmail.com <ma...@gmail.com> #15Jan 15, 2024 02:33PM

Add comment

Issue metadata