> What do you all think about the practice of pushing changes to an open pull request based on code review feedback? For branches / commit sets? Totally fine.

> What do you all think about the practice of pushing changes to an open pull request based on code review feedback? 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.