<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
inline comments below<br>
<br>
<div class="moz-cite-prefix">On 6/19/2024 8:53 AM, Andrew Hughes
wrote:<br>
</div>
<blockquote type="cite" cite="mid:ZnL-9hGoH4he_u7X@hex.discworld.ac.uk">
<pre class="moz-quote-pre" wrap="">On 16:31 Wed 19 Jun 2024, Severin Gehwolf wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On Wed, 2024-06-19 at 15:00 +0100, Andrew Haley wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Maybe this policy will result in some otherwise unnecessary
re-reviewing. But as long as it's only a single reviewer to do a final
pass, rather than a tick from everyone who has approved, I agree with
this policy.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I like this idea of a single reviewer only (not all of them) needing to
approve the final version.
FWIW, I tend to merge latest master before integrating so as to reduce
chance of breakages caused by rebases at integration time. This process
change seems to me then needing to chase up all reviewers again to
approve the final version.
Thanks,
Severin
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Only the number of reviewers required by Skara need to re-review. The
default in .jcheck/conf for jdk is currently one, but this can be
raised by the /reviewers command.</pre>
</blockquote>
<br>
Correct. In the default case, where the number of required reviewers
for a PR was not raised via the "/reviewers N" command, only one of
the reviewers who approved the PR needs to review it.<br>
<br>
For PRs where "/reviewers N" was used to increase the minimum number
of reviewers, "N" must re-review. Worth noting is that a Reviewer
can lower the number of reviewers needed back to 1, so that a single
re-review is needed. I can imagine this being done by a Reviewer who
re-reviews and notices that the only change was to fix a typo,
update the Copyright year, or do a clean merge from master.<br>
<br>
<span style="white-space: pre-wrap">
</span>
<blockquote type="cite" cite="mid:ZnL-9hGoH4he_u7X@hex.discworld.ac.uk">
<pre class="moz-quote-pre" wrap="">At present, if a Reviewer approves the first draft of a change, there
can be innumerable subsequent changes to the change, even to the point
of being completely different to the original, and the change can
still be committed. I think it would be good to close this loophole in
the process.
I've had situations in the past where I've been surprised when I've
requested changes and not had chance to review them before the patch
has been pushed on the basis of an older stale review. At least one
Reviewer should ack the version that is finally committed to the
repository, so I think this is a good proposal. We also tended to do
this informally back in the Mercurial days, so I don't think it's a
completely new idea either.
There is a trade-off against expediency here, but I think it is
warranted. As Kevin and Aleksei mentioned, this re-review need
only be checking any changes since the previous review, not a
review from scratch.
I think there is some confusion in that the 'Reviewed-by' line on the
commit already seems to have become detached from the set of Reviewers
who approved the commit. With this change, not only will it include
Authors and Committers who reviewed the change, but also Reviewers
whose review was stale and did not count towards Skara's approval
assessment. Maybe a separate topic is whether we want to lock down
Reviewed-by to just being the Reviewers Skara counted as valid, rather
than anyone who reviewed it on GitHub?</pre>
</blockquote>
<br>
That would be a bigger change and would require additional (and
separate) discussion. I'm not in favor of such a change myself,
especially given the default case where only one of multiple
Reviewers re-reviews a minor change to the PR.<br>
<br>
-- Kevin<br>
<br>
<br>
<blockquote type="cite" cite="mid:ZnL-9hGoH4he_u7X@hex.discworld.ac.uk">
<pre class="moz-quote-pre" wrap="">
Thanks,
</pre>
</blockquote>
<br>
</body>
</html>