On Wed, Jul 8, 2015 at 5:47 AM, Matthew Brush mbrush@codebrainz.ca wrote:
I propose that we disallow force pushing, rebasing, squashing, etc.
on any PR until it is fully ready for final merging. […] ready for merging. At that point it _might_ make sense to fudge history and get rid of some noisy "fixup" commits[0].
The question is how to detect the "fully ready for final merging" moment. For my patches the workflow typically looks like
1. I submit a patch 2. Colomban reviews it 3. I repush the patch with fixes 4. Colomban merges it
I kind of implicitly assume that after (3) the patch will be ready for merging (and it typically is) so I do the force push but of course there may be further comments.
For bigger patch sets one should choose what seems to be most practical. For instance for
https://github.com/geany/geany/pull/488
where there were many commits and also review comments I decided to create a separate pull request containing the fixes
https://github.com/geany/geany/pull/505
to preserve the comments in the original pull request. In this case adding fix commits would make things too messy.
So personally I wouldn't carve any rules in stone and would decide case to case. For bigger patches with many review comments it's probably best to ask the reviewer which way he prefers to have the fixes committed.
Cheers,
Jiri