turn github PR squash feature off.

[mbien]

see discussion @ dev list

reasons:

• not transparent how exactly github is squashing a PR, this can lead to noreply email commit headers etc
• squashing locally as last post-review step before merge is probably the better workflow anyway and makes it less likely to merge with the wrong setting

[vieiro]

I think I will never use Github's "squash & merge" again, so it's up to you to decide if you want to set this project-wise or not.

[ebarboni]

I think it's good to avoid it for future. I think it was also an issue during a RC phase some months ago.

Could we have a nice clear alternative on cli in a readme for newbie ?

[mbien]

we have two wiki pages, one for reviewers and one for someone who is new to git(hub):
https://cwiki.apache.org/confluence/display/NETBEANS/Submitting+Pull+Requests (for onboarding)
https://cwiki.apache.org/confluence/display/NETBEANS/PRs+and+You+-+A+reviewer+Guide (for reviewers, this page is only a few week sold)

lets update those if something is missing or could be clearer.

[matthiasblaesing]

Looks sane to me. Thank you.

[vieiro]

Quick question, do we want to keep rebase: true?

[matthiasblaesing]

Quick question, do we want to keep rebase: true?

I have some serious hope, that github does not screw that up. For a rebase you can keep the author and just update the committer.

[mbien]

I never used rebase-merge for NB. But I use it often in other projects. It removes the merge message basically, the commit shows up on top of of the target branch.

Merge messages are useful for finding PRs again so we should use that in 99% of all cases. I don't mind leaving it enabled though.

[mbien]

@pepness I believe you use the github squash feature during merge, right? Would it be a problem if we turn it off?
(this means we would have to squash locally)

[lkishalmi]

-0 for me. The original issue was that it did not treat the email right. Now the UI offers updating the email on such merges. So that thing is solved.

What we are suspecting now, is for PRs with long commit history, it still does a merge commit instead of a squash. It would be good to confirm that, but that is not tat serious issue like using mage-up emails.

[matthiasblaesing]

I have no strong feelings here. We should know how to handle merges. All committers were informed multiple times, that the github GUI is a flaky tool, that might or might not skrew your merge and might have less sane defaults than the git CLI tools. I have more trust in my manual merge+force push foo, than on github, but others can see this differently.

[mbien]

the main reason why I believe turning it off would be a positive change is because this actually changes the workflow and merges exactly what you see in the PR once you press the button. There is no second guessing or "wrong setting by mistake".

It leaves less room for mistakes, what you see in the PR is what you get post merge.

[vieiro]

I have no strong feelings here. We should know how to handle merges. All committers were informed multiple times, that the github GUI is a flaky tool, that might or might not skrew your merge and might have less sane defaults than the git CLI tools. I have more trust in my manual merge+force push foo, than on github, but others can see this differently.

I don't think the problem is committers knowing how to handle merges, nor how to squash stuff. I think the problems are:

• that the Github's merge button has three very different functions in one single button (that's indeed a bad UI design) and it's easy to choose the wrong one if you're distracted. Accidents happen!
• github did some weird stuff with author names and may be a problem to track code provenance (I can't tell if it still does or not).

Reading the 2020 thread in the mailing list I see that @neilcsmith-net wanted to keep the squash option in github. It eased things during releases, IIRC.

One solution that may fit all is to keep the button as is, and let each committer to decide whether contributions are to be squashed using this github three-in-one button or to request a squash for contributtions. I personally will choose the second one from one on.

[neilcsmith-net]

Reading the 2020 thread in the mailing list I see that @neilcsmith-net wanted to keep the squash option in github. It eased things during releases, IIRC.

Don't use my past opinion as a benchmark! 😄 I'm currently +1 to disabling both squash and rebase options.

One of my concerns in the past thread, and still, is losing the relationship between PRs and commits, eg. by squashing and merging to master locally. Now that everyone seems to understand that they can force push to any PR branch (by default) that seems less of an issue. If it wasn't for edge cases, I'd argue to also disable all direct pushes to master.

[neilcsmith-net]

Would like to see rebase option also disabled, but +1 either way.

[mbien]

I'd argue to also disable all direct pushes to master.

@neilcsmith-net we enabled force push protection via #4785, I am not sure if it is even possible to enable regular pushes, it would be possible to do this indirectly by requiring a green checkmark in CI or by adding a PR approval requirement for example.

If we manage to lock us out we still have two options: simulate a green checkmark by hacking a job or an infra ticket as last resort. (without wanting to point fingers you can find tickets like that on jira - it happens :))

