9 ms·

Not necessarily. It sounds like all of the things mentioned in the commit message are interconnected (updating build scripts and updating gitignore to remove th

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.

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.