Skip to content

Decide merge options for repo #1934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
petertseng opened this issue Jan 31, 2022 · 11 comments
Closed

Decide merge options for repo #1934

petertseng opened this issue Jan 31, 2022 · 11 comments

Comments

@petertseng
Copy link
Member

A discussion was starting in #1933, but it should be moved into its own discussion otherwise we won't be able to find it from the title.

Originally, I reacted harshly to the idea that squashing should be the only option. But after considering it, I realise I should not be so harsh. It is still my preference that a non-squash option remain available, in case there are instances where we do not want to squash. However, given this is a repo shared among many, if the consensus is that the advantage of only enabling squashes outweighs the drawbacks, I accept that consensus.

Advantages: simplifies decisions and avoids questions of "should this be squashed" - if it's the only choice, no more questions!
Drawbacks: Makes contributors' lives harder in instances where they wish to contribute commits that should not be squashed - they'll be forced to make one PR for each.

@SaschaMann
Copy link
Contributor

SaschaMann commented Jan 31, 2022

Originally, I reacted harshly to the idea that squashing should be the only option. But after considering it, I realise I should not be so harsh. It is still my preference that a non-squash option remain available, in case there are instances where we do not want to squash. However, given this is a repo shared among many, if the consensus is that the advantage of only enabling squashes outweighs the drawbacks, I accept that consensus.

In other larger repos like the former v3 repo it was rare that a PR was in a state where it shouldn't have been squashed. Given that the vast majority of PRs in this repo only edit one exercise, I imagine it'll remain similar here. However, in those rare cases there's still the option to have it rebase-merged by an admin as they can override the settings that are available.

@ErikSchierboom
Copy link
Member

It is still my preference that a non-squash option remain available, in case there are instances where we do not want to squash.

This is also my preference, given that we do sometimes have PRs with multiple commits that we'd like to merge as individual commits.

Advantages: simplifies decisions and avoids questions of "should this be squashed" - if it's the only choice, no more questions!

A key question is: how often does this occur (having a discussion on the merge strategy)?

However, in those rare cases there's still the option to have it rebase-merged by an admin as they can override the settings that are available.

This could also work., but would effectively make admins a bottleneck. Not to say that I think that won't work in practice, but just wanted to point it out.

@SaschaMann
Copy link
Contributor

A key question is: how often does this occur (having a discussion on the merge strategy)?

I think the main concern is the opposite. This discussion doesn't happen, someone merges without squashing, and the commit history gets cluttered (see the recent commits)

@ErikSchierboom
Copy link
Member

Ah, that is a very good argument.

@ErikSchierboom
Copy link
Member

@exercism/reviewers Thoughts?

TL;DR disabling the merge and rebase merge options forces all PRs to be squashed, preventing any potential messiness in the commit history. The downside being that for those cases in which we do not want squashing, an admin (Jeremy or I) have to do so.

@kotp
Copy link
Member

kotp commented Feb 3, 2022

I prefer the well curated git history that can be afforded with the rebase, but that does not always happen (perhaps more often than not we have contributors that do not realize what that is, which is fine) and the ability to revert a part of a patch-set that proves troublesome, rather than an entire patch that has to be broken down.

But when in Rome, which I have never been, but one day hope to go, perhaps I would wear a toga. (Or is that Greece?)

@BethanyG
Copy link
Member

BethanyG commented Feb 3, 2022

I'd prefer the option of a rebase as well -- but am happy to go with the majority opinion. 😄

@SleeplessByte
Copy link
Member

I hate rebases when I have PRs from PRs. I don't do that in this repo, but I do in others.

That said, I would be okay with squash-only, as even though I think sometimes we really show regular merge / rebase merge, the times that happened are soooo little compared to squash being the best choice, that I think in those cases we'll be okay cherry-picking a PR into multiple ones if we really care.

@angelikatyborska
Copy link
Member

I am very happy with only squash-merges being allowed. If there's only one option available, there is no room for accidentally choosing the wrong one or not giving it enough thought.

I have had exactly this situation at work for the last 3 years. In rare cases when we are interested what separate commits were actually in a specific PR, GitHub still has all of this data available for all PRs. The only problem we experienced with always squash-merging is, in rare cases, when somebody uses git blame to find the author of some code to ask for help, they might find the wrong person if the PR had multiple commits by different authors.

@ErikSchierboom
Copy link
Member

Okay. Having heard from a lot of people (thanks all!), let's make this a squash-only repository. If we ever want to merge/rebase, ping me and I can override it.

@ErikSchierboom
Copy link
Member

We've disabled the non-squash options. Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants