Right, the primary difference between "squash and merge" and "rebase and merge" is who is responsible for writing the commit message. Squash asks the committer to do it, as part of the squash. Rebase asks the author to do it, as part of the commits in the PR. Generally speaking, I think authors should be responsible for their own commit messages. It can be a bit tough to find committers for merging PRs as it is, I don't know that asking folks to write commit logs for everything they merge is likely to improve things. :/ In my mind, a really solid PR process example might look like this:
1. Author implements a moderately sized feature. 2. The author creates commits, one for each major atomic unit of code for the feature (usually, just one commit). The commit messages have subjects and bodies, like I mentioned before. Each commit has all the relevant testing and documentation that is associated with the code it contains. 3. The author creates a PR. The subject of the PR is the broad feature title. The body contains the information we use the template to ask for. 4. A committer comes by and identifies areas for improvement. (A non-committer may also join the review, but at least one committer is involved, naturally.) 5. The author improves those areas until everyone reaches a consensus on the mergeability. 6. If there were improvement commits, the author does a `rebase -i` to squash those improvement commits appropriately up into the originals, leaving the same set of commits the PR started with, but better than before, because of the review. 7. If, during this time, master has drifted so that the branch no longer rebases cleanly, the author also rebases onto master and fixes the conflicts. 8. The author force-pushes to the branch. 9. A committer does one last sanity check to make sure the commits look right and hits "rebase and merge". For comparison, here's an example of what I think a squash process might look like: 1. Author implements a moderately sized feature. 2. The author creates commits as they go, with mildly useful, but not really informative commit messages. 3. The author creates a PR. The subject and body contain the information one might expect in the subject and body of an atomic commit. 4. A committer comes by and identifies areas for improvement. (And non-committers, as appropriate.) 5. The author adds commits to the PR to address things until consensus is reached. 6. If master has drifted, the author rebases onto master and force-pushes. 7. The committer hits "squash and merge" and, using the PR title and body as a guide, possibly verbatim. The squash and merge require slightly more of the committer and slightly less of the author, and it doesn't work if the unit of work isn't a single atomic unit of code. But for most of our work, I think it amounts to the same thing. And then, we can use `git log` to produce changelogs. If we really want to, we could use the standardized words from Keep a Changelog to identify kinds of commits: > "Added" for new features. > "Changed" for changes in existing functionality. > "Deprecated" for soon-to-be removed features. > "Removed" for now removed features. > "Fixed" for any bug fixes. Ultimately, PRs are transitory, so I'd really like for any genuinely important information to make its way into a commit message that will stick around for a while. On Wed, Oct 24, 2018 at 10:44 AM Jeremy Mitchell <[email protected]> wrote: > > I do like the idea of adding the 'squash and merge` option so we end up > with 2 Github options: > > - rebase and merge > - squash and merge > > Leave it up to the merger which option to choose. > > Now as far as how to create a "change log", back to what Leif said earlier: > > "Why not generate the ChangeLog from the actual PR “subject” line? Then it > wouldn’t matter if you have 1 or 100 commits for a particular PR. The > Sorber wrote a little script for ATS that generates our ChangeLogs *)" > > > > On Wed, Oct 24, 2018 at 10:24 AM Rawlin Peters <[email protected]> > wrote: > > > I agree with most of this and wish everyone would structure their PRs > > and commits like this, but the fact of the matter is that small > > checkpoint commits are simply just the way some people like to work. > > For the last few things I've worked on, I've tried to separate changes > > to the different components for the same logical feature into > > different PRs (API, UI, Traffic Router). I find that breaking down the > > feature into a PR per component makes things a bit easier to review, > > since not everyone has experience all the various TC components that > > could be mashed into one mega-PR. If a mega-PR contains changes to TO, > > TP, and TR all in one, some people maybe be comfortable reviewing TO > > but not necessarily TP or TR, so you end up having to coordinate with > > multiple reviewers/committers who are comfortable reviewing different > > areas just to get a single PR merged. > > > > Unless we start making good commit messages and "one standalone > > change" per commit a hard requirement for all contributors to the > > project, I doubt we can ever get to this ideal, and I'm not sure we > > should make that a requirement right now. However, by squashing and > > merging commits, we can clean up the git history by removing all of > > the non-atomic/checkpoint commits as well as other benefits that have > > already been discussed. > > > > Nonetheless, we will still be able to use the "rebase and merge" > > option at the committer's discretion. If the contributor and committer > > feel that the commits in the PR are more valuable in the history > > because they're atomic and each have good commit messages, the > > committer should choose the "rebase and merge" option. If the PR > > contains lots of small, nonatomic, checkpoint commits that would > > obfuscate the git history and make it harder for someone doing a `git > > blame` to figure out why a change was made, then maybe it would be > > better for the committer to choose the "squash and merge" option or > > kindly ask the PR submitter to squash the commits themselves into a > > single commit and write a good title+message. > > > > - Rawlin > > > > On Tue, Oct 23, 2018 at 4:49 PM Chris Lemmons <[email protected]> wrote: > > > > > > I don't think I'm quite -1 on this, but I think we can do better. > > > > > > I think a PR should be a single unit of work and a commit should be a > > > single unit of code. > > > > > > I like using atomic commits. That is, each commit does one thing. So > > > if I need to revert a fix or track down when something was introduced, > > > I can find it. For commit messages, I really like the seven rules > > > documented here, but especially the last one: > > > https://chris.beams.io/posts/git-commit/ > > > > > > > Use the body to explain what and why vs. how > > > > > > I think it's important to put the body in the commit message in order > > > to explain why the change is necessary what the non-obvious effects of > > > it are. Having a good subject for the commit is also very useful, > > > since that's what GitHub (and really all the tools) shows when you > > > scroll through commits, looking for what changed. > > > > > > In order to have atomic commits, sometimes you'll need one or more > > > commits as part of a single PR. Not most of the time, but sometimes. > > > Especially with a large feature, you may make multiple dependent > > > changes, but each one is a logical unit of work. For example, if the > > > task is "add support for foobaz plugin", there might be three commits, > > > with summaries that look like "Add support for foobaz plugin to config > > > generation", "Add UI for editing foobaz plugin config", and "Add API > > > endpoints for foobaz plugin config". That also makes it easy to direct > > > domain experts to each of the changesets for focused review. > > > > > > A PR on the other hand, is a single unit of work. While each commit > > > should be stand-alone, capable of running correctly before and after, > > > each commit might not be desirable on a standalone basis. For example, > > > while "Add support for foobaz plugin to config generation" is a single > > > unit of code in that it operates correctly, has tests, is documented, > > > and is completely implemented, it's not exceptionally useful without a > > > UI to edit it. It makes sense to group all these changes and merge > > > them as a unit. > > > > > > For almost all bug fixes and most medium to small features, each PR > > > will have precisely one commit. But especially for larger "projects", > > > it makes the most sense to group it all as one PR, but merge it as a > > > feature. > > > > > > Relatedly, it's very rarely correct to merge commits from two > > > different authors. (This is important for legal reasons as well, if it > > > ever came to it; authors license their code when they contribute it, > > > so it's important to know whence it came.) However, in the event that > > > two people jointly produce what amounts to a single unit of code (for > > > example, by pair programming, or by collaborating on a snippet of code > > > on Slack or IRC), you can document that in git by adding it to the end > > > of the commit message body: > > > > > > > Co-authored-by: name <[email protected]> > > > > Co-authored-by: another-name <[email protected]>" > > > > > > Details for that technique here: > > > > > https://help.github.com/articles/creating-a-commit-with-multiple-authors/ > > > > > > It's not quite a "standard", but it's quickly being adopted by most of > > > the tools I use, at least. > > > > > > As for the "Squash" option, I actually prefer the "Rebase" choice. > > > It's the same thing if you have single-commit PRs, but it handles > > > multi-commit PRs a bit more nicely. > > > > > > So, to summarize, here's what I think: > > > > > > - Each commit should be an atomic unit of code and be described by a > > > descriptive commit subject and body. > > > - Each PR should be a logical grouping of closely related work; > > > usually a single commit. > > > - We should use Rebase for flattening our changes. > > > - Squash is still an "okay" heuristic that accomplishes the same > > > thing, most of the time. > > > On Tue, Oct 23, 2018 at 1:06 PM Dewayne Richardson <[email protected]> > > wrote: > > > > > > > > +1 based upon the fact that the Changelog.md can't seem to stay > > current, we > > > > do need to be able to maybe at least generate one from the commit > > messages > > > > within that release window. > > > > > > > > -Dew > > > > > > > > On Tue, Oct 23, 2018 at 12:58 PM Gray, Jonathan < > > [email protected]> > > > > wrote: > > > > > > > > > +1 > > > > > > > > > > In a past life when using Microsoft VSTS with git, the default > > action was > > > > > squash. The rule of thumb we used was squash by default and only > > merge if > > > > > every commit was meaningful and required to understand what you were > > trying > > > > > to do. We didn't typically have communal feature branches, so it > > wasn't as > > > > > large a use case for us. The repo forensics are nice with merge, > > but the > > > > > reality we found was that git blame was more useful when we could > > track to > > > > > the PR rather than a mid-stream commit and then having to make a > > second > > > > > jump to the PR from there. It also cut down on back/forth design > > change > > > > > commits and helped us stay on track with 1 PR == 1 issue because it > > made > > > > > unrelated fixes more apparent. > > > > > > > > > > Jonathan G > > > > > > > > > > > > > > > On 10/23/18, 12:40 PM, "Fieck, Brennan" <[email protected]> > > > > > wrote: > > > > > > > > > > I'm not gonna say +1 or -1, but I'd still like to chime in. I > > think > > > > > it'd be nice to require PRs to squash things into atomic chunks. No > > commits > > > > > that say "started x" or "minor fixes", but idk if it's reasonable to > > be > > > > > able to boil every PR forever down to 70 characters. And sure you > > can make > > > > > multiple PRs if changes are too big to fit in one commit message, > > but then > > > > > it becomes really easy to become entwined in a chain of dependent > > PRs so > > > > > that every time one of them gets merged you have to rebase them all. > > > > > > > > > > Of course, a "reasonable squash" is pretty hard to define > > absolutely, > > > > > and could actually just make PRs harder to review - maybe it's not > > even > > > > > enforceable at all. > > > > > ________________________________________ > > > > > From: Rawlin Peters <[email protected]> > > > > > Sent: Tuesday, October 23, 2018 11:14 AM > > > > > To: [email protected] > > > > > Subject: Re: [EXTERNAL] Re: Squash and Merge for PRs > > > > > > > > > > +1 on enabling squash and merge. > > > > > > > > > > This will not only make cherry picks easier to manage with > > respect to > > > > > merge conflicts, but it will also make the git log more useful > > because > > > > > we will be able to see the full changeset of the PR in a single > > commit > > > > > of the git log. Ideally, I'd also like the squashed commit to > > contain > > > > > a link to the actual github PR so that I can easily view > > comments, but > > > > > that is a proposal for another day. > > > > > > > > > > - Rawlin > > > > > > > > > > On Tue, Oct 23, 2018 at 10:41 AM Dave Neuman <[email protected]> > > > > > wrote: > > > > > > > > > > > > This seems to have stalled, so let me try to move it along. > > > > > > > > > > > > There is an ability to do "squash and merge" in github, we > > just need > > > > > to > > > > > > agree that we want to enable it for our project. > > > > > > > > > > > > @Jeremy Mitchell <[email protected]> I am not sure > > exactly how > > > > > to > > > > > > handle the multiple committers thing, but I assume we can > > squash > > > > > into two > > > > > > commits and give one to each. However, if we are doing > > something big > > > > > > enough to require that many commits from multiple users, it > > may be > > > > > best as > > > > > > many PRs? I don't think this is a very common situation, so I > > would > > > > > like > > > > > > to avoid getting bogged down in what-ifs if possible. > > > > > > > > > > > > As for the Changelog, I am fine with enabling squash and merge > > and > > > > > keeping > > > > > > the changelog as-is while we decide if squash and merge meets > > our > > > > > needs. I > > > > > > am also game to revisit different ways to do it, and I > > shouldn't have > > > > > > conflated that conversation with this one. If someone wants > > to see > > > > > that > > > > > > changed, we should start a new discussion. > > > > > > > > > > > > Leif brings up a good point about squash and merge making > > cherry > > > > > picks > > > > > > easier so I think we should consider it for that point alone. > > > > > > > > > > > > So, what I am really asking is: "do we think it is a good > > idea to > > > > > enable > > > > > > squash and merge on our Github repo so that we can reduce the > > amount > > > > > of > > > > > > commits per PR?" I would like to get consensus for or > > against. If > > > > > it is > > > > > > not clear already, I am for. > > > > > > > > > > > > > > > > > > Thanks, > > > > > > Dave > > > > > > > > > > > > > > > > > > On Thu, Oct 18, 2018 at 2:41 PM Gelinas, Derek < > > > > > [email protected]> > > > > > > wrote: > > > > > > > > > > > > > I'll be honest, I think the simplest thing is still to just > > > > > require a > > > > > > > changelog message, added to a central file, to be included > > in the > > > > > commit. > > > > > > > We've been going over and over this for months instead of > > actually > > > > > writing > > > > > > > change logs. Let's just propose it and put it to a vote. > > > > > > > > > > > > > > > On Oct 18, 2018, at 4:33 PM, Rawlin Peters < > > > > > [email protected]> > > > > > > > wrote: > > > > > > > > > > > > > > > > I'm +1 on squashing and merging, but I think we need a > > > > > combination of > > > > > > > > a number of things to really have a decent changelog. Even > > if we > > > > > have > > > > > > > > awesome commit messages and squashed PRs, there's still > > way too > > > > > many > > > > > > > > PRs in a release to just generate a decent changelog from > > the PR > > > > > > > > titles and/or commit messages. We still need a way to > > logically > > > > > > > > separate PRs into bugfixes, new features, breaking > > changes, etc. > > > > > We > > > > > > > > could get some of that separation using github tags, but > > what if > > > > > there > > > > > > > > are multiple PRs that compose one new high-level feature? > > At that > > > > > > > > point I think we'd have to create a new tag for every new > > > > > high-level > > > > > > > > feature being worked on in order to group those PRs under > > the > > > > > same > > > > > > > > heading. There's also the problem that only committers can > > tag > > > > > issues, > > > > > > > > so it adds to the burden of committers to make sure all > > > > > non-committer > > > > > > > > issues/PRs are properly tagged. > > > > > > > > > > > > > > > > The main headache with doing a manually-curated changelog > > is the > > > > > merge > > > > > > > > conflicts it causes, but I think we can easily avoid that > > > > > problem by > > > > > > > > using a similar approach to reno [1] which breaks > > individual > > > > > notes out > > > > > > > > into their own separate yaml files which are then parsed > > and > > > > > merged > > > > > > > > into a single, cohesive changelog for each release. What > > that > > > > > means is > > > > > > > > basically every logical bugfix/new feature/upgrade note > > gets its > > > > > own > > > > > > > > file which avoids merge conflicts caused by just adding a > > line > > > > > to the > > > > > > > > end of a CHANGELOG.md file every time. > > > > > > > > > > > > > > > > - Rawlin > > > > > > > > > > > > > > > > [1] https://docs.openstack.org/reno/latest/user/usage.html > > > > > > > >> On Thu, Oct 18, 2018 at 1:45 PM Jeremy Mitchell < > > > > > [email protected]> > > > > > > > wrote: > > > > > > > >> > > > > > > > >> is there a setting in github to enable "squash and merge" > > vs > > > > > "rebase and > > > > > > > >> merge"? > > > > > > > >> > > > > > > > >> i do have a couple of concerns however: > > > > > > > >> > > > > > > > >> 1. what happens when 2+ people contribute to a PR? This > > is a > > > > > pretty > > > > > > > common > > > > > > > >> scenario. If Bob and Sam both work on PR#1 is all the > > commit > > > > > history for > > > > > > > >> Sam lost? > > > > > > > >> 2. i can see benefits of having granular history...to a > > point... > > > > > > > >> > > > > > > > >> The more I think about this. Why is squash needed? > > Commits are > > > > > grouped > > > > > > > by > > > > > > > >> PRs. Why isn't our change log just a list of PR merged > > since > > > > > the last > > > > > > > >> release? If a PR represents a working unit (as it > > should), PR > > > > > titles > > > > > > > should > > > > > > > >> be sufficient, no? > > > > > > > >> > > > > > > > >> For example, this is what our internal release change log > > looks > > > > > like. > > > > > > > >> > > > > > > > >> Changelog from 2526569a to 284555703 > > > > > > > >> ======================================== > > > > > > > >> > > > > > > > >> These changes are deployed in branch deploy-20181002. > > > > > > > >> > > > > > > > >> PR 2300: Add TO Go cachegroups/id/deliveryservices > > > > > > > >> 65e27d2cb7c12b25c97bc98b09ffa6577bb6e1ff: Add TO Go > > > > > > > >> cachegroups/id/deliveryservices (Robert Butts, May 18 > > 2018) > > > > > > > >> 130baa85b122f2827621c98f9e87b3245e18d80b: Add TO Go > > > > > > > cachegroups/dses > > > > > > > >> API test, client funcs (Robert Butts, Jun 28 2018) > > > > > > > >> 5197a2da7e323976d5404ce135ff8c42cbed64ef: Remove TO > > > > > unused func > > > > > > > >> (Robert Butts, Sep 13 2018) > > > > > > > >> PR 2803: Add Traffic Ops Golang Steering Targets > > > > > > > >> 8ba7d24303d2b420d5059654f83a7154ff7a0703: Add TO Go > > > > > > > >> steering/id/targets (Robert Butts, Jul 10 2018) > > > > > > > >> 5ba9a3b40b59f6de2c40ad71c5a0a1a84d3acacf: Add TO > > API > > > > > Tests for > > > > > > > >> steeringtargets (Robert Butts, Sep 11 2018) > > > > > > > >> eca0a7eab65cd37d6e4f0658c7640200b2e9ecda: Add TO Go > > > > > client > > > > > > > >> steeringtarget funcs (Robert Butts, Sep 12 2018) > > > > > > > >> PR 2806: Add sub to validate user input when running > > > > > postinstall script > > > > > > > >> e50eeb2c7f46828cbe1d461288f2d98ccdd4ebaa: Add sub > > to > > > > > validate > > > > > > > user > > > > > > > >> input when running postinstall script changes default cdn > > name > > > > > to not > > > > > > > >> include an underscore. (Dylan Souza, Sep 11 2018) > > > > > > > >> 9ac409904987ad0b52e3de26b79422b9688bbb8e: Altering > > > > > postinstall > > > > > > > cdn > > > > > > > >> name regex to include underscores (Dylan Souza, Sep 21 > > 2018) > > > > > > > >> 70d8893335b63db006974932341378717c42701c: Changing > > back > > > > > the > > > > > > > default > > > > > > > >> cdn name value (Dylan Souza, Sep 21 2018) > > > > > > > >> PR 2812: remove traffic_monitor_java > > > > > > > >> 4e7a7ab26cce0903ebe54dd0892da45feaf8d3de: remove > > > > > > > traffic_monitor_java > > > > > > > >> (Dan Kirkwood, Sep 12 2018) > > > > > > > >> > > > > > > > >> > > > > > > > >> [truncated] > > > > > > > >> > > > > > > > >> > > > > > > > >>> On Thu, Oct 18, 2018 at 9:52 AM Dave Neuman < > > [email protected]> > > > > > wrote: > > > > > > > >>> > > > > > > > >>> Well, our "curated" changelogs are missing A LOT of > > > > > information on what > > > > > > > >>> actually went into the release, which therefore makes it > > not > > > > > useful. > > > > > > > If we > > > > > > > >>> were better about commit messages and used squash and > > merge, > > > > > then we > > > > > > > would > > > > > > > >>> at least have something for every PR that was merged. > > > > > > > >>> > > > > > > > >>> On Thu, Oct 18, 2018 at 8:25 AM Jan van Doorn < > > [email protected]> > > > > > wrote: > > > > > > > >>> > > > > > > > >>>>> I think this just shifts the burden from writing a > > changelog > > > > > entry to > > > > > > > >>>> writing a good commit entry. > > > > > > > >>>> > > > > > > > >>>> > > > > > > > >>>> I agree, and I think that’s where that burden belongs. > > (And I > > > > > _really_ > > > > > > > >>>> hate merge conflicts). > > > > > > > >>>> > > > > > > > >>>>> There might be fewer commit entries if we squash and > > merge, > > > > > but I’m > > > > > > > >>>> doubtful that it would be as valuable as our “curated” > > > > > changelogs. > > > > > > > >>>> > > > > > > > >>>> Both are dependent on how disciplined we are. > > > > > > > >>>> > > > > > > > >>>> I’m +1 on Squash and Merge. > > > > > > > >>>> > > > > > > > >>>> Cheers, > > > > > > > >>>> JvD > > > > > > > >>>> > > > > > > > >>>> > > > > > > > >>>>> On Oct 18, 2018, at 08:18, Eric Friedrich (efriedri) > > > > > > > >>>> <[email protected]> wrote: > > > > > > > >>>>> > > > > > > > >>>>> I think this just shifts the burden from writing a > > changelog > > > > > entry to > > > > > > > >>>> writing a good commit entry. > > > > > > > >>>>> > > > > > > > >>>>> There might be fewer commit entries if we squash and > > merge, > > > > > but I’m > > > > > > > >>>> doubtful that it would be as valuable as our “curated” > > > > > changelogs. > > > > > > > >>>>> > > > > > > > >>>>> > > > > > > > >>>>> > > > > > > > >>>>> > > > > > > > >>>>>> On Oct 18, 2018, at 9:40 AM, Dave Neuman < > > [email protected]> > > > > > wrote: > > > > > > > >>>>>> > > > > > > > >>>>>> Hey All, > > > > > > > >>>>>> I want to follow up on something that was briefly > > discussed > > > > > at the > > > > > > > >>>> summit > > > > > > > >>>>>> this week. Many people do not like the Changelog and > > feel > > > > > like it > > > > > > > can > > > > > > > >>>> be a > > > > > > > >>>>>> PITA to deal with. One of the reasons we have the > > > > > changelog is > > > > > > > >>> because > > > > > > > >>>>>> there are so many commits in a given release, that it > > is > > > > > hard to > > > > > > > comb > > > > > > > >>>>>> through all of them to figure out what noteworthy > > changes > > > > > or bug > > > > > > > fixes > > > > > > > >>>> went > > > > > > > >>>>>> into the code. One thing that may help with this > > problem > > > > > is to use > > > > > > > >>>> enable > > > > > > > >>>>>> the squash and merge feature on Github for pull > > requests > > > > > [1]. This > > > > > > > >>>>>> feature would squash all commits in a PR into one > > commit. > > > > > If we > > > > > > > pair > > > > > > > >>>> the > > > > > > > >>>>>> one commit with a good commit message, we would > > essentially > > > > > get the > > > > > > > >>>> ability > > > > > > > >>>>>> to create the changelog just from the commits. > > > > > > > >>>>>> > > > > > > > >>>>>> So, I would like to propose that we enable the squash > > and > > > > > merge > > > > > > > >>> feature > > > > > > > >>>> in > > > > > > > >>>>>> the Traffic Control Github repo and use that going > > forward. > > > > > > > Thoughts? > > > > > > > >>>>>> > > > > > > > >>>>>> Thanks, > > > > > > > >>>>>> Dave > > > > > > > >>>>>> > > > > > > > >>>>>> > > > > > > > >>>>>> [1] > > > > > https://help.github.com/articles/about-pull-request-merges/ > > > > > > > >>>>> > > > > > > > >>>> > > > > > > > >>>> > > > > > > > >>> > > > > > > > > > > > > > > > > > > > > > > > >
