Allow rebase strategy #7689
Allow rebase strategy #7689
Conversation
Ryuno-Ki
requested review from
a team and
Rumyra
and removed request for
a team
|
Thanks so much @Ryuno-Ki 🙏 Rebase is the one thing that everyone likes to make confusing 😆 I've got one nit and will also let @escattone take a quick look 👍 |
README.md
Outdated
| 1. An alternative strategy is `git rebase` of `main` on your branch. | ||
| This will rewrite the git history and might confuse reviewers as notifications | ||
| from GitHub leed to nowhere. Your changes are replayed on top of the current | ||
| main branch at that point in time. Any additional changes should be made as |
There was a problem hiding this comment.
I much prefer rebasing. As a reviewer it is rare to get lost in the historical rewrite, and rebase it creates a much better story for git archeologists.
Anyway, this is perhaps simpler?
While merging with the
mainbranch usinggit pullis recommended (because it provides a more consistent git history), rebasing your changes usinggit rebaseis also permitted.
There was a problem hiding this comment.
In my experience, the merge strategy insofar, as it does provide an easier mental model.
For git bisect it's more challenging, since you will have commits with multiple parents.
My intention is to express, that rebase is „okay if you know what you're doing”.
I also noticed some stripped GPG verifications in my personal experience. That could also go into this PR.
There was a problem hiding this comment.
I'm not sure what you've said actually captures "OK if you know what you're doing" because even if you know what you're doing you'll still have the highlighted issue (notifications leading nowhere). I prefer my text because it captures "rebase is allowed but merge is currently preferred".
Further, the "Any additional changes should be made as commits" jarred a bit. Either approach requires changes be made as commit - unless the point is to suggest don't do squash rebase (which is something I often do, and like).
It is completely up to you though - much easier to criticize than come up with text in the first place :-)
There was a problem hiding this comment.
Fair point.
It's quite difficult to me to come up with a good phrasing (in a language that is not my first one) that „sounds good”.
Hence, I'm thankful for all the feedback you provide Hamish.
I will think more about how to put „rebase is allowed but merge is currently preferred” in elegant words.
Meanwhile: would the current state be good to go? Or shall I drop the „Any additional changes should be made as commits” part? (jarred is a word I would need to look up to remember its meaning, but I can understand the sense of it).
Squashing is a whole different subject. I only do it once I'm confident to not have to revert a commit.
But I feel, it would go to deep into git advanced knowledge at this point.
There was a problem hiding this comment.
It is hard to find exactly the right words. I've removed the "Any additional ..." bit and merged. Thank you!
|
@Rumyra Given #7689 (comment) is there any other changes I definitively need to make from your point of view? |
As discussed in https://github.com/mdn/content/discussions/6480 rebase should not be forbidden outright any more.
However, a warning is still apt.
Looking at actual behaviour, sometimes a PR branch was updated by rebasing main instead of merging it.
If this becomes acceptable behaviour, we should reflect that in our documentation.
I'd like to have the discussion of the general direction in https://github.com/mdn/content/discussions/6480 and use this PR for forging the wording.