6 ms·

PR is the atomic level of work. I'd argue PR-level history (i.e. squash) is often enough and is way cleaner. Why would I care about "commit A", "change parts of

PR is the atomic level of work. I'd argue PR-level history (i.e. squash) is often enough and is way cleaner. Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.? If I want that granularity, I'd go read the original PR and the discussion that took place.

ownagefool 1y ago
Yeah, I agree with both you and the GP. There's a mess of commits that usually don't matter because mostly only the before and after level of an actual viable PR does, ergo I squash them. I'm cool with other reasonble approaches though, but I'm pretty over pointless hoops because someone says so.
- singpolyma3 1y ago
  If the commits don't matter why did you make them separate to begin with?
  - ViewTrick1002 1y ago
    ”Updated something tiny and ran CI again until it failed on some other step” Together with backing up your work. Sure you can keep amending your last commit but whenever you detour to another problem in the same PR that turns into a mess. Easier to just treat the PR as the atomic unit of work and squash away all that intermediate noise. It also ensures that CI will pass on every commit on the main branch.
    - pcthrowaway 1y ago
      A lot of engineers do what you suggest rather than `git add; git commit --amend` This is why commits are often noise. If people are using commits well, they tell a story. The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one
      - TeMPOraL 1y ago
        > The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one What do you consider correct usage of git, and why? In this very discussion, I can see at least two distinct purposes that, more often than not, are mutually exclusive: - To "tell a story" for other people - To checkpoint units of work as individual perceives them, helping them deal with interruptions (which include running out of work day). Storytelling is a skill in itself, doing it is a distinct kind of extra work, so you can't really have people use git for both at the same time. Which is where the whole commit history management idea comes from - it's to separate the two into distinct phases; first you commit for yourself, then you rework it to tell a story for others.
        pcthrowaway 1y ago
        In my usage, the story is for myself first and foremost. Telling the story helps keep me organized and helps me remember what I've done and where I'm going. I don't need to know that I fixed a typo in a comment, I need to know what the changes are overall doing. Sometimes I go down a dead end, reverse out, and leave a comment about why a different approach would be a dead end. I (and others) don't need a record of the work I did on that path, just the synthesis (an explanatory comment) There are multiple systems for structuring commits, but the commit message body content approximates to the same in all of them. The classic advice is https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html https://tbaggery.com/2008/04/19/a-note-about-git-commit-mess... , but I find https://www.conventionalcommits.org/en/v1.0.0/ https://www.conventionalcommits.org/en/v1.0.0/ useful for looking at the oneline log To address this point: > To checkpoint units of work as individual perceives them, helping them deal with interruptions (which include running out of work day). Yes, commits can be used like this! But once you have a chunk of work ready for review, cleaning up the commit log/history, grouping related changes, and describing them is useful for maintaining the software. I don't like squash merges personally, though they have their merits. But regardless, I would copy the commit subject/body content into the PR message, which then puts everything into the PR commit also, so technically the granular commits are less relevant when one merges, but occasionally are still useful to refer to
  - sfn42 1y ago
    I'm not the person you asked, but I often don't. I personally don't care about git history - I read code not commit messages. I don't really care how it got the way it is I care about what it is, maybe once in a blue moon there could potentially be some useful information in a commit message but it's not enough. So, I'll often just make lots of changes and then commit them all at once with some vague commit message and that's that. Nobody cares. If I want to tell people why the code is the way it is I'll just add a comment. This is how I work. I have tried to be more disciplined with commits and stuff like that but I find that it just slows me down and makes the work feel more difficult. I also frequently just forget and find myself having made lots of changes without any commits so then I have to retroactively split it up into commits which can be difficult too. So I'd rather just not worry about it, focus on getting good work done and move on rather than obsess over a git history that's unlikely to ever be read by anyone. I realize that's a self-fulfilling prophecy in that it'd be more likely to be read if it was useful and well done but it's not just me. If I was in a team where everyone did it really well I'd try to keep my own work up to par. But usually I'm the one who cares most about how we do things and this just doesn't seem important to me.
    - imiric 1y ago
      > I personally don't care about git history - I read code not commit messages. Honest question: why do you even use version control? What do you get out of it? Based on your workflow, you could just as well not use it at all, and create zip files and multiple copies of files with names like `_final3_working_20250925`. Change history is the entire point of version control. It gives you the ability to revert to a specific point in time, to branch off and work on experiments, to track down the source of issues, and, perhaps most useful of all, to see why a specific change was done. A commit gives you the ability to add metadata to a change that would be out of place in the code itself. This is often the best place to describe why the change was done, and include any background or pertinent information that can help future developers—including yourself—in many ways. Adding this to the code base in a comment or another document would be out of place, and difficult to manage and discover. You may rarely need to use these abilities, but when you do, they are invaluable IME. And if you don't have them at that point, you'll be kicking yourself for not leveraging a VCS to its full potential. > I have tried to be more disciplined with commits and stuff like that but I find that it just slows me down and makes the work feel more difficult. Of course it slows you down. Taking care of development history requires time and effort, but the ROI of doing that is many times greater. I encourage you to try to be disciplined for a while, and see how you feel about it. I use conventional commits and create atomic commits with descriptive messages even on personal projects that nobody else will work on. Mainly because it gives me the chance to reflect on changes, and include information that will be useful to my future self, months or years from now, after I inevitably abandon the project and eventually get back to it. Here is an example from a project I was working on recently[1]. It's practically a blog post, but it felt good to air my grievances. :) [1]: https://github.com/hackfixme/sesame/commit/10cd8597559b5f4785c11d693ae03a3f8c063d48 https://github.com/hackfixme/sesame/commit/10cd8597559b5f478...
