Skip to content

Conversation

@Managor
Copy link
Member

@Managor Managor commented Nov 14, 2025

Checklist

  • The page(s) are in the correct platform directories: common, linux, osx, windows, sunos, android, etc.
  • The page description(s) have links to documentation or a homepage.
  • The page(s) follow the content guidelines.
  • The page(s) follow the style guide.
  • The PR contains at most 5 new pages.
  • The PR is authored by me, or has been human-reviewed if it was created with AI or machine translation software.
  • The PR title conforms to the recommended templates.
  • Version of the command being documented (if known):

Reference issue: #

@github-actions github-actions bot added the documentation Issues/PRs modifying the documentation. label Nov 14, 2025
Co-authored-by: K.B.Dharun Krishna <[email protected]>
Copy link
Member

@SethFalco SethFalco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this makes more sense!

Just a pet peeve, but I'm actually not a big fan of the term "subcommit" myself, as this appears to be a non-standard way to refer to commits in the source branch. 🤔

I prefer "mess" which, to me, feels obvious is an informal term, while subcommit gives me the impression it's a concept of git I missed.

I get what Krishna tried to achieve there, and think it does establish the understanding intended. However, I don't think it's worth inventing vocabulary that's rarely if ever used.

I don't feel strongly enough about it to block the PR though, but I thought it was worth sharing in case it does convince anyone to reconsider.

@waldyrious
Copy link
Member

waldyrious commented Nov 15, 2025

Thanks for tackling this, @Managor! I think this is a good direction, but I would prefer it to be a little more explicit. In particular, "so the order of events is preserved" should make it clear we refer to the evolution of the PR based on the review suggestions.

It would also be worth mentioning that GitHub's PR review UI is unable to handle force-pushes, to make it clear it's not a matter of principle, but a limitation of the tooling we use (other forges like Gerrit or GitLab, for example, are able to show various versions of a merge request, so it's easy to see what changed in each iteration.)

Finally, I would like this to acknowledge the reasons why one might want to force-push — for example, to rebase a PR branch due to the GitHub UI's explicit suggestion that the branch needs to be updated:

image

...but also, it's perfectly legitimate that one might want to create a clean set of commits in a PR, especially for larger changes, which would benefit from not being squash-merged. We should at least acknowledge that this is a valid approach, even if we have decided as a community to not accept it.

@waldyrious
Copy link
Member

We should at least acknowledge that this is a valid approach, even if we have decided as a community to not accept it.

In fact, I'd go further and refrain from stating we reject this approach entirely, and make it clear it would be occasionally acceptable (and I'd say even desirable) in PRs with large/complex changes.

I would like to point out the Maintainer's Guide says about this from the other perspective:

When merging PRs, use the merge strategy that produces a clean Git history: If there's a single commit in the PR, or if the multiple commits are not semantically independent changes, use the Squash and merge method. (Don't forget to clean up the body of the squashed commit message.) If instead, the PR author took the time to craft individual, informative messages for each commit, then use the Rebase and merge method, to honor that work and preserve the history of the changes. For less clear-cut cases, a simple heuristic you can follow is that if there are more "dirty" commits than "clean" commits, then prefer squash, else do a rebase.

@Managor
Copy link
Member Author

Managor commented Nov 15, 2025

Judging if a PR would benefit from not squash merging is an additional cognitive load that I would frankly prefer not to take. It would get applied so rarely that I don't see the point of worrying myself about it.

@waldyrious
Copy link
Member

Let's say that for PRs making individual page edits or additions we can automatically assume the commits can be squashed. That would leave only PRs changing several files requiring that decision, which I take to be a minority. Would that help?

@Managor
Copy link
Member Author

Managor commented Nov 15, 2025

Not really. The page additions and edits are almost always grouped by a topic, thus they make sense to be squashed.

@SethFalco SethFalco requested a review from Copilot November 15, 2025 14:31
Copilot finished reviewing on behalf of SethFalco November 15, 2025 14:31

This comment was marked as off-topic.

@waldyrious
Copy link
Member

I fail to see where we are disagreeing. I stated precisely that page additions and edits are squashable 🤔 Can you point out more specifically what you are disagreeing with? (And, by the way, what compromise you would find acceptable?)

@Managor
Copy link
Member Author

Managor commented Nov 15, 2025

That would leave only PRs changing several files requiring that decision, which I take to be a minority.

To me this says that when a PR changes multiple files, it should not be squashed.

@waldyrious
Copy link
Member

waldyrious commented Nov 17, 2025

You are phrasing it as if it is a black-and-white rule. All I meant to suggest was that PRs modifying multiple might be worth merging without squashing — but only if the commits are indeed cleaned up and meaningfully titled, rather than stuff like "Update <file X>" or "Address comments", which IMHO should be pretty easy to spot.

I understand that verifying that is not a zero-effort task, but it's not a huge effort either. Not to mention that under this proposed guideline, PRs modifying a single file or making the same repetitive change to multiple files (which are, AFAIK, the vast majority of tldr-pages PRs) can indeed default to be squash-merged, with no commit inspection whatsoever. I feel that paying attention to the branch history in the few cases that do more complex changes is a fair ask from the maintainers, considering that the contributor would have invested their end of the effort by taking the time to tidy up the branch history.

And once more: I'm not suggesting to give contributors carte blanche to rebase and force-push willy-nilly. I still agree with the spirit of this notice. I'm just proposing that it is not phrased in absolute terms, but does provide room for exceptions where appropriate.

@SethFalco
Copy link
Member

SethFalco commented Nov 18, 2025

To me this says that when a PR changes multiple files, it should not be squashed.

You are phrasing it as if it is a black-and-white rule. All I meant to suggest was that PRs modifying multiple might be worth merging without squashing…

Please correct me if I'm wrong, but I think an example/TL;DR (no pun intended) would help here, so I'm just adding one. I follow the same approach for all PRs I'm responsible for merging, not just in tldr-pages.

If the commit history looks like this, we can respect that the user organized their changes and wrote clean commit messages by preserving their commits:

  • cat: add page
  • cat: remove example
  • cat: dutch translation

If the commit history looks like this, we'll mask the cluttered history by squashing the commits:

  • cat: add page
  • typo
  • code review fixes
  • fix linting
  • AAAAAAAAAAAAAAAAAAAAAA

CONTRIBUTING.md Outdated

> [!IMPORTANT]
> Do not force push. We would prefer to preserve commit history so that the order of events is preserved. All pull requests will be squashed so a messy pull request will not matter.
> Do not force push. We would prefer to preserve commit history within the pull request so that the order of events stays chronological. All pull requests will be squashed so the mess will be contained in the PR.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so sure this improves clarity.

For example, adding within the pull request suggests that preservation of event order matters only inside the PR, and not e.g. on the main branch.

In the same vein, the mess will be contained in the PR seems a little vague compared to the previous wording, and also conflicts with the above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, adding within the pull request suggests that preservation of event order matters only inside the PR, and not e.g. on the main branch.

I fail to see the problem here. If PR's are squashed, nobody needs to think about the main branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From that perspective, why is it that we e.g. do a traditional merge, instead of our current policy of squash+merge?

I was under the impression that it was to ensure the history on the main branch was clean and easy to understand?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these are in opposition. Anyways, I made a new commit that adjusts the wording a little bit ca43858

@github-actions github-actions bot added the review needed Prioritized PRs marked for reviews from maintainers. label Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Issues/PRs modifying the documentation. review needed Prioritized PRs marked for reviews from maintainers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants