7 ms·

Even though you're being downvoted I do agree that this should've been squashed (I don't see any other problems here, if that's not it). I've made sure via rep

Even though you're being downvoted I do agree that this should've been squashed (I don't see any other problems here, if that's not it). I've made sure via repo config that only squash commits are enabled from now on, so this will not happen again. Thanks for the feedback!

Foxboron 3y ago
The squash merge is not going to solve the lack of proper commit messages and the fact that things are breaking the test suite left, and right. Figuring out bugs with `git bisect` is not going to be a fun endeavour for people trying to understand incompatible changes.
- cube2222 3y ago
  > and the fact that things are breaking the test suite left, and right Branch protection doesn't allow merging without a passing test-suite. > The squash merge is not going to solve the lack of proper commit messages Could you expand? You choose a sensible commit message on squash, while the PR's commits become fairly irrelevant at that point.
  - Foxboron 3y ago
    > Branch protection doesn't allow merging without a passing test-suite. https://github.com/opentffoundation/opentf/pull/243 https://github.com/opentffoundation/opentf/pull/243 EDIT: and just to point out. If you have 1 PR with 19 commits that break the test suite. The last commit fixing it doesn't matter as you will be hitting one of those 19 commits at some point during a bisect. >Could you expand? You choose a sensible commit message on squash, while the PR's commits become fairly irrelevant at that point. It's optional. Nothing prevents you from just adopting whatever the PR said initially. Turning it on doesn't automatically make it better.
    - jefftk 3y ago
      > you will be hitting one of those 19 commits at some point during a bisect But not if you merge the PR as a squash-merge, which turns those 19 "development" commits into a single "permanent" commit. Using PRs as the unit of development, with all intra-PR work squashed into a single atomic test-passing commit, is a well functioning process that many teams use.
      - jen20 3y ago
        This would be true if and only if GitHub allowed you to collaboratively review and modify the squashed commit message _as part of the approval_ process, a feature which I have been requesting from them ever since "squash and merge" became an option.
        jefftk 3y ago
        The teams I've worked on generally use a "copy the PR description into the commit message", which works well. And if there are oversights, the PR number is in the commit ID which is a link to the larger context.
        pnt12 3y ago
        It would be nice, but unfortunately it doesn't align with their goals - they want you to use github features, not git.
        jen20 3y ago
        What I’m asking for is explicitly a GitHub feature - code review around the message that will be applied when you click the “squash and merge” button in GitHub.
    - starttoaster 3y ago
      The other 19 commits are also not the current state of the code, and if the test suite is all currently passing, and nobody is using a version of the code in production from the other 19 commits, then none of that matters anyway. So I'm still confused why anyone should care. The test suite did its job by alerting you to make changes before merging your work to the trunk branch, I don't see this as anything anybody could possibly pick a fight over. And to be clear, I still use mainline terraform because I never really wanted a wrapper around terraform. It's just silly to me to take issue with the terraform wrappers for this non-issue.
Faaak 3y ago
I'm sorry, but making only "squash commits" is also a bad idea. You can have a MR with multiple independent, atomic, commits. The "real" solution is making the developers aware of the issue and cleanup up their history before doing an MR.
- orra 3y ago
  Absolutely this. A series of commits dealing with different aspects (e.g. whitespace fix, minor related bug fix, necessary refactor, then the feature) is ideal. For a start, it's easy to review, and easier to disect if something breaks. When messy git history is provided (i.e. a history of the developer fixing their own code that they missed), squashing can be a reasonable fallback. But it's never the best option, IMHO.
  - timost 3y ago
    I've found `git commit --fixup` and `git rebase --autosquash` to be very useful when accumulating fixes of previous commits.
    - jenadine 3y ago
      git-absorb is nice too
      - timost 3y ago
        Nice, no need to look up past commits ! Didn't know about this, I had to look it up. It's a separate project from git [0]. [0]: https://github.com/tummychow/git-absorb https://github.com/tummychow/git-absorb
- alexchamberlain 3y ago
  I couldn't agree more, but this is a discipline that is rare in my experience. With GitHub lacking fast forward merges, I've struggled to articulate why it's important.