imiric 1y ago
You should care because if the author cared enough to make descriptive atomic commits, they will help you understand why a particular change was done. This can often avoid unnecessary discussions. And, no, PRs are not necessarily an atomic level of work. While they should contain a single feature, fix, etc., sometimes that work can span multiple commits. If the PR includes superfluous commits, then they should be squashed into the appropriate commit. Squashing the entire PR when it includes multiple changes is simply a bad practice. It's bad because you lose all the history of how the overall change was done, which will be useful in the future when you need to do a blame, cherry pick, bisect, etc. It's surprising to me how many developers misunderstand the value of atomic commits, or even what they are. And at the same time, it's exhausting having this discussion every time that happens, especially if there is continued pushback. I am not against people having their preferred way of using VCS tools. As long as it works for their team, that's fine. But there are certain best practices that simply help everyone in the long-term, including the author, that I'm baffled whenever they're willfully ignored. I can't help but think that it's often done out of laziness, and lack of discipline and care into the work they do, which somehow becomes part of their persona as they gain more experience.
- mvdtnz 1y ago
  > You should care because if the author cared enough to make descriptive atomic commits, they will help you understand why a particular change was done. This can often avoid unnecessary discussions. That's what the description field is for. I never, ever inspect the "commits" tab in a PR unless I see some lucicrous number on it. And even then it's just to see what the heck happened. > If the PR includes superfluous commits, then they should be squashed into the appropriate commit. This happens on merge if your Github is set up correctly. > Squashing the entire PR when it includes multiple changes is simply a bad practice. The bad practice is the PR changing multiple distinct things. > It's bad because you lose all the history of how the overall change was done, which will be useful in the future when you need to do a blame, cherry pick, bisect, etc. It's not.
  - imiric 1y ago
    > That's what the description field is for. No. The PR description is for describing the overall change, which, again, may include multiple commits. The description can also include testing instructions, reviewing suggestions, and other information which is not suitable for a commit message. PR descriptions can be edited and updated during the review, which can be helpful. A commit message is immutable, and remains as a historical artifact. Also, when I'm working on a code base, the last thing I want is to go hunting for PRs to get context about a specific change. The commit message should have all the information I need directly in the repo. > I never, ever inspect the "commits" tab in a PR unless I see some lucicrous number on it. And... you're actually proud of this? Amazing. Have you ever read a descriptive commit message? Do you even know what they look like? I'm taken aback by the idea that there are developers who would take the time and effort to write a detailed commit message, only for others to not only never read it, but to be proud of that fact. Disgraceful. > This happens on merge if your Github is set up correctly. No. This is what I mean about developers not understanding what atomic commits even are. There are commits that will be done during a review, or as ad-hoc fixes, which indeed shouldn't exist when the PR is merged. But this doesn't mean that the entire PR should be squashed into a single commit. Those useless commits should instead be squashed into the most relevant commit, which is straightforward if you create `--fixup` commits which can then be automatically squashed with `rebase --autosquash`. But the PR may ultimately end up with multiple atomic commits, and squashing them all into a single commit would nullify the hard work the author did to keep them atomic in the first place. If you configure GitHub to always squash PRs, or to always create a merge commit, or to always rebase, you're doing it wrong. Instead, these are decisions that should be made on a case-by-case basis for each PR. There are situations when either one of them is the best approach. > The bad practice is the PR changing multiple distinct things. Right. I'm sure you enjoy the overhead of dealing with a flood of small PRs that are all related to a single change, when all of it could be done in a single PR with multiple commits. This is easier to review, discuss, and merge as a single unit, rather than have it spread out over multiple PRs because of a strict "one PR-one commit" policy. All that rule does, especially if you have PR squashing enabled by default, is create a history of bloated commits with thousands of lines of unrelated changes, that are practically useless for cherry picking, bisecting, and determining why a specific change was done, which is the entire point of commits. Good luck working on that codebase. > It's not. k.
