we do not have any additional merge commits in the repo
the contributors do not have to learn how to squash their commits
The downside is that there is no direct reference to the PR anymore afterwards. In ESH, we (the person that does the merge) adds a line “PR: #xyz” to the commit message, so that such a reference is available after the merge, see e.g. here.
For the maintainers, this means that the commit message should be carefully checked and condensed - otherwise they can look a bit messy.
For some more background, you can also see the discussion and tests we did beforehand at the ESH mailing list.
So if no one vetoes, I would suggest this to be the new way to merge PRs!
It looks like you need to make sure you use a new branch after one of these merges. I tend to develop in a “zwave development” branch, and then merge this to master. I then periodically rebase my development branch with master - normally after I’ve merged other PRs from someone else that will affect ZWave.
This doesn’t appear to work any more which is annoying. Each time I now create a new PR, all the old PRs are still there (I assume since GH doesn’t think they are merged) - sure, I can just do another merge commit and they’ll go away, but the list gets longer each time so it’s not really a usable solution.
Another downside is that it seems we now have to force delete branches that have been merged since git can’t tell that they are merged… I fear this will end in someone accidentally deleting their code…
Why is this an advantage? I find it useful to see the flow of where merges have come from - we’ve now lost this tracability and we also end up with this negative side effects as above .
This I agree is good since it’s hard to get people to squash.
How about a compromise - we ask people to squash their commits, and if they do, we use the normal merge. If they don’t, then we use the squash merge?
This is imho a very common practice and has always been the suggested way to work. PRs are meant to be done from feature branches, and a feature branch is EOL with the implementation and merge of the feature.
I think your situation of a “continuous development” with frequent merged into the master is rather an exceptional case and we should not treat this as the default.
Another downside is that it seems we now have to force delete branches that have been merged since git can’t tell that they are merged
I cannot confirm this - Github still shows me the same option as before:
Why is this an advantage? I find it useful to see the flow of where merges have come from
Because you only have the commits which really are about code. And traceability is fully kept as the commit still has the author and the committer in it and Github also keeps the reference to the PR, so no information is lost at all:
I’m not an active maintainer*, so take this with a grain of salt.
Given the limited # of active maintainers, and the large number of contributors (based upon backlog/community data) anything that increases the workload of the active maintainer[s] is a bad idea.
To me, squashed commits, with a meaningful commit-description** needs to be the entry criteria provided by PR submitters.
Maintainers then focus only on performing code & commit-comment review, and PR submitters should hold/retain responsibility to provide code to spec, meaningful commit-comments, any resulting revisions, and a squashed set of changes & descriptions.
Assuming a perfect PR, it should be one-click for a maintainer to accept/merge it - something that can readily be done waiting in a checkout line, or over a pint down the pub on a mobile app
If you’ve provided review commentary to get it up to spec, then the squashed commit/description belongs with the PR submitter. Eventually this breads more maintainers.
Again, there are many PR submitters, so having them implement against the common standard distributes, and scales-out, the workload better than a centralized model.
**There are a number of PR’s that you are forced to read the code, just to follow along as to what’s logically being changed. It should be possible to understand each PR’s intended function based solely upon the commit description.
So far the theory, but in practice I had to spend a significant amount of time to explain people how to rebase and squash etc. In most cases the description was provided nicely in the first commit and they just had some subsequent fixes, merged or whatever. I agree that the description has to be delivered by the contributor and we can easily ask them to do so if necessary before merging. But having to either teach them git or to abandon their contribution is pretty annoying and that’s one of the huge gains of the new method (besides a much cleaner history because there are no merge commits anymore).
I based my commentary upon how we train people at work on git. We’re using the other interaction model, but still walk folks through the process of squashed commits and rebasing before we have them push (honor/trust-based push model at the office as we have a private repo) and that parts the same in either model.
I did one of each yesterday, using simple PR’s from John, and the differences in the commit log looked (IMHO) marginal - both have the relevant traceback to the source Issue/PR#.
Anyhow, if time is spent letting people know how to squash commits, in their own branches prior to commit, then over time we’ll build more people capable of being maintainers.
You posted links early on how to do that, and I learnt from it (thanks!)
There’s often more to a merge-commit than simply pressing a button (eg. aged code requiring rebasing). These already require the PR submitter to perform the same process, with the same opportunity to fix up the description.
I consider it training camp for making more maintainers, or at least more folks that are directly able to maintain their own code - through increased trust
BTW: Is there a way for this to be set as the Default option for github’s Merge UI?
It’s non-default now, which means folks will routinely trip over it.
It is not possible to make it the default, but it is possible to deactivate merge commits - which I now just did. So if you ever come across the situation that you need a merge commit, simply use the command line tooling, which should be fine.
.
