Parity between regular merges and rebase merges #7919
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 abool
, 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 usesbool
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
andPullsPreferRebase
. 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
Test plan
Basic use
Migration (should be covered by a test -> less detail)