singpolyma3 1y ago
The PR UI on GitHub definitely leads to treating it as the unit of work. I consider this unfortunate for the most part, but the basic side effect is that I'll often end up submitting every commit as a separate PR so they actually get looked at
- William_BB 1y ago
  Respectfully, I disagree. I understand people have vastly different experiences and preferences. In my ideal world, a PR is a unit of work that has an in-state and an out-state. I don't have to see an initial commit within a PR with a full fledged spec and then wonder if any future commits within the same PR overrode those changes. A PR will rarely be clean from the start. The way it appears to me, if there's multiple commits submitted as separate PRs, then maybe the PR wasn't so atomic to begin with.
  - singpolyma3 1y ago
    Indeed, I agree. If a PR has multiple commits and those commits are atomic then by definition the PR is not atomic.
DSMan195276 1y ago
> I'd go read the original PR and the discussion that took place. Until your company switches code repos multiple times and all the PR history is gone or hard/impossible to track down. I will say, I don't usually make people clean up their commits and also usually recommend squashing PRs for any teams that aren't comfortable with `git`. When people do take the time to make a sensible commit history (when a PR warrants more than one commit) it makes looking back through their code history to understand what was going on 1000% easier. It also forces people to actually look over all of their changes, which is something I find a lot of people don't bother to do and their code quality suffers a lot as a result.
- agentultra 1y ago
  It also enables bisect to work properly. Bisecting on squashed commits is not usually helpful. You can still narrow down to the offending commit that introduced the error but… it’s somewhere in +1200/-325 lines of change. Good luck.
  - siva7 1y ago
    If your PR is + 1000 code lines long, you already made a mistake at the requirements and planning stage (like many teams do).
    - Freedom2 1y ago
      I don't agree, for example my team includes yarn.lock in the commit which adds quite a few lines to the PR.
    - sigseg1v 1y ago
      This sounds unattainable to me. For code bases in the 2 million or more lines range, something as simple as refactoring the name of a poorly named base class can hit 5000 lines. It also was not a mistake with the original name it had, but you'd still like to change it to make it more readable given the evolution of the codebase. You would not split that up into multiple commits because that would be a mess and it would not compile unless done in one commit. How is this a mistake?
jonahx 1y ago
> PR is the atomic level of work. The atomic level of work should be a single, logically coherent change to the codebase. It's not managerial, it's explanatory. As you work things naturally arise. Over here a reformatted file, over there comments to clarify an old function that confused you, to help the next developer who encounters it. Cleaning, preparatory refactoring that is properly viewed as a separate action, and so on. Each of these is a distinct "operation" on the codebase, and should be reviewed in isolation, as a commit. Some of these operations have nothing to do with the new feature you're adding. And yet creating separate PRs for each of them would be onerous to your reviewers and spammy. Clean, atomic history lets you work naturally while still telling a clear story about how the code changed, both for reviewers and future developers.
- BurningFrog 1y ago
  These opinions could use some arguments for why they're useful.
- lucyjojo 1y ago
  maybe if the majority of devs switched to jj or something similar your idea might become semi-common. but it seems to me that currently for most most project, it generates a lot of cost for very little gain. My personal idea is that the code in each PR should be well documented enough for a review, but also for when people join the team and need to learn. Or when a pour soul needs to check upon your code while you are out on a holiday. This personal rule does not apply to all projects, but for bread and butter stuff I tend to go by it and not care about clean commit histories. The cost to reward seems way better.
