Should Skara tooling invalidate approvals when new commits are pushed? [was: RFR: 8130738: Add tabSize property to Text and TextFlow]

[Nir Lisker]

Looks like an all-or-nothing situation - either any commit requires
re-approval or no commit requires re-approval. In this case, I would say
that all commits should require re-approval since it's the safer approach.
Having the issue stay in approved state after a significant change is much
worse than requiring the reviewers to take a look at an insignificant
commit and re-approve. The time is takes for a reviewer to go over a
comment or formatting change is short and, I believe, not intrusive to
their workflow.

On Tue, Dec 31, 2019 at 7:48 PM Kevin Rushforth <kevin.rushforth at oracle.com>
wrote:

> Since this isn't directly related to the PR in question, I'm starting a
> new thread.
>
> On 12/20/2019 7:22 PM, Philip Race wrote:
> > On 12/20/19, 7:04 PM, Scott Palmer wrote:
> >> I'm not sure if I'me supposed to try to integrate now that I've made
> >> that 10 ->  0 change, or if the new change resets the need for review...
> >
> > It shows ready, which surprises me.
> > Still learning skara .. I'd expect any change to reset as how can it
> > know if it is
> > a  good or insignificant change or not no matter how the commenter
> > characterised the issue ?
>
> Pushing a new commit doesn't automatically invalidate existing
> approvals, although it is highlighted in the "Approvers" section that
> the approval was for a prior commit:
>
>      Approvers
>          Phil Race (prr - Reviewer) Note! Review applies to f846ad6
>
> I remember bringing this up with the Skara team during my initial
> testing, since I also had wondered whether pushing a new commit should
> invalidate approvals. The current policy allows for doing trivial fixes
> brought up when approving a review (e.g., a change in a comment or
> formatting) without requiring all reviewers to re-approve. It matches
> current practice for email-based reviews, so I think the current
> behavior is a reasonable default.
>
> They did say that they could implement an optional feature (i.e., a
> project would need to opt-in to this behavior, probably via a
> .jcheck/conf property) to invalidate all approvals upon pushing a new
> commit, but I don't think an RFE was filed to track it.
>
> This seems like it could be a valuable feature, although it does come
> with a slight cost in terms of timing and flexibility. What do others
> think?
>
> -- Kevin
>
>