On Wed, Oct 25, 2017 at 4:04 AM, Nico Huber nico.h@gmx.de wrote:
On 23.10.2017 09:39, Stefan Tauner wrote:
On Fri, 13 Oct 2017 17:42:11 +0200 Nico Huber nico.h@gmx.de wrote:
What I didn't realize last night: the `staging` branch contains valuable information in lots of fixup! commits that would be lost if we don't keep `staging`. They look ugly in the log but their messages still contain some reasoning about the changes. I prefer to use the current `staging` branch as `master` therefore.
*If* there is valuable information in them then they should get into the final commit message (i.e. the one that squashes everything to stable). I deem them basically as part of the review process of the whole change (i.e. the commit eventually committed to stable) and sure, there is information in it that might be interesting but this likewise applies to all review comments and I would not want them to be part of the commit messages, would you?
No, ofc not. But there is always a compromise of what to include into the commit message. Though, maybe I'd write too elaborate ones, I think yours are often too thin (it only doesn't look like it because you do too much in single commits, IMHO).
This is another area where Gerrit helps us. Each patch gets a "Reviewed-on:" line appended which makes it easy refer to review comments when additional context is needed. If we squash several patches into one we lose that context. We could have multiple "Reviewed-on:" lines in squashed patches, but that would be sort of awkward.
For someone not involved in the respective review/development process (i.e. everybody but the author of the fixups and respective reviewer(s)) it is very hard to track what the actual outcome of all of the fixups is.
In the git way of fixups (e.g. `fixup! <original commit>`), this is pretty easy: The outcome is what the original commit message promised.
Also, fixups and refactoring should not be conflated. Fixups should only ensure the intended outcome of the original patch is met. Refactoring code, for example simplification, cosmetic changes, etc. should be done in separate patches anyway.
Both can make bisecting for specific problems tedious if unlucky.Lol, that's still better than squashing commits into bigger ones which makes bisecting less feasible.
Squashing into uber-patches also defeats the purpose of `git blame`.
Squashing approach has a lot of significant drawbacks - it breaks `git bisect`, it breaks `git blame`, and depending on who is doing the squashing we might lose references to code reviews from "Reviewed-on:" lines. It also requires a lot of time and manual effort which leads to arbitrary decisions and possibly more errors. I just don't see why we would go thru all that when git and gerrit make things so simple and without all the manual steps.