- bccdee 1y ago
  > The atomic level of work should be a single, logically coherent change to the codebase. Sure, and we must make that correspond to the atomic unit that our collaboration tools provide us for reviewing and merging. In Github and similar git forges, that's a PR, not as a commit. A string of atomic changes should be represented as a series of PRs, not a series of commits in one PR, because Github isn't designed to review and merge individual commits. The "atomic commits" crowd are (in my opinion) coming up with best practices for the tools they wish they had and working against the grain of the tools we actually use.
illuminator83 1y ago
I always tell my engineers to create atomic commits and we usually review commit by commit. Obviously commits like "fixed review comments" or "removed some left-over comments" or "fixed typo" should not be pushed into a PR you asked others to review. I expect people to understand how to clean their commit history - if they don't I teach them. The senior people who are capable of structured work - e.g. are used to contribute open source projects - do it anyway. Because messiness is usually not tolerated by maintainers of important projects. You find people how aren't able to craft clean commits and PRs usually thrive in environments in which people are either work mostly alone or in which cooperation is enforced by external circumstances (like being in the same team in a company). As soon as developers many are free to choose whom to associate with and whose code they accept - rules are usually made and enforced.
- nitwit005 1y ago
  The reason for that tidiness with some open source projects is because they want to conserve the valuable time of the maintainers, and are willing to expend a lot of time from people wanting changes to do that. That's not the situation in a normal corporate environment. You want to reduce total time expended (or total cost, at least). It's going to be cheaper to just have a chat with your coworker when a PR is confusing.
- sigseg1v 1y ago
  > Obviously commits like "fixed review comments" or "removed some left-over comments" or "fixed typo" should not be pushed into a PR you asked others to review. Could you explain this a bit more? I'm having trouble visualizing the end to end process. 1. Someone has what they feel is a complete change and submits a PR for review. 2. The reviewers read part of it, first half looks good, and halfway through they have concerns and request changes. 3. The submitter now has to fix those concerns. If they are not allowed to push an additional commit to do this, how do you propose they accomplish this? Certainly they should not force push a public branch, right? That would cause pain to any reviewer who fetched their changes, and also break the features on GitHub such as them marking which files they have already read in the UI. But if we cannot push new commits and we cannot edit the existing commits, what is the method you are suggesting?
themk 1y ago
> Why would I care about ... You would not allow those commits. Code review improvements should appear as fixup commits which should be autosquashed on merge. It is a shame that GitHub does not support autosquash though.
- deleted 1y ago
  [deleted]
- krzyk 1y ago
  Not sure, squashing is hiding history, I prefer to see history, even if it is not clean or buildable.
