Can we finally enable "Squash and Merge"?

GitHub's "Squash and merge" has been improved over time. Now that we are no longer really using consistent checks script, can we finally enable this option? Should we?

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-commits

Example commits using "Squash and merge" from DKIST (PR numbers and authors seem to be captured now):

Are there some undesirable side effects that would prevent us from using this in astropy?

Things to consider:

How this affects existing development workflows.
How this could potentially reduce contribution barrier (e.g., a trivial PR with a lot of commits that should be squashed).
How we collect author credits.
How we check for backport consistency.
How we bisect or git blame.

I think at least allowing squash & merge would be good, especially indeed for reducing the contribution barrier. Good to keep the option not to for complicated PRs with multiple, well-considered commit (or commits from different people).

I don't think you can choose. You either enable it for the whole repo or not at all. It is up to whoever is merging the PR to decide.

That's OK -- I just wanted a maintainer to still have the choice (and perhaps set expectations for what to do; e.g., that anything that includes pre-commit commits or "fix..." or "adjusted for review comments" should be squashed).

We should most certainly not make the "Squash and Merge" option the default, but I think it should be made available as an option because it makes things more convenient for both contributors and maintainers in the common enough case where all commits should be squashed to one.

Perhaps we should go even further and decide it is always the responsibility of a maintainer to squash commits, but that discussion might be beyond the scope if this issue.

Perhaps we should go even further and decide it is always the responsibility of a maintainer to squash commits, but that discussion might be beyond the scope if this issue.

Indeed, beyond the discussion here, but I think it is a good idea. Would be especially nice if just a partial squashing wouldn't trigger full CI again - I guess an advantage of "squash and merge" is that that does not add extra CI.

Separately, I've wondered whether we could make some use of the commit message one can add to the merge commit. As is, the commits give very little insight in why choices are made - it is all in the github discussion (which I suspect/hope will be around less long than astropy).

@nstarman

@nstarman's #14952 reminded us all of this - I think we should go ahead and allow squash & merge. Personally, I'm happy for it to be the default (but it should be possible to not squash).

The one worry I have is for the (very few) people who actually write useful commit messages; it would be up to the maintainer to check that that is not lost, and ideally add to the squashed commit message as needed.

p.s. Unlike for anything in the fit history, one cannot safely assume that the github content is going to stay around (and/or be freely available). Indeed, we seem somewhat overdue for a new best platform to develop code; certainly, more involved code review and discussion on github remains rather painful, so there is room for a better alternative.

A proposal to discuss:

enable Squash & Merge
set the Squash & Merge settings to give the most verbose possible commit message (see attached image). The reviewer merging the PR is responsible for tidying up the commit message.
disable Merge and Rebase & Merge except for admins. This one is the juicy one. Why this suggestion? I don't think there is a way to set default merge types, so it would be very easy to have a PR with dozens of commits and to accidentally merge it without squashing. Currently we don't squash, so it's front-of-mind. With Squash & Merge it won't be and accidents will happen. How do I know? I use Squash & Merge for all my repos and I constantly have to remind myself Astropy does not when I review PRs. If a PR really needs many distinct commits (and IMO this is pretty rare) the reviewer just pings an admin to merge w/out squashing.

If we cannot have squash & merge as the default, then I guess we indeed may have to make it admin-only. Really, this is useful for new features only, which has become relatively rare. Plus, the multiple commits are usually more for development (at least I often branch a lot while trying out different approaches), and probably less relevant for possible reverts, etc. (one can also think about large refactoring efforts, but those can be done in multiple PRs).

I'm in favour of the maximum default commit message (which will generally just be commit titles anyway) and it being up to the maintainer to edit.

I think having the option to squash and merge has such obvious benefits that it should be enabled as soon as possible. We can afford to discuss the more controversial suggestions afterwards.

I put this on the dev telecon agenda for 2023-07-07. 🤞

Notes from 2023-06-27 infrastructure tag-up:

Indecision hereblocks Discussion of the consistency checks #14897 , so we need to do something soon.
Add a checkbox in PR checklist if someone lovingly crafted PR commits and want to opt out of maintainer using "squash & merge".
Educating maintainers with push access before enabling this feature is important.
- Have to be careful about cleaning up the commit message before clicking the button.
- Have to respect PR author opting out (pay attention to checkboxes, etc; see above).
- Have to be careful which button is currently on the browser before you push it because browser remembers previous use (see below).
- Have to document policy somewhere and make sure people can find it (development and maintenance guides).
- Have to e-mail to astropy-dev summarizing this. See https://groups.google.com/g/astropy-dev/c/f7vWb1Iav5g
We cannot make this feature the default. However, it becomes the default based on previous use anyway on the browser, and there isn't anything we can do about that...
It does not affect contributor attributions. William and Stuart checked their own repos that already use this feature.

Great summary -- all makes sense. I guess the TODO is to include the above in the development and maintenance guides and then send an e-mail to astropy-dev summarizing it?

@mhvk

Thanks, @mhvk ! I added your TODO to #14473 (comment) under the "educating" bullet point. Does that look ok?

@taldcroft

Notes from 2023-07-07 dev telecon:

Attendees decide that the benefits outweigh the costs. Accidental squash-merge of carefully crafted commits is very rare. Much more common to have new contributors being asked to squash.
If it becomes a problem we can use merge queues to help alleviate any issues with PRs accidentally being squash merged.
@taldcroft will open PR to update developer doc(s) with new policy, taking what is relevant from this issue including Can we finally enable "Squash and Merge"? #14473 (comment)

@taldcroft

For those passionate about this topic, please see #15100 . Thanks, @taldcroft !

Squash merge button is enabled. Policy is in dev docs now.

Navigation Menu

Repository navigation

Can we finally enable "Squash and Merge"? #14473

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Can we finally enable "Squash and Merge"? #14473

Description

Activity

mhvk commented on Feb 28, 2023

pllim commented on Feb 28, 2023

mhvk commented on Feb 28, 2023

eerovaher commented on Mar 6, 2023

mhvk commented on Mar 6, 2023

mhvk commented on Jun 16, 2023

nstarman commented on Jun 16, 2023

mhvk commented on Jun 16, 2023

eerovaher commented on Jun 16, 2023

pllim commented on Jun 16, 2023

pllim commented on Jun 27, 2023

mhvk commented on Jun 27, 2023

pllim commented on Jun 27, 2023

pllim commented on Jul 7, 2023

pllim commented on Jul 28, 2023

pllim commented on Aug 10, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions