5 ms·

I agree, the PR is invaluable to preserve context: the trunk history remains simple and linear thanks to squash merges, and you can still see the fine grained c

I agree, the PR is invaluable to preserve context: the trunk history remains simple and linear thanks to squash merges, and you can still see the fine grained commits (and matching discussions/reviews) in the closed PR. It's also a way to give insight when onboarding new developers: if something is surprising, they can see why things came to be this way, not just accept the result at face value.

someweirdperson 5y ago
Explanations should end up in comments, documents, and the commit-comment of the squash (creating a good one takes some time when squashing). The PR and original branch show dead-ends that should not be required reading to understand things.
- shawnz 5y ago
  Maybe they should not be required in the ideal state, but I don't think it's wise to design your processes to only accommodate the ideal state.
- NateEag 5y ago
  I'm personally a fan of small, clean commits, rebasing without squashing before merge, always making a merge commit, and writing a clear, through merge commit explaining what the branch does. That way, I can treat the series of merge commits to trunk as the simple, linear overview of history, but when I'm bug-hunting I get small, clear commits to search through with git bisect. It also means I get more useful blame output, as instead of huge hunks of a file being all from one giant bomb commit, I can see the process and evolution that the whole change went through as the original author pieced it together. That can be really helpful when dealing with obscure bugs or understanding systems with little documentation, by helping you get back more of the original author's thought process and seeing how their mental model evolved as they built the feature.
  - ithkuil 5y ago
    Do you ensure your clean commits all pass all CI tests?
    - hiptobecubic 5y ago
      Yes, generally. I don't really understand why anyone commits broken junk and then leaves it there.
      - lovich 5y ago
        My places test suite nukes my local development environment for the full integration tests. If I am working on a hairy piece of code I open up a PR and let the CI system farm out the suite to multiple instances so I can get an answer in less than an hour. The "right" answer is probably to refactor the test suite to be more performant, but that's never going to get approved given the amount of work that would take, and it would take me longer than I plan on being at the company to get it fixed in my spare time. I do have it passing all tests before I try to merge if that counts?
    - rectang 5y ago
      I use the same workflow as NateEag and mdavidn. My preference is: • All commits SHOULD pass all CI tests • Merge commits MUST pass all CI tests The reason I don't require every commit to pass all tests is to maximize reviewability and logical consistency of commits. For example, if a file needs to move, and then be modified slightly to function in its new location, I prefer to break that into two commits: 1. A verbatim move operation. This commit will fail CI but can be effortlessly reviewed. 2. Small modifications. This commit will pass CI, and its reviewability is proportional to its size. In contrast, if you smush those two commits together, it can be hard to see where the meaningful changes occur in one giant diff. (Some tools may be smarter about showing a minimal diff for a move-and-modify commit under limited circumstances, but that doesn't always work.)
      - bornfreddy 5y ago
        I think this is the crucial thing. Commits help with code reviews and they give hints about why some code change happened.

someweirdperson 5y ago

Explanations should end up in comments, documents, and the commit-comment of the squash (creating a good one takes some time when squashing). The PR and original branch show dead-ends that should not be required reading to understand things.

shawnz 5y ago
Maybe they should not be required in the ideal state, but I don't think it's wise to design your processes to only accommodate the ideal state.
NateEag 5y ago
I'm personally a fan of small, clean commits, rebasing without squashing before merge, always making a merge commit, and writing a clear, through merge commit explaining what the branch does. That way, I can treat the series of merge commits to trunk as the simple, linear overview of history, but when I'm bug-hunting I get small, clear commits to search through with git bisect. It also means I get more useful blame output, as instead of huge hunks of a file being all from one giant bomb commit, I can see the process and evolution that the whole change went through as the original author pieced it together. That can be really helpful when dealing with obscure bugs or understanding systems with little documentation, by helping you get back more of the original author's thought process and seeing how their mental model evolved as they built the feature.
- ithkuil 5y ago
  Do you ensure your clean commits all pass all CI tests?
  - hiptobecubic 5y ago
    Yes, generally. I don't really understand why anyone commits broken junk and then leaves it there.
    - lovich 5y ago
      My places test suite nukes my local development environment for the full integration tests. If I am working on a hairy piece of code I open up a PR and let the CI system farm out the suite to multiple instances so I can get an answer in less than an hour. The "right" answer is probably to refactor the test suite to be more performant, but that's never going to get approved given the amount of work that would take, and it would take me longer than I plan on being at the company to get it fixed in my spare time. I do have it passing all tests before I try to merge if that counts?
  - rectang 5y ago
    I use the same workflow as NateEag and mdavidn. My preference is: • All commits SHOULD pass all CI tests • Merge commits MUST pass all CI tests The reason I don't require every commit to pass all tests is to maximize reviewability and logical consistency of commits. For example, if a file needs to move, and then be modified slightly to function in its new location, I prefer to break that into two commits: 1. A verbatim move operation. This commit will fail CI but can be effortlessly reviewed. 2. Small modifications. This commit will pass CI, and its reviewability is proportional to its size. In contrast, if you smush those two commits together, it can be hard to see where the meaningful changes occur in one giant diff. (Some tools may be smarter about showing a minimal diff for a move-and-modify commit under limited circumstances, but that doesn't always work.)
    - bornfreddy 5y ago
      I think this is the crucial thing. Commits help with code reviews and they give hints about why some code change happened.