matheusmoreira 1y ago
> "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc. People are supposed to rebase all that noise away. Changes are supposed to be structured as sensible chunks that build up to the desired feature. It's like showing your work in a math exercise: you don't write out the final answer with no explanation, you demonstrate step by step how you reached it.
porridgeraisin 1y ago
I actually prefer to not be dogmatic about one approach or the other, and I think the answer will be different for different dev/ci/deployment workflows. Here's my approach, of course from experience limited to my (past) workplace. We have the usual CI setup, where each merged PR triggers a build followed by a deploy to staging. This means that what goes in a PR is decided by what sub-functionality of the feature at hand has to be tested first[0], whereas what goes in commits is decided by what is easy to read for reviewers for PRs where such an approach makes sense [1], or it simply doesn't matter much like you said, for a lot of other cases. That is the way I like to think about it. I know I know git bisect etc... but IME in the rare cases we used it we just ran bisect on the master branch which had squashed PR level commits, and once we found the PR, it was fairly straightforward to manually narrow it down after that. In more systems level projects there will actually be clear layers for different parts of your code (let's be honest, business logic apps are not structured that well, especially as time goes) and the individual-commits-should-work approach works well. [0] ideally, the whole feature is one PR and you config-gate each behaviour to test individually, but that's not always possible. [1] for example, let's say we have a kafka producer and consumer in the same repo. They each call a myriad of internal business logic functions, and modifications were required there too. It is much easier to read commits separating changes, even to the same function, made as part of the producer flow and the consumer flow.
phatskat 1y ago
> Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.? For me it’s because Feature A may largely be fine but one of those intermediary commits introduced a regression. I can bisect and isolate an issue much more easily if I have the full history to step through as opposed to “this big commit intrigued a one-line regression _somewhere_ in a 900 line commit”
rurban 1y ago
I never work with squashers. There's no open source project I know of, which accepts squashers. Many companies do so, because their management is fucked, and if you see such a thing, look for another job immediately.
- whatevaa 1y ago
  Baseless claim, just your opinion.

ownagefool 1y ago

Yeah, I agree with both you and the GP. There's a mess of commits that usually don't matter because mostly only the before and after level of an actual viable PR does, ergo I squash them. I'm cool with other reasonble approaches though, but I'm pretty over pointless hoops because someone says so.

singpolyma3 1y ago
If the commits don't matter why did you make them separate to begin with?
- ViewTrick1002 1y ago
  ”Updated something tiny and ran CI again until it failed on some other step” Together with backing up your work. Sure you can keep amending your last commit but whenever you detour to another problem in the same PR that turns into a mess. Easier to just treat the PR as the atomic unit of work and squash away all that intermediate noise. It also ensures that CI will pass on every commit on the main branch.
  - pcthrowaway 1y ago
    A lot of engineers do what you suggest rather than `git add; git commit --amend` This is why commits are often noise. If people are using commits well, they tell a story. The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one
    - TeMPOraL 1y ago
      > The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one What do you consider correct usage of git, and why? In this very discussion, I can see at least two distinct purposes that, more often than not, are mutually exclusive: - To "tell a story" for other people - To checkpoint units of work as individual perceives them, helping them deal with interruptions (which include running out of work day). Storytelling is a skill in itself, doing it is a distinct kind of extra work, so you can't really have people use git for both at the same time. Which is where the whole commit history management idea comes from - it's to separate the two into distinct phases; first you commit for yourself, then you rework it to tell a story for others.
      - pcthrowaway 1y ago
        In my usage, the story is for myself first and foremost. Telling the story helps keep me organized and helps me remember what I've done and where I'm going. I don't need to know that I fixed a typo in a comment, I need to know what the changes are overall doing. Sometimes I go down a dead end, reverse out, and leave a comment about why a different approach would be a dead end. I (and others) don't need a record of the work I did on that path, just the synthesis (an explanatory comment) There are multiple systems for structuring commits, but the commit message body content approximates to the same in all of them. The classic advice is https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html https://tbaggery.com/2008/04/19/a-note-about-git-commit-mess... , but I find https://www.conventionalcommits.org/en/v1.0.0/ https://www.conventionalcommits.org/en/v1.0.0/ useful for looking at the oneline log To address this point: > To checkpoint units of work as individual perceives them, helping them deal with interruptions (which include running out of work day). Yes, commits can be used like this! But once you have a chunk of work ready for review, cleaning up the commit log/history, grouping related changes, and describing them is useful for maintaining the software. I don't like squash merges personally, though they have their merits. But regardless, I would copy the commit subject/body content into the PR message, which then puts everything into the PR commit also, so technically the granular commits are less relevant when one merges, but occasionally are still useful to refer to
- sfn42 1y ago
  I'm not the person you asked, but I often don't. I personally don't care about git history - I read code not commit messages. I don't really care how it got the way it is I care about what it is, maybe once in a blue moon there could potentially be some useful information in a commit message but it's not enough. So, I'll often just make lots of changes and then commit them all at once with some vague commit message and that's that. Nobody cares. If I want to tell people why the code is the way it is I'll just add a comment. This is how I work. I have tried to be more disciplined with commits and stuff like that but I find that it just slows me down and makes the work feel more difficult. I also frequently just forget and find myself having made lots of changes without any commits so then I have to retroactively split it up into commits which can be difficult too. So I'd rather just not worry about it, focus on getting good work done and move on rather than obsess over a git history that's unlikely to ever be read by anyone. I realize that's a self-fulfilling prophecy in that it'd be more likely to be read if it was useful and well done but it's not just me. If I was in a team where everyone did it really well I'd try to keep my own work up to par. But usually I'm the one who cares most about how we do things and this just doesn't seem important to me.
  - imiric 1y ago
    > I personally don't care about git history - I read code not commit messages. Honest question: why do you even use version control? What do you get out of it? Based on your workflow, you could just as well not use it at all, and create zip files and multiple copies of files with names like `_final3_working_20250925`. Change history is the entire point of version control. It gives you the ability to revert to a specific point in time, to branch off and work on experiments, to track down the source of issues, and, perhaps most useful of all, to see why a specific change was done. A commit gives you the ability to add metadata to a change that would be out of place in the code itself. This is often the best place to describe why the change was done, and include any background or pertinent information that can help future developers—including yourself—in many ways. Adding this to the code base in a comment or another document would be out of place, and difficult to manage and discover. You may rarely need to use these abilities, but when you do, they are invaluable IME. And if you don't have them at that point, you'll be kicking yourself for not leveraging a VCS to its full potential. > I have tried to be more disciplined with commits and stuff like that but I find that it just slows me down and makes the work feel more difficult. Of course it slows you down. Taking care of development history requires time and effort, but the ROI of doing that is many times greater. I encourage you to try to be disciplined for a while, and see how you feel about it. I use conventional commits and create atomic commits with descriptive messages even on personal projects that nobody else will work on. Mainly because it gives me the chance to reflect on changes, and include information that will be useful to my future self, months or years from now, after I inevitably abandon the project and eventually get back to it. Here is an example from a project I was working on recently[1]. It's practically a blog post, but it felt good to air my grievances. :) [1]: https://github.com/hackfixme/sesame/commit/10cd8597559b5f4785c11d693ae03a3f8c063d48 https://github.com/hackfixme/sesame/commit/10cd8597559b5f478...