[pepness]

I am ok with this; then, the recommended procedure from now on is:

• squash locally after approval
• merge the PR with Create a merge commit

Is this correct?

[neilcsmith-net]

I am ok with this; then, the recommended procedure from now on is:

* squash locally after approval

* merge the PR with `Create a merge commit`

Is this correct?

• squash locally (or rebase locally, whatever needs doing)
• force push to PR (this is possible even to other people's PRs)
• wait for CI
• merge as normal via GitHub UI (create a merge commit)

That's the recommended procedure already - this PR doesn't change that.

[lkishalmi]

Create a merge commit? If it is already squashed, then it's just one commit. "rebase+merge" on GitHub UI would keep the master branch straight.

[neilcsmith-net]

@lkishalmi well, we've always used merge commits. Could be changed, but should be discussed on dev@. If that changed, it would be important to keep the option for merge commits in the UI as well though as they're essential for the release process.

[lkishalmi]

@neilcsmith-net I'm not questioning the use of merge commits in general. Maybe I just were too liberal when handling merges so far. So I confess my wrong doings here:

• I've been using rebase+merge for one commit PR-s
• I've used the GitHub Squash + Merge for multi commit PRs
• In some rare cases, when thought some commits were individually important (like merging back the delivery branch to master), I used simple merge.

[mbien]

we could turn off rebase too on github. The advantage of merge commits in projects with many contributors is that it links back to the PR the commit came from (even the branch if it still exists). It is an additional "tag" and tools like git bisect know to ignore it.

Some projects don't like merge commits, so they rebase everything for a clean, linear history. But some information will be lost when doing so. Merge commits also clearly separate from the person who integrates and the person who wrote it (all metadata, dates etc are also kept as is). Which is often not the same person here too.

I remember when git was new I didn't like merge commits at all and saw them more as a workaround for when your branch diverged from master and you were too lazy to fix it locally, but I got used to it over time and do see their benefit.

edit: we also don't have any policy to mention issues or PR numbers in the title or commit msg, merge commits are doing this for us right now.

[jlahoda]

FWIW, if we can't use squash at merge time, then we can't, but it is pretty unfortunate IMO.

The reasons why I am saying that are:

• force pushing into a branch makes reviewing more difficult; and even force pushing into the branch right before a merge is breaking the record in the PR - it is not clear what has been reviewed, because the commits that were reviewed before do not exist anymore (AFAIK, at least)
• I am not a git expert, so I may be missing some simple way of doing that, but when the feature branch includes merges (which it often does, as one sometimes needs to merge with master, and rebasing breaks the PR history), it is not, AFAIK, so easy to squash. I usually generate a patch and re-apply to a clean branch, which not as easy.

So, overall, automatic squash at merge time seems like a fairly good option to me: keeps the main history reasonable, preserves the PR history for the future and is easy to do. I wonder if we should look at options to recover a way of doing that.

[neilcsmith-net]

FWIW, if we can't use squash at merge time, then we can't, but it is pretty unfortunate IMO.

@jlahoda but it's also only codifying something that was discussed and (mostly) decided in 2020. I thought it was unfortunate at the time (as was mentioned above), but it's generally working effectively. Whether or not we want to codify makes sense here. Discussing changing the process should probably be a reopened discussion on dev@ so more people are involved / know what is expected / we document it better this time ... Having one way to do this certainly makes it easier to explain to contributors!

force pushing into a branch makes reviewing more difficult; and even force pushing into the branch right before a merge is breaking the record in the PR - it is not clear what has been reviewed, because the commits that were reviewed before do not exist anymore (AFAIK, at least)

The PR UI isn't too bad at tracking that in my opinion. It seems to have improved recently too ... or am I just noticing things I hadn't before?!

... one sometimes needs to merge with master, and rebasing breaks the PR history)

Not sure what you mean exactly by breaking the PR history? eg. I always use rebase --onto to move just the PR commits when fixing PRs moved from master to delivery. I'm curious what is "broken" there?

[mbien]

merging, since it just happened again :(

[jglick]

Note https://cwiki.apache.org/confluence/display/NETBEANS/Submitting+Pull+Requests?focusedCommentId=283118094#comment-283118094 (I no longer seem to have a valid Confluence login).