Keyframe 3y ago
Don't sweat much over it yet. Default / point people to something like merge into PRs, squash PRs when bringing them to main branches, and keep everyone follow https://www.conventionalcommits.org/en/v1.0.0/ https://www.conventionalcommits.org/en/v1.0.0/ for commits untill you find your way / more edge cases crop up and you're set.

Foxboron 3y ago

The squash merge is not going to solve the lack of proper commit messages and the fact that things are breaking the test suite left, and right. Figuring out bugs with `git bisect` is not going to be a fun endeavour for people trying to understand incompatible changes.

cube2222 3y ago
> and the fact that things are breaking the test suite left, and right Branch protection doesn't allow merging without a passing test-suite. > The squash merge is not going to solve the lack of proper commit messages Could you expand? You choose a sensible commit message on squash, while the PR's commits become fairly irrelevant at that point.
- Foxboron 3y ago
  > Branch protection doesn't allow merging without a passing test-suite. https://github.com/opentffoundation/opentf/pull/243 https://github.com/opentffoundation/opentf/pull/243 EDIT: and just to point out. If you have 1 PR with 19 commits that break the test suite. The last commit fixing it doesn't matter as you will be hitting one of those 19 commits at some point during a bisect. >Could you expand? You choose a sensible commit message on squash, while the PR's commits become fairly irrelevant at that point. It's optional. Nothing prevents you from just adopting whatever the PR said initially. Turning it on doesn't automatically make it better.
  - jefftk 3y ago
    > you will be hitting one of those 19 commits at some point during a bisect But not if you merge the PR as a squash-merge, which turns those 19 "development" commits into a single "permanent" commit. Using PRs as the unit of development, with all intra-PR work squashed into a single atomic test-passing commit, is a well functioning process that many teams use.
    - jen20 3y ago
      This would be true if and only if GitHub allowed you to collaboratively review and modify the squashed commit message _as part of the approval_ process, a feature which I have been requesting from them ever since "squash and merge" became an option.
      - jefftk 3y ago
        The teams I've worked on generally use a "copy the PR description into the commit message", which works well. And if there are oversights, the PR number is in the commit ID which is a link to the larger context.
      - pnt12 3y ago
        It would be nice, but unfortunately it doesn't align with their goals - they want you to use github features, not git.
        jen20 3y ago
        What I’m asking for is explicitly a GitHub feature - code review around the message that will be applied when you click the “squash and merge” button in GitHub.
  - starttoaster 3y ago
    The other 19 commits are also not the current state of the code, and if the test suite is all currently passing, and nobody is using a version of the code in production from the other 19 commits, then none of that matters anyway. So I'm still confused why anyone should care. The test suite did its job by alerting you to make changes before merging your work to the trunk branch, I don't see this as anything anybody could possibly pick a fight over. And to be clear, I still use mainline terraform because I never really wanted a wrapper around terraform. It's just silly to me to take issue with the terraform wrappers for this non-issue.

Faaak 3y ago

I'm sorry, but making only "squash commits" is also a bad idea. You can have a MR with multiple independent, atomic, commits. The "real" solution is making the developers aware of the issue and cleanup up their history before doing an MR.

orra 3y ago
Absolutely this. A series of commits dealing with different aspects (e.g. whitespace fix, minor related bug fix, necessary refactor, then the feature) is ideal. For a start, it's easy to review, and easier to disect if something breaks. When messy git history is provided (i.e. a history of the developer fixing their own code that they missed), squashing can be a reasonable fallback. But it's never the best option, IMHO.
- timost 3y ago
  I've found `git commit --fixup` and `git rebase --autosquash` to be very useful when accumulating fixes of previous commits.
  - jenadine 3y ago
    git-absorb is nice too
    - timost 3y ago
      Nice, no need to look up past commits ! Didn't know about this, I had to look it up. It's a separate project from git [0]. [0]: https://github.com/tummychow/git-absorb https://github.com/tummychow/git-absorb
alexchamberlain 3y ago
I couldn't agree more, but this is a discipline that is rare in my experience. With GitHub lacking fast forward merges, I've struggled to articulate why it's important.

Keyframe 3y ago

Don't sweat much over it yet. Default / point people to something like merge into PRs, squash PRs when bringing them to main branches, and keep everyone follow https://www.conventionalcommits.org/en/v1.0.0/ https://www.conventionalcommits.org/en/v1.0.0/ for commits untill you find your way / more edge cases crop up and you're set.