5 ms·

Git commit crafting (and rebase to achieve it) is overrated. If you care about crafting beautiful series of commits so that the future readers understands what'

by danwee 4y ago

Git commit crafting (and rebase to achieve it) is overrated. If you care about crafting beautiful series of commits so that the future readers understands what's going on: don't. Context is more useful to find out why something changed. Example: - you build feature F that is touching N files and M lines of code - you craft your git commits so that each of them is atomic and "understandable" on its own - now if I want to see the context of change for line X, I won't get the changes in all the N files... I'll get your crafted commit that updates only line X (worste case) or its immediate surroundings (best case). But how do I get the full context (i.e., this line is about feature F)? I have to git blame lines around line X and see if I can actually make a picture of the bigger feature. Examples of feature F: a whole new http endpoint touching controllers, services, data access models, middleware. What do you get when people craft their commits? 1 commit for the controller, 1 commit for the service, 1 commit for the data access model, etc. Zero context. Time wasted for the reader

aulin 4y ago
If you keep merge commits you can get the full diff at once and see all the context you need, if you don't you can still write meaningful commit messages that identify the feature you're working on so that in the future you can still do a diff between the first and the last commit and see it all at once.
- willio58 4y ago
  Yeah if you just use commits as they are intended then the complexity of git drops off massively. I’ve used got for a decade and want to know how many times I’ve rebased? Zero
  - aulin 4y ago
    I tend to stage everything I do in my local branches, experiments, hacks, partial changes I don't have time to finish right now, unfinished refactorings. No way I share that without rebasing it and cleaning it up first.
  - mook 4y ago
    Isn't commits as they are intended a patch queue in email with a cover letter (0/n commit)? At least, that seemed to be how the Linux kernel works.
  - __blockcipher__ 4y ago
    I am deeply suspicious of people who claim to have never rebased. I've never met such a person who didn't have absolutely disgusting commits: either (1) no commit body to explain the context of why a chance was made, for patches that clearly needed one, (2) random merge commits in the middle of the actual commits because they don't know any workflow besides merging master into their ongoing dev branch, or (3) massive pull requests (if using a pull request workflow) that touch 30 files and implement way more than just the feature the branch was supposed to be for. It's also a big sign to me that the person probably rarely if ever actually spelunks through the git log to understand why a certain change was made. Because if one does, they very quickly discover why a workflow involving sane rebasing onto origin/master produces such a better history. There's times when merge commits are appropriate, but they are few and far between compared to the endless abuses of merge commits by people who don't understand git in the slightest (my definition of "don't understand git" is someone who can't visualize the DAG)
  - lelandfe 4y ago
    10 years of typos in commit messages
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.
cnity 4y ago
> commit crafting is overrated. For you, in your use case. When I look at my neat history and use git bisect, I get plenty of value out of it. People keep telling me to stop rebasing. I keep ignoring them; nothing new here.
- cube2222 4y ago
  This is basically solved by squashing each PR on merge and having a good PR title + description.
  - theonething 4y ago
    This is my habit and I advocate for it on new teams. It's so simple and effective. Why make it complicated?
  - ajorgensen 4y ago
    But why not do both? I find that nice commit messages make it easier on the review to see whats going on rather than a bunch of wip commits. Then squash all of the commits into a single commit with a nice title and message. The benefits of the squash at the end are, assuming you require tests pass before merge, you have a history of commits all with passing builds which makes bisect possible.
    - mixmastamyk 4y ago
      Wouldn't the cleanup work be lost in the squash?
  - hardolaf 4y ago
    This is exactly what we do at my job. The only time it ever causes issues with some things is when we're ramping up a new project and the PR and ticket titles don't necessarily match the work we've done. But that's more of a problem where we could make everything bite-sized, or we can deliver complete packages in terms of a feature plus all of the miscellaneous changes that we needed to make along the way to make it work. Now, is this the best way to do things? No, you should ideally make those changes separately and push each one separately. But that slows down progress.
jrochkind1 4y ago
But I've most often seen rebase used, as in OP, to squash multiple commits into ONE commit that covers the feature that was added. I feel like the outcome of rebase used there is to give you what you are asking for, and without rebase you wouldn't be getting it? (I have very mixed feelings toward rebase, myself)
- prepend 4y ago
  Why does this matter? I can just diff from the first commit before to some point if I really want to see it all wrapped up. It seems like lots of work that almost always does not matter. I don’t think git is a good project management system. If I want easy to read stuff I can look at the release notes or tag notes or whatnot.
montebicyclelo 4y ago
For a nice record of history - rather than squashing all of the smaller commits into a single commit for a given feature, and then rebasing (which makes it less easy to have small commits with specific explanations that add together to create a feature), something like semantic-release [1] could be used, which autogenerates release notes based on commit messages. [1] https://github.com/semantic-release/semantic-release https://github.com/semantic-release/semantic-release
theptip 4y ago
I think you are arguing against an extreme version of the practice. Normal advice is to shoot for 100-200 lines in a commit, not split that into ten commits that each change 15 lines. If someone is splitting 100-line commits into 10-line commits, I would advise not doing that. However the direction that people normally err (IME) is submitting 500-2000-line commits which conflate multiple atomic changes. I would also advise not doing that. It’s easier to review a sweet-spot commit, and it’s easier to understand when you later have to debug/bisect a change set. Another related concept is to try to split “functional no-op” refactors from behavioral changes. This is usually the first and easiest way of getting your commit size down, as refactors often bloat the diff. In your example case I’d hope you have UTs exercising each chunk that is added. There are sensible APIs to shape at granularities smaller than the whole feature (model, service, etc) (or should be at least). If you really can’t add a new endpoint without a 1k- line PR I think you might need a new abstraction layer. But often you can add a new endpoint in meaningful chunks that are feature-flagged off, if you craft it thoughtfully.
- deleted 4y ago
  [deleted]
- thfuran 4y ago
  I don't think lines of code is much better an estimate of commit complexity than it is of productivity. I could make a 1,000 line change by changing the signature of something that gets called a lot, but that's essentially atomic. I could make a 200 line change by sightly updating 100 totally unrelated functions and that's probably totally nuts.
  - theptip 4y ago
    Sure, it's just a rule of thumb, not a hard requirement. In my experience most functional/behavioral commits benefit from being in the 100-250 line range, and it's more common for engineers to make things too big rather than too small. YMMV. As you say, non-functional refactors can be bigger while still being readable/comprehensible (but I think it's important to keep them scoped as such, and commit crafting is important for this).

by danwee 4y ago

aulin 4y ago

If you keep merge commits you can get the full diff at once and see all the context you need, if you don't you can still write meaningful commit messages that identify the feature you're working on so that in the future you can still do a diff between the first and the last commit and see it all at once.

willio58 4y ago
Yeah if you just use commits as they are intended then the complexity of git drops off massively. I’ve used got for a decade and want to know how many times I’ve rebased? Zero
- aulin 4y ago
  I tend to stage everything I do in my local branches, experiments, hacks, partial changes I don't have time to finish right now, unfinished refactorings. No way I share that without rebasing it and cleaning it up first.
- mook 4y ago
  Isn't commits as they are intended a patch queue in email with a cover letter (0/n commit)? At least, that seemed to be how the Linux kernel works.
- __blockcipher__ 4y ago
  I am deeply suspicious of people who claim to have never rebased. I've never met such a person who didn't have absolutely disgusting commits: either (1) no commit body to explain the context of why a chance was made, for patches that clearly needed one, (2) random merge commits in the middle of the actual commits because they don't know any workflow besides merging master into their ongoing dev branch, or (3) massive pull requests (if using a pull request workflow) that touch 30 files and implement way more than just the feature the branch was supposed to be for. It's also a big sign to me that the person probably rarely if ever actually spelunks through the git log to understand why a certain change was made. Because if one does, they very quickly discover why a workflow involving sane rebasing onto origin/master produces such a better history. There's times when merge commits are appropriate, but they are few and far between compared to the endless abuses of merge commits by people who don't understand git in the slightest (my definition of "don't understand git" is someone who can't visualize the DAG)
- lelandfe 4y ago
  10 years of typos in commit messages

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.

cnity 4y ago

> commit crafting is overrated. For you, in your use case. When I look at my neat history and use git bisect, I get plenty of value out of it. People keep telling me to stop rebasing. I keep ignoring them; nothing new here.

cube2222 4y ago
This is basically solved by squashing each PR on merge and having a good PR title + description.
- theonething 4y ago
  This is my habit and I advocate for it on new teams. It's so simple and effective. Why make it complicated?
- ajorgensen 4y ago
  But why not do both? I find that nice commit messages make it easier on the review to see whats going on rather than a bunch of wip commits. Then squash all of the commits into a single commit with a nice title and message. The benefits of the squash at the end are, assuming you require tests pass before merge, you have a history of commits all with passing builds which makes bisect possible.
  - mixmastamyk 4y ago
    Wouldn't the cleanup work be lost in the squash?
- hardolaf 4y ago
  This is exactly what we do at my job. The only time it ever causes issues with some things is when we're ramping up a new project and the PR and ticket titles don't necessarily match the work we've done. But that's more of a problem where we could make everything bite-sized, or we can deliver complete packages in terms of a feature plus all of the miscellaneous changes that we needed to make along the way to make it work. Now, is this the best way to do things? No, you should ideally make those changes separately and push each one separately. But that slows down progress.

jrochkind1 4y ago

But I've most often seen rebase used, as in OP, to squash multiple commits into ONE commit that covers the feature that was added. I feel like the outcome of rebase used there is to give you what you are asking for, and without rebase you wouldn't be getting it? (I have very mixed feelings toward rebase, myself)

prepend 4y ago
Why does this matter? I can just diff from the first commit before to some point if I really want to see it all wrapped up. It seems like lots of work that almost always does not matter. I don’t think git is a good project management system. If I want easy to read stuff I can look at the release notes or tag notes or whatnot.

montebicyclelo 4y ago

For a nice record of history - rather than squashing all of the smaller commits into a single commit for a given feature, and then rebasing (which makes it less easy to have small commits with specific explanations that add together to create a feature), something like semantic-release [1] could be used, which autogenerates release notes based on commit messages. [1] https://github.com/semantic-release/semantic-release https://github.com/semantic-release/semantic-release

theptip 4y ago

I think you are arguing against an extreme version of the practice. Normal advice is to shoot for 100-200 lines in a commit, not split that into ten commits that each change 15 lines. If someone is splitting 100-line commits into 10-line commits, I would advise not doing that. However the direction that people normally err (IME) is submitting 500-2000-line commits which conflate multiple atomic changes. I would also advise not doing that. It’s easier to review a sweet-spot commit, and it’s easier to understand when you later have to debug/bisect a change set. Another related concept is to try to split “functional no-op” refactors from behavioral changes. This is usually the first and easiest way of getting your commit size down, as refactors often bloat the diff. In your example case I’d hope you have UTs exercising each chunk that is added. There are sensible APIs to shape at granularities smaller than the whole feature (model, service, etc) (or should be at least). If you really can’t add a new endpoint without a 1k- line PR I think you might need a new abstraction layer. But often you can add a new endpoint in meaningful chunks that are feature-flagged off, if you craft it thoughtfully.

deleted 4y ago
[deleted]
thfuran 4y ago
I don't think lines of code is much better an estimate of commit complexity than it is of productivity. I could make a 1,000 line change by changing the signature of something that gets called a lot, but that's essentially atomic. I could make a 200 line change by sightly updating 100 totally unrelated functions and that's probably totally nuts.
- theptip 4y ago
  Sure, it's just a rule of thumb, not a hard requirement. In my experience most functional/behavioral commits benefit from being in the 100-250 line range, and it's more common for engineers to make things too big rather than too small. YMMV. As you say, non-functional refactors can be bigger while still being readable/comprehensible (but I think it's important to keep them scoped as such, and commit crafting is important for this).