Is anybody else sometimes frustrated by the fact that force-pushing makes it more difficult to review incremental changes during iterative review? E.g. I review a big PR, submitter addresses comments and force pushes. This seems to completely break GitHub’s “Changes since last commit”, forcing me to review everything again – as opposed to only the incremental changes. Sometimes I’m in luck and still have that PR checked out locally and I can compare. And I know that there’s a link to the force-pushes diff in the PR timeline, but that doesn’t really play well with GitHub’s review UI.
I’m thinking it would be good to discourage force-pushing as a general practice – at least for big ones. Since in scikit-image, PR commits get squashed at the end anyway, I don’t really see its benefit? I’d like to make a PR to our contribution guide when I get around to it but wanted to check in before and get possible perspectives and a discussion going.
Am I missing something? Do you guys have other ways to alleviate this problem or are you just biting the bullet?
I don’t rely on this feature, so I don’t have strong feelings about it. But I’m happy to use merges only, and to tidy up the commit message to remove merge commits.
This may be a Github-only thing. I should double check but I think Gitlab is smarter and will give you a link “changes since last push” or something of the sort, and it will also detect changes that relate to open threads.
I completely agree. In the ASPP summer school, @otizonaizit teaches that it’s “Git pornography” since it rewrites history! So, generally speaking, we shouldn’t do it. I think it’s okay only if you really know what you’re doing and if you do it only with yourself… (No joke intended!) If you have submitted a PR, and it’s not a draft PR, it’s implied that it’s {ready for review, possibly being reviewed} so there are typically other people involved at this point, and there’s no way you should rewrite commits! I think.
As you rightfully point out, @lagru, since we’ll squash commits upon merge, there is no good reason to ‘streamline’ commits during the review process. Basically, I’m in favour of including a note about this in the contribution guide, e.g.: “You may force-push onto your feature branch only if the PR hasn’t been created yet, or if it’s been created as a draft PR.”