Gerrit comment tracking

[Phil Hord]

Gerrit has some fundamental limitations in comment tracking.

I would like to examine a branch which has had lots of development activity and review traffic and see which review comments went unaddressed.  I would like to be able to mark a comment as completed or not in a way that lets me look back later and determine what is left to do on this branch.  Gerrit's comment limitations do not allow me to do that.

Here's a list of impediments off the top of my head.


1. In-line comments are tied to specific patch sets.

Sure, there's no deterministic way to apply a comment automatically to a later patch set if the file has changed.  The comment may no longer be applicable even if no files changed between the patch sets.  But I find that in most cases, it is a simple matter (but a manual one) to copy a comment forward from one patch set to the next, and Gerrit's diffing engine knows how find and show just the lines where comments exist.


2. "Change" comments are mixed in with status updates.

I see this has been discussed before, but I do not see any resolution on it yet.  It should be possible to distinguish reviewer's change comments from myriad other "log postings" to a change like "Rebased", "Uploaded patch set 3" and so on.  In fact, I do not think these should be in the same category.  It's fine to display them chronologically, but it's crazy to put them in the same bucket internally.  When I post comments in a review, those comments should belong to a different category of "thing" than does the text "Patch Set 5: Looks good to me, approved".


3. "Change" comments do not support conversations like in-line comments do.

This is probably because they do not have line-numbers to associate with for proper sorting.  But see the next issue.


4. In-line comments are not threaded.

In-line comments are only tied to a specific line number.  There is no relationship between different comments except to say that one came after the other and that both are assigned to the same line number.  But if there are two issues raised concerning the same line number, there is no way to keep the comments and discussions for these two issues separate.  A threading model would help here by letting me reply to a specific comment rather than just in-line in the same location.  The same threading model could support the "change" comments conversation issue, too.


5. Comments' statuses are not trackable.

In formal environments when a review comment is made there is a requirement that the comment is addressed before the review is considered complete and the patch is accepted.  I see no way to do this in Gerrit.  The "Done" button tries to address this by giving 2nd-pass reviewers an easy way to mark which changes have been addressed, but this only adds a new comment and does not actually mark the original comment in any way.  It's just a hack.  There is no programmatic way to answer the question, "which review comments still require attention?"

For that matter, however, there is no way for a reviewer to assert that his review comment is still not addressed.

As a reviewer I would like to be able to flag a comment with some sort of severity or disposition.  Maybe something like this:

 * Question = I do not understand this change and would like some explanation or assurance.
 * Nit = There is a minor issue here, or this code smells bad or does not conform to coding standards.
 * Fixme = This is broken and must be addressed before this patch can be merged.

 * Done = Question was answered. Style or Fixme was fixed.
 * Debt = Issue was not addressed but is acceptable to merge as-is.  May be tracked as technical debt for future consideration.

This would make more sense if comments were threaded.




Is anyone else concerned about these and/or working on solution(s)?

Phil

[Saša Živkov]

Your observations are correct.

Fell free to start contributing to Gerrit :-)

 

Phil

--
--
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.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Magnus Bäck]

On Monday, May 27, 2013 at 18:35 EDT,
Phil Hord <phil...@gmail.com> wrote:

[...]

> For that matter, however, there is no way for a reviewer to assert
> that his review comment is still not addressed.
>
> As a reviewer I would like to be able to flag a comment with some
> sort of severity or disposition. Maybe something like this:
>
> * Question = I do not understand this change and would like some
> explanation or assurance.
> * Nit = There is a minor issue here, or this code smells bad or does
> not conform to coding standards.
> * Fixme = This is broken and must be addressed before this patch can
> be merged.
>
> * Done = Question was answered. Style or Fixme was fixed.
> * Debt = Issue was not addressed but is acceptable to merge as-is.
> May be tracked as technical debt for future consideration.
>
> This would make more sense if comments were threaded.

Having a way to indicate whether a comment has been addressed would be
good, but I think the five-state proposal above is overworked. A boolean
"has this comment been resolved or not" should be enough. For me there's
no fundamental difference between questions and fixmes -- they're both
cases of unresolved comments that the change author needs to address
somehow before that change is good to go.

--
Magnus Bäck
ba...@google.com

[Phil Hord]

Good point. I went overboard in my generalization. A thread needs
only two states: Unresolved, Resolved

But consider the next level once my impediments-list is resolved. The
acceptance criteria for a project may become CR+2 && V+1 &&
Count(Unresolved)==0

I would be quite happy with this scenario because it would mean that
everyone fixes their patches to salve my every complaint. But
sometimes the answer to a comment is "I agree, but I cannot fix this
yet. I will fix it later."

So I would like a thread to have three states: Unresolved, Resolved,
Debt (Accepted, but unresolved)

The project acceptance criteria still holds, but now I can accept
"Debt" into my branch: CR+2 && V+1 && Count(Unresolved)==0

