Parity between regular merges and rebase merges by ralokt · Pull Request #7919 · gogs/gogs · GitHub | Latest TMZ Celebrity News & Gossip | Watch TMZ Live
Skip to content

Parity between regular merges and rebase merges #7919

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ralokt
Copy link

@ralokt ralokt commented Feb 24, 2025

Parity between regular merges and rebase merges

This PR implements parity between regular merges and rebase merges, providing the "mirror" behaviors where rebase is the default, or even the only option.

Contains a small migration.

Design choices/reasoning

Some design choices I made here could be argued about, so I want to explain why I made them. Feel free to skip.

Data model changes

First off, PullsAllowRebase is a bool, but I needed to represent 4 states - so a change was going to be necessary. I could have gone for a single enum-ish field, but noticed that gogs uses bool options a lot, and has very few enum-ish fields.

I also realized that there was a semantic meaning to the new flag I was going to need to add: "Does this project prefer merges or rebases?". And in this more general context, of course, "Do we allow rebases" becomes "Do we allow the alternative option". This is pretty natural once you think about it, and it turns out that the flags are useful on their own.

Plus, doing it this way made migrating extremely simple. A enum-ish field would have required a data migration instead of a simple column rename.

Another alternative considered was to add two new bool options - PullsAllowMerge and PullsPreferRebase. This would have had the advantages of requiring no migrations as well as being an approach that scales to more merge styles, should they be added (users might want to enable two styles, but not the third). But I decided that more merge styles were probably unlikely, and the advantages overall not worth the downside of half the representable states making no sense.

Presentation

As great as "pretty natural once you think about it" is on the backend side of things, it's not enough in the frontend. Users (especially the technical audience in this case) could probably figure out a combination of two checkboxes, and what different combinations of them do - but it would still be a process ("What does 'alternative merge style' mean here?"). If I imagine myself using that for the first time, I imagine myself deducing what it probably means quickly, but then trying a couple of combinations out, just to make sure I'm right.

So, instead, I decided to go for a single radio button list. It takes up a bit more space, and probably wouldn't be practical if more merge styles were added - but neither would the underlying data model, as outlined above. And: This approach explicitly states the behavior that the user can expect from the application. Here, I imagine myself understanding immediately what each option does, and being confident in my assessment. To me, that's worth the slight increase in complexity.

Nonexistent issue link/YOLO

Link to the issue: n/a

I know that this is a nontrivial change, and that I violated "Talk, then code" here - I'm doing this at my own risk. If it doesn't get accepted, I'll still have gotten some actual development done in Go, which was a big part of my goal (although I would also personally really like to be able to use this feature!).

Checklist

  • I agree to follow the Code of Conduct by submitting this pull request.
  • I have read and acknowledge the Contributing guide.
  • I have added test cases to cover the new code or have provided the test plan.

Test plan

Basic use

  • Create a new repository
  • Push a new branch with at least one commit to that repository
  • Create a PR for that branch (but do not merge it)
    • Expected behavior: You are only being offered the option to perform a regular merge
  • Open the repo's advanced settings in a new tab
    • Expected behavior: You should see that the radio button group for configuring merge styles for PRs has the "Always use merge to merge commits" option selected
  • Select "Merge by default, but allow to use rebase to merge commits" and save
  • Reload the PR tab.
    • Expected behavior: You being offered the option to perform a regular merge or a rebase merge, with the regular merge being preselected.
  • In the settings tab: Select "Rebase by default, but allow to use merge to merge commits" and save
  • Reload the PR tab.
    • Expected behavior: You being offered the option to perform a regular merge or a rebase merge, with the rebase merge being preselected.
  • In the settings tab: Select "Always use rebase to merge commits" and save
  • Reload the PR tab.
    • Expected behavior: You are only being offered the option to perform a rebase merge.

Migration (should be covered by a test -> less detail)

  • before applying this change, make two repositories. Allow rebase merges in only one of them.
  • apply this change, running migrations.
  • both repositories should have remembered their respective behavior.

@ralokt ralokt requested a review from unknwon as a code owner February 24, 2025 07:51
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

Successfully merging this pull request may close these issues.

1 participant

TMZ Celebrity News – Breaking Stories, Videos & Gossip

Looking for the latest TMZ celebrity news? You've come to the right place. From shocking Hollywood scandals to exclusive videos, TMZ delivers it all in real time.

Whether it’s a red carpet slip-up, a viral paparazzi moment, or a legal drama involving your favorite stars, TMZ news is always first to break the story. Stay in the loop with daily updates, insider tips, and jaw-dropping photos.

🎥 Watch TMZ Live

TMZ Live brings you daily celebrity news and interviews straight from the TMZ newsroom. Don’t miss a beat—watch now and see what’s trending in Hollywood.