There a few reasons why I personally prefer squashing: 1) Single commits make it easier to see what was done to add a feature/fix a bug. This can be useful when added a feature that's similar to another feature. For example, say someone added feature A spread out across multiple commits, and then someone else wanted to add feature B that was very similar. It would be useful to be able to look at the diff for feature A to figure out exactly what changes were needed. But if that was spread out across multiple commits that can be difficult. You would need to figure out which commits were part of feature A and view those commits as one diff. Not impossible, but just more difficult.
2) Single commits make it easier to undo/revert changes. This hopefully doesn't happen often, but if something goes wrong and you realize a pull request completely broke everything after being merged, sometimes the best solution is to just revert it. If it's split across multiple commits, you need to revert all of them. Just more work to figure out which commits need to be reverted if it's possible there's more than one. 3) Single commits make it easier to bisect history. Sometimes we realize there's a bug or performance degradation well after a pull request is merged, but have no idea which commit added the bug. Using git bisect can be a good way to figure out which commit created the issue, and we can look at the diff to try to narrow down specifically what caused it. But it's not uncommon for the first commit of a pull requests to fail to compile or tests to fail and a later commit in the PR fixes it. These broken commits make bisecting a bit more difficult since certain commits just don't work. By squashing, we have a better chance that every commit compiles and tests pass, which makes git bisect easier to use. 4) Maybe most importantly to me personaly, it just makes the commit history easier to look at and reason about. Without squashing, we'd probably see a history like: Add feature A Add missing semicolon so it compiles Fix test I accidentally broke Reword comment to make more sense None of that is really useful to have in the history and just gets in the way if you're trying to look at what was recently changed. All we really want in the history is the fact that you added a new feature A. That fact that you forgot a semicolon or fixed a comment isn't really important to point out as a single change--it's just all part of adding the new feature. Squashing these kinds of changes (which is probably the most common case of extra commits we see in pull requests) just makes the history more useful. 5) I think it actually makes git blame more useful. Using the example above, git blame might say a certain line was changed by the "Add missing semicolon so it compiles" commit. That's really just not that useful. We really wanted to know that the line was added as part of added feature A. By squashing, we hiding those kinds of things that don't matter. I'll add that there are definitely reasons not to squash sometimes. For example, maybe we're making a giant change, and it's easier to reason about if it's split up into multiple steps. In that case, perhaps we would want to review it as a single pull request, but we might want to keep the story that multiple commits tells of how the giant change was made. I don't think this is a hard rule that we must always squash, but in the majority of cases, it makes our lives easier down the road. On 12/13/19 3:18 AM, Christofer Dutz wrote: > Hi all, > > May I ask why you want to squash anyway? > I usually try to commit often and write commit comments on what I did. This > way when using git blame you sort of get an explanation why a line is the way > it is. If you sqasch everything this information gets lost. > > So what's the benefit of squashing anyway? > > Chris > ________________________________ > Von: Julian Feinauer <j.feina...@pragmaticminds.de> > Gesendet: Freitag, 13. Dezember 2019 07:39:43 > An: dev@daffodil.apache.org <dev@daffodil.apache.org> > Betreff: Re: merge without squash - fix or leave it? > > Hi all, > > I suggest to not rely on features e.g. from github as this is just a "mirror" > for us as ASF and we should only rely on our own infra for our processes. > I think a force push to develop / master branch should not be a regular > process but if it happens once or twice WITH proper discussion on the list > its fine. > We also had that several times in the beginning of the PLC4X project when > things were new and some changes rather breaking. > > But, I also think its not to bad to have some more commits in the history. > This is something you will also observe is harder to keep as soon as the > number of comitters (hopefully) grows. So its probably best to already start > getting used to it. > > Julian > > Am 12.12.19, 16:00 schrieb "Beckerle, Mike" <mbecke...@tresys.com>: > > Remembering to push new changes with "autosquash" is one more thing to > forget to do, just like forgetting to squash them was in the first place. > > So I'm not sure this improves the situation unless we can require commits > to a PR to be pushed in this way. (which would be yet another hook?) > > We want a hook that just says PRs with more than 1 commit cannot be > merged. That prevents the error. > > > > > ________________________________ > From: Steve Lawrence <slawre...@apache.org> > Sent: Thursday, December 12, 2019 7:17 AM > To: dev@daffodil.apache.org <dev@daffodil.apache.org> > Subject: Re: merge without squash - fix or leave it? > > Looking into the GitHub actions solution a bit, I found this GitHub > action: > > https://github.com/xt0rted/block-autosquash-commits-action > > It's not exactly what I described, but when added to our config it will > show a build failure (similar to a test failing) if a pull request has > any *autosquash* commits. > > So one option would be to enable this and then adopt a workflow where > commits added to a pull request should be created with either "git > commit --fixup HEAD" or "git commit --squash HEAD". This will mark the > new commit as an autosquash commit and it will be more visible when it's > time to merge. > > Thoughts? > > On 12/11/19 8:08 AM, Steve Lawrence wrote: > > Considering our user base is still fairly small and you just committed > > it, I don't think it's unreasonable to force push to have a clean git > > commit history. > > > > But as the project grows, this is something we'll likely want to avoid. > > > > I think there's a way to use the new github actions feature to add > > custom checks before allowing a merge, similar to the CI stuff we do > > now. To prevent this in the future, I imagine it wouldn't be hard to add > > a check that won't allow merging if there are more than one commit in > > the PR. > > > > On 12/11/19 8:02 AM, Beckerle, Mike wrote: > >> I neglected to squash the two commits together before merging the > daffodil-2242-tunable branch, which is the standard for our workflow. > >> > >> Should I fix and force push, or just leave it? I.e., which is the > greater sin, to not squash and litter the history, or force push to master? > >> > >> ________________________________________ > >> From: mbecke...@apache.org <mbecke...@apache.org> > >> Sent: Wednesday, December 11, 2019 7:54 AM > >> To: comm...@daffodil.apache.org > >> Subject: [incubator-daffodil] 02/02: Fix 2.11 scala compile issue. > >> > >> This is an automated email from the ASF dual-hosted git repository. > >> > >> mbeckerle pushed a commit to branch master > >> in repository > https://gitbox.apache.org/repos/asf/incubator-daffodil.git > >> > >> commit 7aaabca399367b59f1eb36503d8510ed71d1e11b > >> Author: Michael Beckerle <mbecke...@tresys.com> > >> AuthorDate: Tue Dec 10 13:19:37 2019 -0500 > >> > >> Fix 2.11 scala compile issue. > >> > >> Recursive definition needed type in 2.11. Somehow 2.12 does without > >> this. > >> > >> DAFFODIL-2242 > >> --- > >> .../src/main/scala/org/apache/daffodil/dpath/Expression.scala | 10 > ++++++---- > >> 1 file changed, 6 insertions(+), 4 deletions(-) > >> > >> diff --git > a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala > b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala > >> index eb135fd..fcf666f 100644 > >> --- > a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala > >> +++ > b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala > >> @@ -39,6 +39,7 @@ import org.apache.daffodil.BasicComponent > >> import org.apache.daffodil.api.DaffodilTunables > >> import org.apache.daffodil.oolag.OOLAG.OOLAGHostImpl > >> import org.apache.daffodil.oolag.OOLAG.OOLAGHost > >> +import org.apache.daffodil.api.UnqualifiedPathStepPolicy > >> > >> /** > >> * Root class of the type hierarchy for the AST nodes used when we > >> @@ -62,8 +63,8 @@ abstract class Expression extends OOLAGHostImpl() > >> requiredEvaluations(isTypeCorrect) > >> requiredEvaluations(compiledDPath_) > >> > >> - override lazy val tunable = parent.tunable > >> - override lazy val unqualifiedPathStepPolicy = > parent.unqualifiedPathStepPolicy > >> + override lazy val tunable: DaffodilTunables = parent.tunable > >> + override lazy val unqualifiedPathStepPolicy: > UnqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy > >> /** > >> * Override where we traverse/access elements. > >> */ > >> @@ -575,8 +576,9 @@ case class WholeExpression( > >> host: BasicComponent) > >> extends Expression { > >> > >> - final override lazy val tunable = host.tunable > >> - final override lazy val unqualifiedPathStepPolicy = > host.unqualifiedPathStepPolicy > >> + final override lazy val tunable: DaffodilTunables = host.tunable > >> + final override lazy val unqualifiedPathStepPolicy : > UnqualifiedPathStepPolicy > >> + = host.unqualifiedPathStepPolicy > >> > >> def init() { > >> this.setOOLAGContext(host) // we are the root of expression, but > we propagate diagnostics further. > >> > >> > > > > >
