Hacker News Reader

Comment by Foxboron | Hacker News Reader

> Branch protection doesn't allow merging without a passing test-suite.

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.

View on HN

> 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.

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.

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.

It would be nice, but unfortunately it doesn't align with their goals - they want you to use github features, not git.

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.

> 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.

--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.

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.

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?

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.

Bisecting to find the root cause is always going to be a better strategy if you know there is a good version.

I really recommend adopting this strategy.

Bisecting can be a good strategy. But you need to look through 100 commits. I need to look through 5-8 PR diffs. Everyone thinks they have the best strategy because they get results with it. Anyway, I'll try your strategy if mine is failing.

No, it does a binary search over the 100 commits. You would probably hit the issue before you hit 7 or 10 commits depending on how lucky you are.

You should play around with git bisect - seems like you've not used it much. It's a life changer when it comes to finding what broke.

Don't forget, a project like Terraform is too big. No one person can know how the whole system works. Trying to look through PRs as a means of debugging is a fools errand. You have to take on a different mindset when working on these large codebases.

Reply on news.ycombinator.com