We seem to have some strong opinions on how pull requests should be merged for core.
On the one hand, there is the view (which I share) that we should not rewrite the work of contributors. On the other hand, there is the view that we should squash all pull requests before merging. Now when I look at the other point of view, I believe I can see where they are coming from: * The GitHub UI for merging pull requests currently does not allow you to edit the first line of the commit message, forcing it to be "Merge pull request #1234 from joebloggs/jenkins-32198" * The GitHub UI for browsing the commit history currently does not allow you to view the history of the branch from the --first-parent point of view * The GitHub UI for looking at the blame on a file currently does not allow you to view the blame of the file with the --first-parent point of view So if we restrict ourselves to using the GitHub UI, you need to trade off between the ability to do code archeology from the release viewpoint and the ability to do code archeology from the what were they thinking viewpoint. Since there is a slight majority of use cases looking at things from the release viewpoint, it therefore makes sense *for users of the GitHub UI* to request that pull requests be merged... in other words: Assume that we are using the GitHub UI Assume that release viewpoint archeology is more frequent Then we should squash commits before merging Now I have a number of issues with the above set of assumptions, but the main one as I see it is this: When you throw away data, you can never recover it. I remember a science project where we recorded the temperature at different times of the day over a month... every day we would take a number of readings and record the average of the days readings... at the end of the month we were introduced to statistical analysis (this was evil teacher making us learn through making mistakes) so that we would realise that *YOU NEVER THROW AWAY THE RAW DATA* because you do not know what you will need it for in the future. I agree with some things: * The GitHub UI for merging commits is braindead and gives crappy commit summary messages... it would totally be much better if the merge commit summary was actually the PR branch description (i.e. something like what you would get locally with `merge.branchdesc=true` in your git config). I have lodged a feature request for that with GitHub. * The GitHub UI for browsing commits is a pain as you cannot see the `git log --first-parent` view (but while we merge pull requests using the GitHub UI with it's crappy "Merge pull request #1234 from joebloggs/jenkins-32198" style summaries it's not like `git log --first-parent` gives us a great history view either) * The GitHub UI for attributing blame should allow for the `--first-parent` `-w` `-C` and `-M` options to be configured. So I see lots of issues with the GitHub UI... but I do not see that the solution is to squash commits. Firstly, if you are squashing commits, then you have to be merging from the command line... and if you are merging from the command line, well you can give a proper merge commit message and then `git log --first-parent` is beautiful and we don't need to throw away the raw data. Secondly, the code archeology almost always starts from `git blame` and the number of hours I have wasted with `git blame` when I really wanted `git blame --first-parent -w` (sometimes with a mix of `-C` `-M`) is such that I have learned *never* to use the GitHub UI to track these things down. Attempting to do any code archeology from the GitHub UI is a complete and utter waste of time and will cause you to lose the will to live. Thirdly, there is an argument about cherry picking which seems to ignore two simple facts: * You can cherry pick merge commits - provided you know what number to pass to `-m` * When you use `git log --first-parent` then you know it's always `git cherry-pick -m 1` So I see the advocates of squashing commits as looking for * a short-term gain (because GitHub will eventually fix their crappy UI bugs) * of limited utility (because you almost always are better off with the Git CLI anyway) * with questionable value (because the commit history is still distracted by all those "Merge pull request #... from .../..." noise) Now be careful, because I am not against asking the pull request author to tidy up their PR before we merge it... there is a big difference between asking for a tidy-up and mandating a squash. If you tidy-up a pull request you might probably: * reorganise the history into a logical flow. * clean up the commit messages to better reflect your thinking * remove noise commits from formatting changes and reverts of those formatting changes * combine some commits together as they form a single logical change * split some commits apart as they co-mix two logically separate changes I have some concerns about mandating a tidy-up (I note that Jesse is against rewriting the history of a PR branch as that means that GitHub hides the code review comments) but I see nothing wrong with asking for a tidy-up as being a choice available to the merger... Aside: There is also a possible small question of the copyright of commit messages and whether those commit messages are covered by the MIT license that the code diff is covered by... after all, at the time the MIT license was drafted there was no such thing as a pull request containing multiple commits, so the concept of a license applying to the patch metadata would not even have occurred. It's probably not too important but it may mean that even under the current CLA for core we do not have the legal right to modify the original author's commit messages for the initial merge to master.... never mind the drive-by pull requests we get from people without a CLA. But really what do others think? How do people want to merge pull requests against core? Do we want to use the GitHub UI to merge the pull requests? Do we want to mandate squashing commits (which will prevent us from using the GitHub UI to merge pull requests and force manual closing of the PRs)? Do we want to mandate tidying up commits (which will hide code review comments)? -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMwia%3DRdtqWN3GAnA1VOvJzDd1zwk41USkEvrHkWZEmdeg%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
