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

[Johan Vos]

I agree. If the fix is trivial, the time for the reviewer will be really
short. The more complex the fix, the more time it will take -- with the
risk of being delayed to the next release.
The github interface makes it very easy to inspect the new commit, hence a
typo in javadoc can easily be fixed and re-approved.

I think this approach is safer than the approach were "trivial" fixes don't
need re-approval, as that approach leaves more room for interpretation (and
probably even more interactions with the reviewers ("is this a trivial
fix?")).

- Johan

Op wo 8 jan. 2020 om 19:34 schreef Nir Lisker <nlisker at gmail.com>:

> 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
> >
> >
>