Maybe some future change will allow me to define different acceptance
criteria for different branches.

dev: CR+2 && V+1
master: CR+2 && V+1 && Count(Unresolved)==0
release: CR+2 && V+1 && Count(Unresolved)==0 && Count(Debt)==0

But some debt must be accepted even in a release branch, so another
disposition state is needed. Now I have these: Unresolved, Resolved,
Debt, Releasable-Debt (Accepted for release, but unresolved). This
scenario allows me to release code with known deficiencies without
losing track of those deficiencies in the future.

Phil

[Phil Hord]

On Tue, May 28, 2013 at 8:03 AM, Saša Živkov <ziv...@gmail.com> wrote:
> On Tue, May 28, 2013 at 12:35 AM, Phil Hord <phil...@gmail.com> wrote:
>>
>> Gerrit has some fundamental limitations in comment tracking.

...

>> Is anyone else concerned about these and/or working on solution(s)?
>
> Your observations are correct.

Thanks. I'm glad to know I'm on the right track.

> Fell free to start contributing to Gerrit :-)

I do, but my Java-fu is weak. I seek consensus and disclosure before
I choose a path since the path for me will be arduous.

Phil

[Thomas Swindells (tswindel)]

> Good point. I went overboard in my generalization. A thread needs only two
> states: Unresolved, Resolved
>
> But consider the next level once my impediments-list is resolved. The
> acceptance criteria for a project may become CR+2 && V+1 &&
> Count(Unresolved)==0
>
> I would be quite happy with this scenario because it would mean that
> everyone fixes their patches to salve my every complaint. But sometimes
> the answer to a comment is "I agree, but I cannot fix this yet. I will fix it later."
>
> So I would like a thread to have three states: Unresolved, Resolved, Debt
> (Accepted, but unresolved)
>
> The project acceptance criteria still holds, but now I can accept "Debt" into
> my branch: CR+2 && V+1 && Count(Unresolved)==0
>
> Maybe some future change will allow me to define different acceptance
> criteria for different branches.
>
> dev: CR+2 && V+1
> master: CR+2 && V+1 && Count(Unresolved)==0
> release: CR+2 && V+1 && Count(Unresolved)==0 && Count(Debt)==0
>
> But some debt must be accepted even in a release branch, so another
> disposition state is needed. Now I have these: Unresolved, Resolved, Debt,
> Releasable-Debt (Accepted for release, but unresolved). This scenario allows
> me to release code with known deficiencies without losing track of those
> deficiencies in the future.
>

I agree with you for having 3 states, however by adding the fourth as well you are
getting into the position where you want Gerrit to be a defect tracking system.
I'm also not sure how the UI would work to be usable. If someone later fixes
(as a side effect) the unreleasable technical debt, how do they know that
they need to go change the state of a random comment in some arbitrary commit
they may never have seen?

Perhaps what we need is support for a plugin to allow an issue tracking system to
be launched directly from the review page, with some text pre-filled in on the issue
system, and the review comment populated with a "Won't fix now, issue XXX raised
<link>" text added and setting the comment to the "Done" state.


Thomas

[MartinFick]

You brought up many issues, I suspect it will be hard to get consensus on all of them.  Maybe pick the one you plan to address and get consensus just on that one?

Phil

[Doug Kelly]

I agree, there are some definite good points in here, and in fact other code review platforms I've used handle some of them.  One key thing I would like to have is the ability to mark a specific issue as a defect/blocking for commit.  For example, when someone submits code which is fundamentally broken, won't compile, etc. -- that would be good to call out and ensure it is resolved properly.  I don't think Gerrit should try to be a defect management/issue tracking system, but as a separate wish, it would be nice to have better tie-in with existing issue systems (Bugzilla, JIRA, etc.), perhaps leveraging the plugin API.  While this wouldn't immediately fix all your problems, it would allow for easier management of many of the suggestions you've proposed.

I do think some enhancement with threaded comments would be a benefit, as it would make the conversation that tends to occur in long comments easier to follow.  Similarly, your observations about comments on changes as a whole are valid, and I'd even add that I've seen quirks when commenting on deleted change lines (the comment was added to the DB, but I couldn't find the comment otherwise--though this was some time ago).

Even with my experience using other review software, in that role, we didn't have the ability to do most of the things you propose.  Instead, we relied upon the role of the moderator to decide what was/wasn't important, what needed to be addressed before submit, etc.  Of course, this was in a post-commit review environment, so the workflow was quite different from Gerrit's, but I don't think the role of the moderator has changed significantly.

[ShengHai Lan]

Any good news on this topic?
Thanks.

在 2013年5月28日星期二UTC+8上午6时35分02秒，Phil Hord写道：

[Chao Liu]

It would be really nice if comment status could be tracked somehow.

For example, in the DiffTable, when base is not specified, show the one with unresolved comments.