On Fri, Nov 2, 2012 at 6:50 AM, Robert Newson <[email protected]> wrote:
> Sure, and it couldn't hurt to clarify how we should use git in these > circumstances. The principal rule is that each commit should be > self-contained and, further, that it should at least potentially be worth > reading in the future. A bug in a branch, found during a review and then > fixed, does not to be preserved and should be squashed into a single > commit. I would expect most bugfix branches to have a single commit, and > most feature branches to have several (where each is incrementally > introducing the feature, or refactoring to make way for it). And, of > course, we need the prohibition on merge commits lifted so that can > preserve that history and record the point at which it joined master. > Unfortunately, once the Pull Request is opened, it's hard to modify it. With review, it's easy to add more commits, but I do not think asking contributors to force push their PR branch is a good idea. >From the Pull Request UI on GitHub, the Files Changed tab is essentially the squashed view, while the Commits tab shows the individual commits. I tend to review the squashed view. >From here, one possible best flow is for the *committer* to squash the branch in the merge and add the appropriate "close #" or "fix #" to the commit message, so that the PR is automatically closed. Of course, that loses the authorship information. So one could do a "git merge --squash" followed by a "git commit --author=". But I wonder if squashing just isn't the wrong thing to do anyway. If we allow merge commits, instead, it becomes possible to use "git log --merges" to get the nice, squashy history, but we don't have to muck around with anything at all. I've even heard people make the case for *always* using --no-ff on every merge. "git log --first-parent" might be even more useful, since (if I'm not mistaken) it would show fast-forward pushes committers have made straight to the repo, but only show the merge commits from pull requests and feature merges. I think if we want to play nice with pull requests, we may want to re-evaluate our no-merges policy.