imiric 1y ago

You should care because if the author cared enough to make descriptive atomic commits, they will help you understand why a particular change was done. This can often avoid unnecessary discussions. And, no, PRs are not necessarily an atomic level of work. While they should contain a single feature, fix, etc., sometimes that work can span multiple commits. If the PR includes superfluous commits, then they should be squashed into the appropriate commit. Squashing the entire PR when it includes multiple changes is simply a bad practice. It's bad because you lose all the history of how the overall change was done, which will be useful in the future when you need to do a blame, cherry pick, bisect, etc. It's surprising to me how many developers misunderstand the value of atomic commits, or even what they are. And at the same time, it's exhausting having this discussion every time that happens, especially if there is continued pushback. I am not against people having their preferred way of using VCS tools. As long as it works for their team, that's fine. But there are certain best practices that simply help everyone in the long-term, including the author, that I'm baffled whenever they're willfully ignored. I can't help but think that it's often done out of laziness, and lack of discipline and care into the work they do, which somehow becomes part of their persona as they gain more experience.

mvdtnz 1y ago
> You should care because if the author cared enough to make descriptive atomic commits, they will help you understand why a particular change was done. This can often avoid unnecessary discussions. That's what the description field is for. I never, ever inspect the "commits" tab in a PR unless I see some lucicrous number on it. And even then it's just to see what the heck happened. > If the PR includes superfluous commits, then they should be squashed into the appropriate commit. This happens on merge if your Github is set up correctly. > Squashing the entire PR when it includes multiple changes is simply a bad practice. The bad practice is the PR changing multiple distinct things. > It's bad because you lose all the history of how the overall change was done, which will be useful in the future when you need to do a blame, cherry pick, bisect, etc. It's not.
- imiric 1y ago
  > That's what the description field is for. No. The PR description is for describing the overall change, which, again, may include multiple commits. The description can also include testing instructions, reviewing suggestions, and other information which is not suitable for a commit message. PR descriptions can be edited and updated during the review, which can be helpful. A commit message is immutable, and remains as a historical artifact. Also, when I'm working on a code base, the last thing I want is to go hunting for PRs to get context about a specific change. The commit message should have all the information I need directly in the repo. > I never, ever inspect the "commits" tab in a PR unless I see some lucicrous number on it. And... you're actually proud of this? Amazing. Have you ever read a descriptive commit message? Do you even know what they look like? I'm taken aback by the idea that there are developers who would take the time and effort to write a detailed commit message, only for others to not only never read it, but to be proud of that fact. Disgraceful. > This happens on merge if your Github is set up correctly. No. This is what I mean about developers not understanding what atomic commits even are. There are commits that will be done during a review, or as ad-hoc fixes, which indeed shouldn't exist when the PR is merged. But this doesn't mean that the entire PR should be squashed into a single commit. Those useless commits should instead be squashed into the most relevant commit, which is straightforward if you create `--fixup` commits which can then be automatically squashed with `rebase --autosquash`. But the PR may ultimately end up with multiple atomic commits, and squashing them all into a single commit would nullify the hard work the author did to keep them atomic in the first place. If you configure GitHub to always squash PRs, or to always create a merge commit, or to always rebase, you're doing it wrong. Instead, these are decisions that should be made on a case-by-case basis for each PR. There are situations when either one of them is the best approach. > The bad practice is the PR changing multiple distinct things. Right. I'm sure you enjoy the overhead of dealing with a flood of small PRs that are all related to a single change, when all of it could be done in a single PR with multiple commits. This is easier to review, discuss, and merge as a single unit, rather than have it spread out over multiple PRs because of a strict "one PR-one commit" policy. All that rule does, especially if you have PR squashing enabled by default, is create a history of bloated commits with thousands of lines of unrelated changes, that are practically useless for cherry picking, bisecting, and determining why a specific change was done, which is the entire point of commits. Good luck working on that codebase. > It's not. k.

