[wmij]

This is the flow my team uses for addressing any comments, concerns, changes, bugs, etc. We push all subsequent commits to the branch under review and prefix the commit messages with either 'fixup!', 'squash!', etc. that address the comments. The team then reviews the fixup commits further making additional comments or giving a :+1 to the specific commit SHAs. Once everything, including all fixup commits get a :+1 as sign-off will be the only time the developer does a force push to the remote branch under review. Then the developer that owns the PR is responsible for the squash, merge and closing the PR.

Using this flow we never lose any comments during the PR process and can revisit the individual commits, diffs and files changed while the PR is open.

[Myrmornis]

When I do fixup commits, I often think there will be no conflict when it gets rebased but it turns out there is. Do you not worry about mistakes entering at this (post-review) stage?

[wmij]

Yes. It has happened especially as the team has scaled in developer size and when we are in development modes where there's better chances for conflicts, for example iterating on core code during feature development, or when we're touching/adding to common configuration settings. But it hasn't been that often where a developer needed to fix a tricky conflict.

I encourage members on our team to rebase frequently as they're iterating prior to opening their PRs for their individual feature branches and I think this helps us avoid problems for our typical PRs.

Also as a rule, don't sign off on a PR until all conflicts are resolved, if there were any and review the final commit(s) carefully.

We also have the option because of some of the CI tools we've written to give our QA people access to specific SHAs that can be tested, so sometimes (rarely) QA sign-off is asked for as part of the PR.