5 ms·

Agreed. The first and last place I look at when doing a git blame is the PR that the commit was in. That contains all the useful information for me, as well as

by stnmtn 4y ago

Agreed. The first and last place I look at when doing a git blame is the PR that the commit was in. That contains all the useful information for me, as well as much-needed context around review comments, discussion, etc that is not able to find in native git.

lelandfe 4y ago
Pull requests vanish when repos change hands. If you leave unique information in PRs, that information may be lost in the future. This has happened to me at 3 different companies now, where we inherited another company's code base. Keeping commits self-contained is the only way to future proof your explanations.
- aulin 4y ago
  Also PRs belong to the review and CI system, everything needed to understand the code and its history should be staged in version control
  - darkwater 4y ago
    I agree and this sounds super neat but in reality it's pretty complicated to keep that boundary and not leak info from one side to the other. I mean, when you open a PR you will probably describe the rationale for the change and why you had to touch files A,B & C (if it's not obvious). You can and you should replicate that info in a multi-line commit message but sometimes through code review you reshape the code structure or even some of the requirements (most probably because there was some initial misunderstanding). So, since we are humans, it's normal to take shortcuts when we did already "a good enough job" and leave out those small details. Anyway I agree that we should aim at this: PRs are in the CI/review land, why a commit is like it is, should go into VC.
- t43562 4y ago
  IMO one should maintain a CHANGES.md file with whatever you would be putting in the PR description in it. There's no specific need to squash commits to do this as long as you create it just before you create the PR. Even the odd bugfix or review comment after that is no big deal.
  - lelandfe 4y ago
    I don't see the advantage of doing this over ensuring the commits stay atomic, and I see several disadvantages. Namely, git conflicts will happen constantly; and the file size will graduate from unwieldy to unusable over time. One repo I work on has >45,000 PRs merged. Good luck even opening that file. This approach also poses risk: people will forget to add to it.
    - t43562 4y ago
      If a file is too big you rename it to changes.2022.md. People forget everything but you do reviews to check that presumably. with merge conflicts - if those become a problem for your project you can be more sophisticated. In one case we put all our unreleased change descriptions into a directory and generate CHANGES.md as part of the release. For any PR merge however, you can see a detailed description of what was merged.
- s4i 4y ago
  It would be great if GitHub (not sure about GitLab, Gitea, etc) included the title and the description of the PR (maybe up to a certain character limit) in the merge commit when you merge the PR. This would be at least a minimal level of backup for the PRs but it would also prevent common scenario for me where I need to switch to the browser from `git log` just to remind what the heck this one PR was about again.

by stnmtn 4y ago

lelandfe 4y ago

Pull requests vanish when repos change hands. If you leave unique information in PRs, that information may be lost in the future. This has happened to me at 3 different companies now, where we inherited another company's code base. Keeping commits self-contained is the only way to future proof your explanations.

aulin 4y ago
Also PRs belong to the review and CI system, everything needed to understand the code and its history should be staged in version control
- darkwater 4y ago
  I agree and this sounds super neat but in reality it's pretty complicated to keep that boundary and not leak info from one side to the other. I mean, when you open a PR you will probably describe the rationale for the change and why you had to touch files A,B & C (if it's not obvious). You can and you should replicate that info in a multi-line commit message but sometimes through code review you reshape the code structure or even some of the requirements (most probably because there was some initial misunderstanding). So, since we are humans, it's normal to take shortcuts when we did already "a good enough job" and leave out those small details. Anyway I agree that we should aim at this: PRs are in the CI/review land, why a commit is like it is, should go into VC.
t43562 4y ago
IMO one should maintain a CHANGES.md file with whatever you would be putting in the PR description in it. There's no specific need to squash commits to do this as long as you create it just before you create the PR. Even the odd bugfix or review comment after that is no big deal.
- lelandfe 4y ago
  I don't see the advantage of doing this over ensuring the commits stay atomic, and I see several disadvantages. Namely, git conflicts will happen constantly; and the file size will graduate from unwieldy to unusable over time. One repo I work on has >45,000 PRs merged. Good luck even opening that file. This approach also poses risk: people will forget to add to it.
  - t43562 4y ago
    If a file is too big you rename it to changes.2022.md. People forget everything but you do reviews to check that presumably. with merge conflicts - if those become a problem for your project you can be more sophisticated. In one case we put all our unreleased change descriptions into a directory and generate CHANGES.md as part of the release. For any PR merge however, you can see a detailed description of what was merged.
s4i 4y ago
It would be great if GitHub (not sure about GitLab, Gitea, etc) included the title and the description of the PR (maybe up to a certain character limit) in the merge commit when you merge the PR. This would be at least a minimal level of backup for the PRs but it would also prevent common scenario for me where I need to switch to the browser from `git log` just to remind what the heck this one PR was about again.