singpolyma3 1y ago

The PR UI on GitHub definitely leads to treating it as the unit of work. I consider this unfortunate for the most part, but the basic side effect is that I'll often end up submitting every commit as a separate PR so they actually get looked at

William_BB 1y ago
Respectfully, I disagree. I understand people have vastly different experiences and preferences. In my ideal world, a PR is a unit of work that has an in-state and an out-state. I don't have to see an initial commit within a PR with a full fledged spec and then wonder if any future commits within the same PR overrode those changes. A PR will rarely be clean from the start. The way it appears to me, if there's multiple commits submitted as separate PRs, then maybe the PR wasn't so atomic to begin with.
- singpolyma3 1y ago
  Indeed, I agree. If a PR has multiple commits and those commits are atomic then by definition the PR is not atomic.

DSMan195276 1y ago

> I'd go read the original PR and the discussion that took place. Until your company switches code repos multiple times and all the PR history is gone or hard/impossible to track down. I will say, I don't usually make people clean up their commits and also usually recommend squashing PRs for any teams that aren't comfortable with `git`. When people do take the time to make a sensible commit history (when a PR warrants more than one commit) it makes looking back through their code history to understand what was going on 1000% easier. It also forces people to actually look over all of their changes, which is something I find a lot of people don't bother to do and their code quality suffers a lot as a result.

agentultra 1y ago
It also enables bisect to work properly. Bisecting on squashed commits is not usually helpful. You can still narrow down to the offending commit that introduced the error but… it’s somewhere in +1200/-325 lines of change. Good luck.
- siva7 1y ago
  If your PR is + 1000 code lines long, you already made a mistake at the requirements and planning stage (like many teams do).
  - Freedom2 1y ago
    I don't agree, for example my team includes yarn.lock in the commit which adds quite a few lines to the PR.
  - sigseg1v 1y ago
    This sounds unattainable to me. For code bases in the 2 million or more lines range, something as simple as refactoring the name of a poorly named base class can hit 5000 lines. It also was not a mistake with the original name it had, but you'd still like to change it to make it more readable given the evolution of the codebase. You would not split that up into multiple commits because that would be a mess and it would not compile unless done in one commit. How is this a mistake?

jonahx 1y ago

> PR is the atomic level of work. The atomic level of work should be a single, logically coherent change to the codebase. It's not managerial, it's explanatory. As you work things naturally arise. Over here a reformatted file, over there comments to clarify an old function that confused you, to help the next developer who encounters it. Cleaning, preparatory refactoring that is properly viewed as a separate action, and so on. Each of these is a distinct "operation" on the codebase, and should be reviewed in isolation, as a commit. Some of these operations have nothing to do with the new feature you're adding. And yet creating separate PRs for each of them would be onerous to your reviewers and spammy. Clean, atomic history lets you work naturally while still telling a clear story about how the code changed, both for reviewers and future developers.

BurningFrog 1y ago
These opinions could use some arguments for why they're useful.
lucyjojo 1y ago
maybe if the majority of devs switched to jj or something similar your idea might become semi-common. but it seems to me that currently for most most project, it generates a lot of cost for very little gain. My personal idea is that the code in each PR should be well documented enough for a review, but also for when people join the team and need to learn. Or when a pour soul needs to check upon your code while you are out on a holiday. This personal rule does not apply to all projects, but for bread and butter stuff I tend to go by it and not care about clean commit histories. The cost to reward seems way better.
bccdee 1y ago
> The atomic level of work should be a single, logically coherent change to the codebase. Sure, and we must make that correspond to the atomic unit that our collaboration tools provide us for reviewing and merging. In Github and similar git forges, that's a PR, not as a commit. A string of atomic changes should be represented as a series of PRs, not a series of commits in one PR, because Github isn't designed to review and merge individual commits. The "atomic commits" crowd are (in my opinion) coming up with best practices for the tools they wish they had and working against the grain of the tools we actually use.

