I think the whole idea of pushing commits to an open pull request is a good one because you can see the discussion and the commits which come after it.
GitHub actually does this nicely by showing it in a timeline where you have a comment and then a commit following it in the discussion thread.
we discussed that someone could make major changes after approval which gets around the technical limitation of merging, and the answer we all agreed on was if we cant trust the author to know when to get re-approval then there was a bigger issue at play.
For branches / commit sets? Totally fine.
For individual commits being rewritten (e.g. because you're using perforce and not on a separate branch)? I tend to push back on anything non-trivial or that will make the diff look dirty - e.g. no running mass autoformatting passes on refactored code, even if the rest of the file desperately needs it, and it's guaranteed not to break anything. Keep that in a separate followup commit. Some minor typo fixes - sure, those are fine.
Key phrases - when accepting feedback: "Sure, but can I save that for a followup commit?" - and when giving feedback: "Looks good - can you rename/fix/tweak X in a followup commit?" I'll generally note "Still TODO" in the actual commit when I do this.
> My org has not established a consistent policy around this and often people will push changes after some reviewers have already approved the request, which I find concerning.
Policy process and tools should all generally treat modified commits as unreviewed or partially reviewed, and modified branches as partially reviewed (assuming the earlier changelists/commits were not rewritten.) The additional changes haven't been reviewed.
If your policy doesn't require reviews for e.g. minor typo fixes, whitespace fixes, etc. - then as long as e.g. all the meaty commits have been reviewed, the pull request is still reviewed enough to commit. A quick "Still look good?" "Yep +1" is all that's necessary if your policy is tighter. Sometimes people will encourage you to not even wait for that - "yeah, go ahead and just commit once you've fixed these X minor issues."
Of course, your policy on that may not be established yet either - I do like it being slightly lax here, and to some degree I see what I can get away with, and it seems to work out OK ;). A tech lead's guideline to our team was simply "if I track a bug down to a changelist and find out it wasn't reviewed I'm going to be cross", leaving it up to your personal judgement how much you wanted to annoy him ;). I'm fairly up front about what I don't bother getting reviewed - this may be enforced by requiring such changelists to be marked, or they may send out email warnings - so I can trust in my coworkers (including e.g. my tech lead) to give me feedback if they'd rather I didn't take it quite so far.
> On the other hand, it is nice to see review comments being addressed in the same branch/PR. Thoughts?
In the same branch/patchset/bundle of commits? Sure. No problem. Encouraged, even, assuming it doesn't break your workflow too much. Unless they're getting unwieldy - but that's an orthogonal issue to the specific act of pushing changes. I get irritated when people throw monster refactoring patchsets at me, even if they do them perfectly and I end up having no changes to suggest. Even worse is when it's one commit. I always give a gentle nudge/reminder to break things up and submit in chunks if at all possible when it happens.