Community
Participate
Working Groups
The current user is shown as <unknown> in Reviewers section, at least for me with https://git.eclipse.org/r/#/c/14955/
Created attachment 233959 [details] screenshot
It happens not only for the current user, I was able to see that for some reviews with users that seem to be random. Investigating...
For some reason, ChangeDetailX returned from GerritClient.getChangeDetail(int, IProgressMonitor) can have incomplete users cache (ChangeDetail#accounts). The RemoteEmfConsumer relies on the cache to be complete (contain all users/reviewers from the change) to create the model.
The idea in https://git.eclipse.org/r/#/c/15018/ was to get the missing reviewers by doing additional REST call, but it doesn't seem to work. Even when the ChangeDetail cache is complete, it looks like reviewers displayed in the editor are cached somewhere else too.
(In reply to comment #3) > can have incomplete users cache (ChangeDetail#accounts) E.g. for https://git.eclipse.org/r/#/c/4718/ the json looks like this: "accounts" : { "accounts" : [{ "id" : 1 }, { "id" : { "id" : 1 }, "fullName" : "Shawn Pearce", "preferredEmail" : "sop@google.com" }, { "id" : 118 }, { "id" : { "id" : 118 }, "fullName" : "Tomasz Zarna", "preferredEmail" : "tomasz.zarna@tasktop.com" }, { "id" : 8 }, { "id" : { "id" : 8 }, "fullName" : "Robin Rosenberg", "preferredEmail" : "robin.rosenberg@dewire.com" }, { "id" : 107 }, { "id" : { "id" : 107 }, "fullName" : "Hudson CI", "preferredEmail" : "nobody@eclipse.org" } ] } Chris Aniszczyk is missing.
I just noticed that Author and Committer fields for a Patch Set are broken too, they both say "Unspecified".
(In reply to comment #4) > The idea in https://git.eclipse.org/r/#/c/15018/ was to get the missing > reviewers by doing additional REST call, but it doesn't seem to work. Even when > the ChangeDetail cache is complete, it looks like reviewers displayed in the > editor are cached somewhere else too. Well, they need to be in the model. There are still some items stored in Task Data as well of course, but IIRC reviewers isn't one of them. BTW, If there is an issue with the ChangeDetailX caching that might explain some other anomalies. As an aside, really the point of the remote API is to insulate the UI concerns from all of this. That's why I had the (since abandoned) proposal to refactor a lot of this stuff into the Remote Factory implementations. As it is, we have a whole another layer of representation. That's impossible to avoid entriely (w/o a lot of new work) given that we need to marshal the JSON objects, but it would be good if these concerns were more cloesely aligned.
Thanks for the comment, but it doesn't help me a bit.
(In reply to comment #6) > I just noticed that Author and Committer fields for a Patch Set are broken too, > they both say "Unspecified". That would follow, I guess. We may need to look at how we're matching these users to their records. Is their a record for a given user in the IRepository#getUsers? If there isn't, we need to pull it before trying to make the association. That's tricky, because we need to anticipate that and do it in the pull phase of getting the Patch Set Content (if we do it in the apply phase, we introduce a potentially blocking call which is a big no-no) and *then* create it in the update phase. See PatchSetContentRemoteFactory L84 and L 112 respectively. I found the whole Gerrit Account caching stuff a bit opaque/magical. It just isn't clear to me how/when the change detail AccountInfoCache is updated, and what triggers that. When we pull, we're simply returning the ache provided by the ChangeDetailX merged w/ what was already there. Perhaps we should be explicitly requesting an update of that somehow? Note that the RemoteKey we use is Account.Id. Dumb question, but is this consistent across changes?
(In reply to comment #9) > Dumb question, but is this consistent across changes? Missing reviewer names seem to be random. The only pattern I've been able to observe so far is that if Author/Committer didn't vote on the change the fields always say "Unspecified".
(In reply to comment #10) > The only pattern I've been able to observe so far is that if Author/Committer > didn't vote on the change the fields always say "Unspecified". It's not an issue on http://mylyn.org/gerrit-2.6.1 though, e.g. http://mylyn.org/gerrit-2.6.1/#/c/692/
(In reply to comment #11) > (In reply to comment #10) > > The only pattern I've been able to observe so far is that if Author/Committer > > didn't vote on the change the fields always say "Unspecified". So it seems that what ChangeDetailX contains is actually different between 2.5. and 2.6.x? > > It's not an issue on http://mylyn.org/gerrit-2.6.1 though, e.g. > http://mylyn.org/gerrit-21.6.1/#/c/692/ But there is only one user there, right? Actually that's a problem w/ the test harness in general. We really don't have any testing for cases of multiple users, or when for example a user is looking at a review for which they aren't a reviewer.
(In reply to comment #12) > So it seems that what ChangeDetailX contains is actually different between 2.5. > and 2.6.x? Hard to tell, I wasn't looking so carefully at ChangeDetailX.getChange().accounts from 2.5. > > It's not an issue on http://mylyn.org/gerrit-2.6.1 though > But there is only one user there, right? Yes.
(In reply to comment #13) > ChangeDetailX.getChange().accounts from 2.5. I meant @ChangeDetailX(ChangeDetail).accounts@.
I just saw a change from Gerrit 2.5.4 [1] for which Committer (on the latest patch set) is "Unspecified". FWIW, it's supposed be a user who is not on the Reviewers list. [1] https://gerrit.chromium.org/gerrit/#/c/57803/
(In reply to comment #15) > I just saw a change from Gerrit 2.5.4 [1] for which Committer (on the latest > patch set) is "Unspecified". FWIW, it's supposed be a user who is not on the > Reviewers list. > > [1] https://gerrit.chromium.org/gerrit/#/c/57803/ See bug Bug 406261. When the committer is unspecified, it's really Gerrit itself doing a merge. In this case we don't have an account IIRC, so we treat this as anonymous, even though it's really the Gerrit system. It would be nice to detect this case. We could auto-create a Reviews model user for System, and use that. But we'd need a way to infer that this was indeed the system user and not a matter of just not properly retrieving the user. And since we don't have an Account.Id, we'd need to do something magic here to do the match in the Remote API. I don't know if this is relevant or not, but I think in some cases we only get the email, not even the user name, but I'm not sure that's relevant to this issue. Steffen's change 604edce1d17845034a9f10dcd19a698d772bde30 didn't address that. FWIW, also see https://git.eclipse.org/r/#/c/14294/ and perhaps https://git.eclipse.org/r/#/c/14328 -- different issue, but in part these changes were designed to get a better handle on all of this.
The last patch set (https://git.eclipse.org/r/#/c/15018/2), seems to fix the issue for me, but: * it doesn't cover the Unspecified Author/Committer case (and I'm not talking about bug 406261) * there is not tests for it, see comment 12 In the meantime, I asked for some hints on Gerrit group, so far no reply.
(In reply to comment #17) > In the meantime, I asked for some hints on Gerrit group, so far no reply. The inconsistency between the user cache and the current change users is not unexpected: https://groups.google.com/d/msg/repo-discuss/3hiIRprAiI0/S_rFoJCOXoIJ
(In reply to comment #18) > (In reply to comment #17) > > In the meantime, I asked for some hints on Gerrit group, so far no reply. > > The inconsistency between the user cache and the current change users is not > unexpected: > https://groups.google.com/d/msg/repo-discuss/3hiIRprAiI0/S_rFoJCOXoIJ Wow, ok this whole time I was thinking that I was totally imagining this. Nice find, Jacek!
So, is this essentially an upstream bug that we can't fix easily?
(In reply to comment #19) > Wow, ok this whole time I was thinking that I was totally imagining this. Have you seen it in version older than 2.6? > Nice find, Jacek! Thx Sam ;) (In reply to comment #20) > So, is this essentially an upstream bug that we can't fix easily? I'm not sure if Gerrit guys would agree with the term "bug", but the fact is, we should not rely on the user cache, especially in Gerrit 2.6. It's going to be deprecated in favor of REST API and inlined user info.
Is there anything more we can do to address this Gerrit limitation or should we consider this bug resolved? Just wondering if makes sense to invest more effort or if we were best off consuming upstream improvements once available and live with the current state.
Well, I admit I don't like the extra call we make in 2.6 to list all reviewers [1] only to see (sometimes) that we had all the data in the ChangeDetail [2] object already. But it's a simple fix and it works. Here are alternative solutions: * create and maintain our own user cache, maybe as part of Gerrit config * for 2.6 and later switch to REST API, convert ChangeInfo [3] to ChangeDetailX [2] * make the extra call only if required, i.e. when GerritUserRemoteFactory.pull returns a user without a name and email (meaning it has just been created in the AccountInfoCache, as it never returns null [4]) I'm not sure if any of those is possible, they are just ideas. [1] https://git.eclipse.org/r/#/c/15018/3/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/GerritClient.java [2] https://git.eclipse.org/c/mylyn/org.eclipse.mylyn.reviews.git/tree/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/compat/ChangeDetailX.java [3] https://git.eclipse.org/c/mylyn/org.eclipse.mylyn.reviews.git/tree/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/rest/ChangeInfo.java [4] https://code.google.com/p/gerrit/source/browse/gerrit-common/src/main/java/com/google/gerrit/common/data/AccountInfoCache.java#61
(In reply to comment #23) > * create and maintain our own user cache, maybe as part of Gerrit config This seems to be tricky, there is no REST API for this, and the only way to get the data is via ssh: https://groups.google.com/forum/#!topic/repo-discuss/igP9cU0f9zE
Fixed in 218959f01e2bd9533b6b034afb1f8722d255a9e8
(In reply to comment #25) > Fixed in 218959f01e2bd9533b6b034afb1f8722d255a9e8 Can you say a bit more about how you fixed it? :)