illuminator83 1y ago

I always tell my engineers to create atomic commits and we usually review commit by commit. Obviously commits like "fixed review comments" or "removed some left-over comments" or "fixed typo" should not be pushed into a PR you asked others to review. I expect people to understand how to clean their commit history - if they don't I teach them. The senior people who are capable of structured work - e.g. are used to contribute open source projects - do it anyway. Because messiness is usually not tolerated by maintainers of important projects. You find people how aren't able to craft clean commits and PRs usually thrive in environments in which people are either work mostly alone or in which cooperation is enforced by external circumstances (like being in the same team in a company). As soon as developers many are free to choose whom to associate with and whose code they accept - rules are usually made and enforced.

nitwit005 1y ago
The reason for that tidiness with some open source projects is because they want to conserve the valuable time of the maintainers, and are willing to expend a lot of time from people wanting changes to do that. That's not the situation in a normal corporate environment. You want to reduce total time expended (or total cost, at least). It's going to be cheaper to just have a chat with your coworker when a PR is confusing.
sigseg1v 1y ago
> Obviously commits like "fixed review comments" or "removed some left-over comments" or "fixed typo" should not be pushed into a PR you asked others to review. Could you explain this a bit more? I'm having trouble visualizing the end to end process. 1. Someone has what they feel is a complete change and submits a PR for review. 2. The reviewers read part of it, first half looks good, and halfway through they have concerns and request changes. 3. The submitter now has to fix those concerns. If they are not allowed to push an additional commit to do this, how do you propose they accomplish this? Certainly they should not force push a public branch, right? That would cause pain to any reviewer who fetched their changes, and also break the features on GitHub such as them marking which files they have already read in the UI. But if we cannot push new commits and we cannot edit the existing commits, what is the method you are suggesting?

themk 1y ago

> Why would I care about ... You would not allow those commits. Code review improvements should appear as fixup commits which should be autosquashed on merge. It is a shame that GitHub does not support autosquash though.

deleted 1y ago
[deleted]
krzyk 1y ago
Not sure, squashing is hiding history, I prefer to see history, even if it is not clean or buildable.

matheusmoreira 1y ago

> "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc. People are supposed to rebase all that noise away. Changes are supposed to be structured as sensible chunks that build up to the desired feature. It's like showing your work in a math exercise: you don't write out the final answer with no explanation, you demonstrate step by step how you reached it.

porridgeraisin 1y ago

I actually prefer to not be dogmatic about one approach or the other, and I think the answer will be different for different dev/ci/deployment workflows. Here's my approach, of course from experience limited to my (past) workplace. We have the usual CI setup, where each merged PR triggers a build followed by a deploy to staging. This means that what goes in a PR is decided by what sub-functionality of the feature at hand has to be tested first[0], whereas what goes in commits is decided by what is easy to read for reviewers for PRs where such an approach makes sense [1], or it simply doesn't matter much like you said, for a lot of other cases. That is the way I like to think about it. I know I know git bisect etc... but IME in the rare cases we used it we just ran bisect on the master branch which had squashed PR level commits, and once we found the PR, it was fairly straightforward to manually narrow it down after that. In more systems level projects there will actually be clear layers for different parts of your code (let's be honest, business logic apps are not structured that well, especially as time goes) and the individual-commits-should-work approach works well. [0] ideally, the whole feature is one PR and you config-gate each behaviour to test individually, but that's not always possible. [1] for example, let's say we have a kafka producer and consumer in the same repo. They each call a myriad of internal business logic functions, and modifications were required there too. It is much easier to read commits separating changes, even to the same function, made as part of the producer flow and the consumer flow.

phatskat 1y ago

> Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.? For me it’s because Feature A may largely be fine but one of those intermediary commits introduced a regression. I can bisect and isolate an issue much more easily if I have the full history to step through as opposed to “this big commit intrigued a one-line regression _somewhere_ in a 900 line commit”

rurban 1y ago

I never work with squashers. There's no open source project I know of, which accepts squashers. Many companies do so, because their management is fucked, and if you see such a thing, look for another job immediately.

whatevaa 1y ago
Baseless claim, just your opinion.