3 ms·

We take a "PR all the things" approach using MS VSTS/DevOps. Every PR needs a work-item attached and a description, and those get cooked into the commit. We a

by Pxtl 7y ago

We take a "PR all the things" approach using MS VSTS/DevOps. Every PR needs a work-item attached and a description, and those get cooked into the commit. We always squash, but squashing is dubious - if the any of the commits of the branch are still kicking around in other branches, it looks like they're ahead when they're not. Imho, this is a huge flaw in GIT - I don't want to squash, but I do want a clean history and I don't want to require that juniors and student developers waste their time curating their history. The fact that there's no way to mark commits as "this is unimportant crap leading up to the merge" in Git is annoying. VSTS logs the PR number and the relevant work-items in the message so for any given commit it's easy to get back to the PR where you can see the detailed discussion of the change, requirements, etc. But either way: reviewers enforce useful PR message, and we discard the intermediate commits. This works very well for clean history.

SkyPuncher 7y ago
Isn't that what rebasing is for? We rebase all of our feature branches for PR's.
- Pxtl 7y ago
  Yeah, I just mean the PR process auto-squashes/rebases instead of merging, so that junior developers don't have to think about it. The problem is that if somebody leaves the branch up (delete-on-merge is obviously preferred), or derivative experimental branches, you can't tell if it's been merged or not because the commit hashes are different after the rebase. The branch list has become a scary basement.
  - bendavis381 7y ago
    Check out how phabricator handles this. It's essentially what you're asking for: https://secure.phabricator.com/phame/post/view/766/write_review_merge_publish_phabricator_review_workflow/ https://secure.phabricator.com/phame/post/view/766/write_rev... TL;DR: PRs are not based off a diff between master and a remote branch. Instead, phabricator sends up patches to code review and when approved, the Phabricator tooling handles the rebase locally. No remote branches and clean history.
    - Pxtl 7y ago
      That's an interesting fix although I'm unsure about the sense in discouraging pushing changes. It seems like the chief benefit they tout - clean history - is already handled by the auto-squash-and-delete approach. The real problem is the difficulty in cleanup of secondary or undeleted branches. It isnt' really what I was asking for, though. What I want is a "soft-squash" where the intermediate commits aren't deleted, rather they're just hidden from history. The ability to draw a dotted line around a region of the DAG and say "treat this blob like it was one commit for all intents and purposes unless I say otherwise.
pavon 7y ago
> VSTS logs the PR number and the relevant work-items in the message so for any given commit it's easy to get back to the PR where you can see the detailed discussion of the change, requirements, etc. I've worked on a couple project that took the approach of documenting the details of what was done in the bug tracker (or PR/MR), etc, and then just putting the bug# and a one-line description in the commit history. In every one of these projects, we ended up later moving to a different bug tracking system. In some cases the bugs were transferred, but the numbers were not, in others nothing was transferred. Suddenly all those bug numbers in the commit history were completely meaningless and useless. I'm now a strong advocate that the detailed documentation should be in the commits messages themselves. One of my biggest complaints with gitlab is that you spend all this time writing a good MR description, and then it has no mechanism for including that description in the actual merge commit.
- Pxtl 7y ago
  Yeah, for me the approach is that the PR describe the what that was done at high-level, and the requirement describes the why. That said, MS is good at long-term support if nothing else. Our TFS/VSTS/DevOps instance is older than dirt so I don't have to worry about losing history.
stefan_ 7y ago
> I don't want to require that juniors and student developers waste their time curating their history Uhm, one of just the most important skills for junior developers to learn is to tackle a larger change through a succession of individually logical, atomic changes.

by Pxtl 7y ago

SkyPuncher 7y ago

Isn't that what rebasing is for? We rebase all of our feature branches for PR's.

Pxtl 7y ago
Yeah, I just mean the PR process auto-squashes/rebases instead of merging, so that junior developers don't have to think about it. The problem is that if somebody leaves the branch up (delete-on-merge is obviously preferred), or derivative experimental branches, you can't tell if it's been merged or not because the commit hashes are different after the rebase. The branch list has become a scary basement.
- bendavis381 7y ago
  Check out how phabricator handles this. It's essentially what you're asking for: https://secure.phabricator.com/phame/post/view/766/write_review_merge_publish_phabricator_review_workflow/ https://secure.phabricator.com/phame/post/view/766/write_rev... TL;DR: PRs are not based off a diff between master and a remote branch. Instead, phabricator sends up patches to code review and when approved, the Phabricator tooling handles the rebase locally. No remote branches and clean history.
  - Pxtl 7y ago
    That's an interesting fix although I'm unsure about the sense in discouraging pushing changes. It seems like the chief benefit they tout - clean history - is already handled by the auto-squash-and-delete approach. The real problem is the difficulty in cleanup of secondary or undeleted branches. It isnt' really what I was asking for, though. What I want is a "soft-squash" where the intermediate commits aren't deleted, rather they're just hidden from history. The ability to draw a dotted line around a region of the DAG and say "treat this blob like it was one commit for all intents and purposes unless I say otherwise.

pavon 7y ago

> VSTS logs the PR number and the relevant work-items in the message so for any given commit it's easy to get back to the PR where you can see the detailed discussion of the change, requirements, etc. I've worked on a couple project that took the approach of documenting the details of what was done in the bug tracker (or PR/MR), etc, and then just putting the bug# and a one-line description in the commit history. In every one of these projects, we ended up later moving to a different bug tracking system. In some cases the bugs were transferred, but the numbers were not, in others nothing was transferred. Suddenly all those bug numbers in the commit history were completely meaningless and useless. I'm now a strong advocate that the detailed documentation should be in the commits messages themselves. One of my biggest complaints with gitlab is that you spend all this time writing a good MR description, and then it has no mechanism for including that description in the actual merge commit.

Pxtl 7y ago
Yeah, for me the approach is that the PR describe the what that was done at high-level, and the requirement describes the why. That said, MS is good at long-term support if nothing else. Our TFS/VSTS/DevOps instance is older than dirt so I don't have to worry about losing history.

stefan_ 7y ago

> I don't want to require that juniors and student developers waste their time curating their history Uhm, one of just the most important skills for junior developers to learn is to tackle a larger change through a succession of individually logical, atomic changes.