Rebase in Pull Request Etiquette

[Ryuno-Ki]

Does that mean, item 3 in mdn/content#pull-request-etiquette is no longer valid?

I didn’t know we had such a guideline, and so I’ve never followed it. And it runs counter to best practices, so I’d be interested to understand the rationale for it, rather than just following it.

Making additional commits is not a typical or practical way to deal with the common case when rebasing is necessary.

Originally posted by @sideshowbarker in mdn/content#6460 (comment)

Moving this into a discussion.

@escattone You added this section in mdn/content@71d085c / mdn/content#143 to fix mdn/yari#1098 which in turn addresses mdn/yari#1097 - would you be willing to reconsider?

[escattone]

@Ryuno-Ki @sideshowbarker Sorry for the delayed reply! All Mozillians were out last week for a wellness week, which was nice!

The guidelines only discourage doing a git rebase (rewriting the git history) on a branch after it's been submitted for review in a pull request, simply for the reason that it's more difficult for reviewers to see what's changed since they last reviewed the PR (although I must admit that I'm guilty of doing that from time to time). What @vinyldarkscratch did in mdn/content#6460 (merging main into her branch via a merge commit) is totally fine, since she didn't rewrite the git history for her branch.

[Ryuno-Ki]

GitHub is patient :-)
No need to excuse.

Yes, a merge occured. I was more irritated by mdn/content#6460 (comment) asking for a rebase.

Personally, I prefer a rebase, as it adds only the changes on top of the most recent status.
Yes, that will screw up the review history. But given that most changes should be small in nature, it shouldn't be that harmful.

I'm fine with following a merge strategy, if that is preferred.

[escattone]

@Ryuno-Ki I'd say that if everyone involved in the pull request is OK with rebasing, it's fine. 😄

[Ryuno-Ki]

If others signal support, I'd open a PR to update our README.

[sideshowbarker]

If others signal support, I'd open a PR to update our README.

Sounds great to me

[Ryuno-Ki]

Okay, finally came around to fill mdn/content#7689 as PR

[pepelsbey]

Closing as answered/settled: mdn/content#7689