gerrit-review.googlesource.com will require resolving comments before Submit

[Kamil Musin]

Dear Gerrit Contributors

Quick heads-up. We plan to enable a submit requirement on all changes in gerrit-review.googlesource.com that contain unresolved comments.

The change won't be able to be submitted if there are unresolved comments, unless a following line is part of the commit message.

> Unresolved-Comment-Reason: <reason>

[Han-Wen Nienhuys]

Adding a footer creates a new patchset, so it can cause extra churn on the review (dropped approvals etc.). Why not drop the header completely? Authors can simply reply 'ack' to all unresolved comments to get them resolved.

 

--

Han-Wen Nienhuys - Google Munich

I work 80%. Don't expect answers from me on Fridays.

--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Liana Sebastian

[Kamil Musin]

> Authors can simply reply 'ack' to all unresolved comments to get them resolved.

Acking all comments is still possible and I expect it will be used. The footer is simply a bit of edge-case-proofing, if for some reason you want to leave an unresolved comment you can (and don't have to do "resolve-submit-unresolve" workaround)

[Luca Milanesio]

Thanks @Kamil for the initiative,

My comments below.

On 19 Jul 2022, at 15:53, 'Kamil Musin' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

> Authors can simply reply 'ack' to all unresolved comments to get them resolved.

Do you mean the author of the change or the author of the comment?

Shouldn’t be the commenter ack-ing the answer?

Acking all comments is still possible and I expect it will be used. The footer is simply a bit of edge-case-proofing, if for some reason you want to leave an unresolved comment you can (and don't have to do "resolve-submit-unresolve" workaround)

I agree with the footer rather than inappropriate acking: it is fine to have unresolved comments if there is a reason for it (e.g. the author of the comment is not active anymore or the comment was a minor styling issue or also was prefixed with ’nit’ or ‘optional’)

Luca.

On Tue, Jul 19, 2022 at 4:45 PM Han-Wen Nienhuys <han...@google.com> wrote:

On Tue, Jul 19, 2022 at 12:29 PM 'Kamil Musin' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

Dear Gerrit Contributors

Quick heads-up. We plan to enable a submit requirement on all changes in gerrit-review.googlesource.com that contain unresolved comments.

The change won't be able to be submitted if there are unresolved comments, unless a following line is part of the commit message.

> Unresolved-Comment-Reason: <reason>

Adding a footer creates a new patchset, so it can cause extra churn on the review (dropped approvals etc.). Why not drop the header completely? Authors can simply reply 'ack' to all unresolved comments to get them resolved.

 

-- 

Han-Wen Nienhuys - Google Munich

I work 80%. Don't expect answers from me on Fridays.

--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Liana Sebastian

-- 
-- 
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/CAEA6Yacea4JPC5wVCDOA2y3w%3D2Eodnv1zuAJ4Gsiw9XNJzcwFA%40mail.gmail.com.

[Oswald Buddenhagen]

On Wed, Jul 20, 2022 at 11:43:56PM +0100, Luca Milanesio wrote:

>> On 19 Jul 2022, at 15:53, 'Kamil Musin' wrote:
>>
>> > Authors can simply reply 'ack' to all unresolved comments to get them resolved.
>
>Do you mean the author of the change or the author of the comment?
>Shouldn’t be the commenter ack-ing the answer?
>

i think we can simply assume "normal conversation rules".

>> Acking all comments is still possible and I expect it will be used.
>> The footer is simply a bit of edge-case-proofing, if for some reason
>> you want to leave an unresolved comment you can (and don't have to do
>> "resolve-submit-unresolve" workaround)
>
>I agree with the footer rather than inappropriate acking:
>

i reject the premise that "inappropriate acking" is a thing.

>(e.g. the author of the comment is not active anymore
>

whoever picks up the slack has to deal with it in *some* way.

>or the comment was a minor styling issue or also was prefixed with
>’nit’ or ‘optional’)
>

such comments are perfectly fine to "ack" (which is a different thing
than "done").

the use case of leaving comments open as a kind of "todo item" is
debatable - comments on closed changes are out of sight and therefore
out of mind. anything still open needs to be entered into *some* kind of
"more present" storage to be actually acted upon.

> On Tue, Jul 19, 2022 at 12:29 PM 'Kamil Musin' wrote:
>> Quick heads-up. We plan to enable a submit requirement on all changes
>> in gerrit-review.googlesource.com that contain unresolved comments.
>>
>> The change won't be able to be submitted if there are unresolved comments,

>>unless a following line is part of the commit message.
>>
>> > Unresolved-Comment-Reason: <reason>
>>

i fundamentally dislike the approach, because it breaks the "data
model": commit messages now contain information that makes no sense
whatsoever without actually consulting gerrit (this is unlike
change-ids, which are merely references). it's information that pertains
to the review *only*, so it should be full contained on gerrit. whenever
one finds that statement incorrect, it's a sure sign that a course of
action other than overriding the requirement should be taken (e.g.,
placing FIXMEs in the code, filing an issue, etc., and then actually
ack'ing the comment).

[Chris Poucet]

It's an escape hatch and should be used as such.  The goal is that all comments are acked.  We already use the footer for other information that is arguably review only (e.g. screenshots).  This is not so different from that.

--
--
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/YtkMkYE8rBXFoCSA%40ugly.

[Oswald Buddenhagen]

On Thu, Jul 21, 2022 at 10:26:09AM +0200, 'Chris Poucet' wrote:
>It's an escape hatch and should be used as such.
>

i suppose, but in practice every escape hatch is misused. and so far, i
haven't seen a conclusive argument that it would be actually necessary.

>We already use the footer for other information that is arguably review
>only (e.g. screenshots). This is not so different from that.
>

let's go with the screenshots. this is simply a reference to external
content, like an issue link. as long as the commit message is
self-contained even if the screenshot is inaccessible, everything is
fine. this is *very* different from essentially continuing the review
discussion in the commit message.

if the linked screenshots have a tendency to disappear, they should not
be included in the commit messages, but instead be posted as comments
(where dead links are less jarring, as old reviews are much less read
than old commit messages).

[Martin Fick]

+1 to Oswald's comments in this email

-Martin

[Edwin Kempin]

FWIW I'm also not a fan of using a footer in the commit message to do the override for the reasons that have been mentioned:
1. you need to edit the commit message to add the footer, which potentially makes the change require new approvals
    (I think this is not relevant for the gerrit project because we copy approvals when only the commit message was changed,
     but it's not unlikely other projects will copy our approach and they may not have such a copy rule in place)
2. It leaves a reference in the commit message and hence in the git history, but this information is only relevant at the moment
    when the change is being submitted and hence should not be kept forever in the git history.

Having said this, I'm still fine with using the commit message footer to do overrides, since I expect that it will be used rarely
and people most likely will just resolve all comments before submission.

However I want to propose an alternative. Instead of a footer in the commit message, could we maybe use a hashtag on the change
to signal that the change can be submitted with unresolved comments? We do have a hashtag predicate and it should be possible
to use that in the override expression. With using a hashtag the 2 problems above should not be an issue. WDYT? 

 

-Martin

[Matthias Sohn]

+1, using a hashtag to override seems a better solution than a commit message footer

 

-Martin

--
--
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/64b96030-cb8b-4958-9398-c0a223ac8eden%40googlegroups.com.

[luca.mi...@gmail.com]

On 22 Jul 2022, at 08:15, Matthias Sohn <matthi...@gmail.com> wrote:



+1 that sounds good to me.

Luca

 

-Martin

--
--
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/64b96030-cb8b-4958-9398-c0a223ac8eden%40googlegroups.com.

--
--
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/CAKSZd3SXTVOdLk6-avVJRotFkM6%3D1XeffQ_g-_08zN3V2L%2BQUg%40mail.gmail.com.

[Martin Fick]

On 7/22/22 12:45 AM, 'Edwin Kempin' via Repo and Gerrit Discussion wrote:

On Thursday, July 21, 2022 at 5:00:42 PM UTC+2 quic_...@quicinc.com wrote:

FWIW I'm also not a fan of using a footer in the commit message to do the override for the reasons that have been mentioned:
1. you need to edit the commit message to add the footer, which potentially makes the change require new approvals
    (I think this is not relevant for the gerrit project because we copy approvals when only the commit message was changed,
     but it's not unlikely other projects will copy our approach and they may not have such a copy rule in place)
2. It leaves a reference in the commit message and hence in the git history, but this information is only relevant at the moment
    when the change is being submitted and hence should not be kept forever in the git history.

However I want to propose an alternative. Instead of a footer in the commit message, could we maybe use a hashtag on the change
to signal that the change can be submitted with unresolved comments? We do have a hashtag predicate and it should be possible
to use that in the override expression. With using a hashtag the 2 problems above should not be an issue. WDYT? 

 

+1

-Martin

[Edwin Kempin]

Thanks for the feedback. I've now reconfigured the No-Unresolved-Comments submit requirement to be overridable by a

hashtag ("allow-unresolved-comments") rather than by adding a commit message footer.

Note that after applying the hashtag, you need to reload the change screen once in order to see the updated submit requirement.

I've filed this problem as Issue 16120.

It's also worth to mention that using a hashtag for overriding the submit requirement has the drawback that it doesn't allow to

specify a reason for the override, but tracking a reason doesn't seem much important in this case. If we are concerned about

this we may define multiple hashtags with pre-defined reasons (e.g. allow-unresolved-comments-unrelated-comments,

allow-unresolved-comments-to-be-resolved-by-follow-up-changes etc.), or use the prefixhashtag predicate to match arbitrary

allow-unresolved-comments- hashtags. But at this point I don't think this is necessary.