6 ms·

Also, if your commit message has parts that say "also, this", "also, that", don't be lazy and break those alsos into separate commits!

by pferde 5y ago

jacobmischka 5y ago
Not necessarily. It sounds like all of the things mentioned in the commit message are interconnected (updating build scripts and updating gitignore to remove the new build directories). It would be pointless to break these into separate commits, you can't revert just one without reverting the others. The commit message is lazy, but breaking these into separate commits because of some "also" rule of thumb is just blindly following a rule for the sake of it.
- capableweb 5y ago
  > breaking these into separate commits because of some "also" rule of thumb is just blindly following a rule for the sake of it Agreed, cargo-culting is always bad! But, without knowing how OBS development team works, sometimes it does make sense to split commits like this, mainly if you have a larger repository where different people are responsible for different areas in the codebase. For example, build-scripts sometimes gets managed by other people than the ones writing some core algorithms, who are also different from the people writing public APIs to be consumed by users. In those cases, it makes reviews a lot easier when you can split out to review different commits to different people, instead of having everyone navigate the same commit. But again, not familiar with the internals nor the development cycle that OBS keeps, so maybe this doesn't make sense in this case.
  - threeseed 5y ago
    As others have mentioned many times here. This commit is based off a branch that was adding support for Apple Silicon. So the entire PR is not going to look clean until that branch is merged first.
    - capableweb 5y ago
      Yes, makes a lot of sense. Sorry if I gave the impression that I think somehow this PR is wrong or similar. I was just replying to the fact that sometimes splitting up commits makes sense, outside of this PR.
  - PostThisTooFast 5y ago
- rlpb 5y ago
  My exception to the "also" rule of thumb is that if the split changes would cover separate areas of the code such that each piece could be easily picked out by "git add -p", then splitting them out isn't required as long as they are all associated to one logical larger story. > ...you can't revert just one without reverting the others That's often true in a split out patchset anyway, so I don't think that on its own it stands as a justification for doing things one way or the other.
Cthulhu_ 5y ago
Dubious. I prefer that style myself, but there's a lot of value in squashing multiple commits around a feature into a single, "this commit implements this feature" commit. Is it valuable to retain the entire development history of a feature, or just the feature itself? What if during the development, it was rewritten a dozen times? What if there's a dozen commits where the author meandered and only in the last three commits got to the point? Ruthless interactive rebase is one way to fix that last point, but if you take the final result and squash that into a single changeset, there's a point to be made about that as well.
- pferde 5y ago
  Agreed, it's a matter of preference, but I prefer logically separate changes to be in separate commits at least within the PR, where it may make reviewing the PR easier. It's basic empathy towards the reviewer(s). Of course, when merged, it often makes sense to squash some commits together, just like you say - long-term, many of the feature development details are not important. It's not an either-or proposition.

Also, if your commit message has parts that say "also, this", "also, that", don't be lazy and break those alsos into separate commits!

by pferde 5y ago

Also, if your commit message has parts that say "also, this", "also, that", don't be lazy and break those alsos into separate commits!

jacobmischka 5y ago

Not necessarily. It sounds like all of the things mentioned in the commit message are interconnected (updating build scripts and updating gitignore to remove the new build directories). It would be pointless to break these into separate commits, you can't revert just one without reverting the others. The commit message is lazy, but breaking these into separate commits because of some "also" rule of thumb is just blindly following a rule for the sake of it.

capableweb 5y ago
> breaking these into separate commits because of some "also" rule of thumb is just blindly following a rule for the sake of it Agreed, cargo-culting is always bad! But, without knowing how OBS development team works, sometimes it does make sense to split commits like this, mainly if you have a larger repository where different people are responsible for different areas in the codebase. For example, build-scripts sometimes gets managed by other people than the ones writing some core algorithms, who are also different from the people writing public APIs to be consumed by users. In those cases, it makes reviews a lot easier when you can split out to review different commits to different people, instead of having everyone navigate the same commit. But again, not familiar with the internals nor the development cycle that OBS keeps, so maybe this doesn't make sense in this case.
- threeseed 5y ago
  As others have mentioned many times here. This commit is based off a branch that was adding support for Apple Silicon. So the entire PR is not going to look clean until that branch is merged first.
  - capableweb 5y ago
    Yes, makes a lot of sense. Sorry if I gave the impression that I think somehow this PR is wrong or similar. I was just replying to the fact that sometimes splitting up commits makes sense, outside of this PR.
- PostThisTooFast 5y ago
rlpb 5y ago
My exception to the "also" rule of thumb is that if the split changes would cover separate areas of the code such that each piece could be easily picked out by "git add -p", then splitting them out isn't required as long as they are all associated to one logical larger story. > ...you can't revert just one without reverting the others That's often true in a split out patchset anyway, so I don't think that on its own it stands as a justification for doing things one way or the other.

Cthulhu_ 5y ago

Dubious. I prefer that style myself, but there's a lot of value in squashing multiple commits around a feature into a single, "this commit implements this feature" commit. Is it valuable to retain the entire development history of a feature, or just the feature itself? What if during the development, it was rewritten a dozen times? What if there's a dozen commits where the author meandered and only in the last three commits got to the point? Ruthless interactive rebase is one way to fix that last point, but if you take the final result and squash that into a single changeset, there's a point to be made about that as well.

pferde 5y ago
Agreed, it's a matter of preference, but I prefer logically separate changes to be in separate commits at least within the PR, where it may make reviewing the PR easier. It's basic empathy towards the reviewer(s). Of course, when merged, it often makes sense to squash some commits together, just like you say - long-term, many of the feature development details are not important. It's not an either-or proposition.