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