Unresolved comments from very old patchsets render on current patchset diffs

[Clark Boylan]

One of our users had some feedback about how Gerrit renders unresolved
comments from old patchsets on current patchset diffs. Older Gerrit
(I'm not sure exactly when this changed) wouldn't show you those
comments unless you specifically opened the old patchset as one side
of the diff. But now you seem to get unresolved comments from any
patchset regardless of age on current patchsets and this can be noisy.

One thing that I expect makes this worse for us is that many of our
users have used Gerrit for many years and comments didn't need to be
resolved. This means no one is resolving comments as they review. I've
also been told that doing that involves too much mouse clicking and
additional accounting.

I'm wondering if something can be done with the Gerrit UI to make this
less problematic. Perhaps we can go back to including the comments
only if the patchset for that comment is on one side of the diff?
Maybe we can add a per user setting to ignore comments beyond a
certain age unless explicitly opening the patchset for those comments?
One crazy idea (and I'm not sure how this would work) is to add a
third comment state that doesn't need to be toggled and Gerrit
installations could default comments to this third state instead of
defaulting to unresolved. But then largely treat those comments as
resolved.

In this example, https://i.imgur.com/IMhNsrT.png, we see patchset 5
comments from 7 months ago rendered on patchset 16's diff. In this
example, https://i.imgur.com/mqGAF9W.png, we have patchset 2 and 3
comments rendered against patchset 5. But patchset 5 no longer has the
context to render those comments on the proper lines so they end up at
the beginning of the file. The lack of context implies that things
have probably changed sufficiently that maybe we should need to render
at all?

I understand that Gerrit is trying to create a more explicit
acknowledgement of each exchange during a code review, but I can also
sympathize with users that see this as far too much overhead in order
to keep things organized reasonably. I'm not sure what the best
solution is, but figured raising the concern here might lead to good
ideas and improvements.

Clark

[Luca Milanesio]

> On 26 Jan 2022, at 22:30, Clark Boylan <clark....@gmail.com> wrote:
>
> One of our users had some feedback about how Gerrit renders unresolved
> comments from old patchsets on current patchset diffs. Older Gerrit
> (I'm not sure exactly when this changed) wouldn't show you those
> comments unless you specifically opened the old patchset as one side
> of the diff. But now you seem to get unresolved comments from any
> patchset regardless of age on current patchsets and this can be noisy.
>
> One thing that I expect makes this worse for us is that many of our
> users have used Gerrit for many years and comments didn't need to be
> resolved. This means no one is resolving comments as they review. I've
> also been told that doing that involves too much mouse clicking and
> additional accounting.
>
> I'm wondering if something can be done with the Gerrit UI to make this
> less problematic. Perhaps we can go back to including the comments
> only if the patchset for that comment is on one side of the diff?
> Maybe we can add a per user setting to ignore comments beyond a
> certain age unless explicitly opening the patchset for those comments?

That looks a good idea IMHO. I would still allow to see all comments *IF* required (a filter on the UI maybe?)

@Ben @Delphine WDYT?

Luca.

> One crazy idea (and I'm not sure how this would work) is to add a
> third comment state that doesn't need to be toggled and Gerrit
> installations could default comments to this third state instead of
> defaulting to unresolved. But then largely treat those comments as
> resolved.
>
> In this example, https://i.imgur.com/IMhNsrT.png, we see patchset 5
> comments from 7 months ago rendered on patchset 16's diff. In this
> example, https://i.imgur.com/mqGAF9W.png, we have patchset 2 and 3
> comments rendered against patchset 5. But patchset 5 no longer has the
> context to render those comments on the proper lines so they end up at
> the beginning of the file. The lack of context implies that things
> have probably changed sufficiently that maybe we should need to render
> at all?
>
> I understand that Gerrit is trying to create a more explicit
> acknowledgement of each exchange during a code review, but I can also
> sympathize with users that see this as far too much overhead in order
> to keep things organized reasonably. I'm not sure what the best
> solution is, but figured raising the concern here might lead to good
> ideas and improvements.
>
> Clark
>

> --
> --
> To unsubscribe, email repo-discuss...@googlegroups.com
> More info at http://groups.google.com/group/repo-discuss?hl=en
>
> ---
> You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/CAAcH74hvD-dM2iEW2Mm2XLykLeR3%2B6Wp-Gn29DbTmiPCYcqHRw%40mail.gmail.com.

[Ben Rohlfs]

Thanks for the feedback. It is the first time that we hear about this being a problem.

I think my preferred way of handling this would be to make it easier to establish a "comments have to be resolved" policy for all Gerrit installations. Maybe we can do deeper and look at where exactly "mouse clicking and additional accounting" is coming from. A known issue is that an owner's reply does not resolve a comment by default. That has been bothering me personally for a long while, but I was not aware of the broader impact. Assuming that users typically reply to comments, I would argue that just changing the default to "resolved" for owner replies might go a long way in addressing your concerns. WDYT?

[Dhruv Srivastava]

Thanks for bringing this up!

The reason for showing the unresolved comments in the new patchsets at the top of the file is that we want users to aggressively resolve these comments.

There are some clicks that we can avoid such as what Ben mentioned above, automatically marking owner replies to comments as resolved. This is something I plan to work on and should be there in the next release.

[Oswald Buddenhagen]

On Thu, Jan 27, 2022 at 09:42:35AM +0100, 'Dhruv Srivastava' via Repo and Gerrit Discussion wrote:
>There are some clicks that we can avoid such as what Ben mentioned
>above,
>automatically marking owner replies to comments as resolved. This is
>something I plan to work on and should be there in the next release.
>

i'm not convinced that this is a good idea. the "closing" comments that
are available through buttons already default to resolved, which covers
95% of the cases. manually entered comments are typically discussion and
shouldn't resolve unless explicitly selected.

the question would be what to do about the legacy comments. one could do
a schema migration kinda thing, where final comments that match certain
patterns resolve the threads. however, that doesn't really fix the
issue, because thread resolving was introduced quite a while before
comment porting. some people just didn't care, because it didn't bother
*them* - regardless of the fact that it disrupts their reviewers'
workflow. so they kinda had it coming.

nonetheless, one could apply a heuristic: if the code a comment applied
to changed (possibly only if it cannot be ported at all any more), it is
automatically resolved based on the fact that it is apparently not
applicable any more. the cut-off point would have to be the migration to
the gerrit version that introduced comment porting - i suppose it would
be possible to deduce that from notedb. the question would be whether
this effort is really worth it, given that large changes with a history
that is so long that this would matter are quite rare.

the quick and dirty solution would be introducing a checkbox to the
"reply" dialog, "resolve all threads", somewhere below the "publish
drafts" checkbox. of course, this kinda invites over-use (counter it by
introducing a nasty confimation dialog that explains possible unintended
consequences for the workflow?).

somewhat on a tangent, but it would help countering the fallout of
resolving more aggressively: instead of a simple "resolved" bit, threads
could have a proper status field. this would allow comment authors to
confirm that issues are *actually* resolved before they go out of view.
a "confirm thread resolutions" checkbox in the reply dialog could
reasonably provide a shortcut for this.
https://bugs.chromium.org/p/gerrit/issues/detail?id=4080 is about this,
though imo it goes somewhat overboard by suggesting issue types and
severities as well.

[Chris Poucet]

Thank you everyone for the great conversation.

Maybe it would be effective to continue this as a feature-request issue?  That will help us during our planning process to prioritize things correctly and ensure we don't lose any context.

 - simply chris
---
I'm always looking for feedback, you can provide it anonymously at go/poucet-feedback

[Clark Boylan]

Our installation hosts a number of publicly facing open source
projects. One of the behaviors we see is that people come and go over
time and reviews may be opportunistic due to availability. I can
definitely see the value in the unresolved/resolved back and forth for
a disciplined group of people that work together consistently, but
this becomes harder when your contributors may be using Gerrit for the
first time or infrequently. I do think that applying some additional
rules to when things can auto resolve would go a long way for us
though. One thing I've just noticed looking at this a bit more closely
is that patchset level comments are marked resolved by default. The
majority of this problem exists in inline comments. This makes me
hopeful that more auto resolving of inline comments would be
beneficial.

[Clark Boylan]

I have filed https://bugs.chromium.org/p/gerrit/issues/detail?id=15626
to continue this conversation.

[Martin Fick]

On 2022-01-27 09:44, Clark Boylan wrote:
> I have filed https://bugs.chromium.org/p/gerrit/issues/detail?id=15626
> to continue this conversation.

Can we potentially split this conversation (and bug) into two distinct
conversations? To me this really seems two separate issues:

1) The current implementation on new sites which don't have comments
from before the implementation

2) How the current implementation interacts with comments from before
the current implementation.

This would allow #1 to be evaluated independently of #2, and help
ensure that complaints about #2 are not seen as complaints against
#1 (and also so that solutions to #2 are not to simply remove #1),

-Martin

--
The Qualcomm Innovation Center, Inc. is a member of Code
Aurora Forum, hosted by The Linux Foundation