3 ms·

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 bu

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.
    - Foxboron 3y ago
      Right. Imagine you are a terraform provider developer that is working on making sure their code is working with `opentf`. Lets imagine opentf does an initial `v1.0.0` release of their code and your provider doesn't work. But you know it worked with the last FOSS release of `terraform`. What do you do? You find the common ancestor between these two projects, lets say 8a085b427b74ce3829500a59508b77465f1bbef0 (as that is the last commit `opentf` has from `terraform`). You will now run `git bisect` on the history between `8a085b427b74ce3829500a59508b77465f1bbef0` and `v1.0.0`. You will do a binary search on the 100-200 commits here, and everytime the source fails to build, or the test suite doesn't pass for whatever reason, you are making it much harder for the downstream provider to figure out why their code doesn't work. You can easily just try do this today and see what happens. Does the current untidy git history cause you any problems?
      - starttoaster 3y ago
        My bias here is that I don't tend to use commits the same way you do. I would look through each PR that had been merged between now and then. Specifically looking for PRs that look like they might change the thing that I'm having issues with. Untidy git histories are so common that it's not really worth counting on, to me. A PR is a body of work that I find actually seems to matter. I wouldn't reach the same hangup you had. On the flipside, when people overload their PRs with 3-4+ deliverable items, that tends to irk me.
  - Volundr 3y ago
    > 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. Then I'll `git bisect --first-parent`, and either isolate it to your merge, or mark to merge OK. `git bisect` doesn't have to walk your branch.
    - Foxboron 3y ago
      --first-parent is a nice option I wasn't aware of. But it would still depend on upstream not merging PRs failing the test suite.

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.
  - Foxboron 3y ago
    Right. Imagine you are a terraform provider developer that is working on making sure their code is working with `opentf`. Lets imagine opentf does an initial `v1.0.0` release of their code and your provider doesn't work. But you know it worked with the last FOSS release of `terraform`. What do you do? You find the common ancestor between these two projects, lets say 8a085b427b74ce3829500a59508b77465f1bbef0 (as that is the last commit `opentf` has from `terraform`). You will now run `git bisect` on the history between `8a085b427b74ce3829500a59508b77465f1bbef0` and `v1.0.0`. You will do a binary search on the 100-200 commits here, and everytime the source fails to build, or the test suite doesn't pass for whatever reason, you are making it much harder for the downstream provider to figure out why their code doesn't work. You can easily just try do this today and see what happens. Does the current untidy git history cause you any problems?
    - starttoaster 3y ago
      My bias here is that I don't tend to use commits the same way you do. I would look through each PR that had been merged between now and then. Specifically looking for PRs that look like they might change the thing that I'm having issues with. Untidy git histories are so common that it's not really worth counting on, to me. A PR is a body of work that I find actually seems to matter. I wouldn't reach the same hangup you had. On the flipside, when people overload their PRs with 3-4+ deliverable items, that tends to irk me.
- Volundr 3y ago
  > 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. Then I'll `git bisect --first-parent`, and either isolate it to your merge, or mark to merge OK. `git bisect` doesn't have to walk your branch.
  - Foxboron 3y ago
    --first-parent is a nice option I wasn't aware of. But it would still depend on upstream not merging PRs failing the test suite.