20 ms·

I actually disagree. Large teams that still have linear commit histories doesn't mean it is a lie. It means that the code review process is more important that

I actually disagree. Large teams that still have linear commit histories doesn't mean it is a lie. It means that the code review process is more important that the code writing process. For example: I check out a repository, and create a local feature branch. I create a commit containing the tests for the new feature, then one for the first draft of the new feature, then two or three for bugfixes. Each commit is small, and self-contained, but importantly isn't standalone. If someone checked out the repository in the middle of my chain of commits, they wouldn't have a working product. Then I upload my change for code review. There's no point in reviewing each of my ~5 commits individually: they only make sense to the reviewer as a combined unit. And there's no point in landing them individually: they only make sense for the overall project history as a combined unit. In a project with many developers (e.g. 1,000 like the Chromium project), every developer has different local practices. Some keep their work based on HEAD of master via rebase, others via merges. Some do test-driven development, some don't. Making the code review the atomic unit of work, rather than the messy string of local commits, helps the project enforce common etiquette, commit formatting, and readable history.

jldugger 11y ago
> It means that the code review process is more important that the code writing process. If that were true, the optimal solution is PRs with individual commits that all pass testing. I find it much easier to review a series of small changes for logical correctness than mashing them together into a single PR. Github recently added this as a feature, so I'm not in a completely invisible minority there. And then, when the review is over, having discreet commits makes git bisecting down to the commit that broke the system more granular.
- phasmantistes 11y ago
  Absolutely. If someone formulates their PR such that every commit in the chain is small, easily reviewable, and passes all tests, that's fantastic! That makes reviewing code, searching history, and bisecting all easier. Unfortunately, that's not the 90% case that I see. Most of the time a multi-commit PR contains N-1 commits of incremental development and one final one that fixes all the tests and typos and removes debugging print statements. Neither the project nor the author benefit from having those intermediate commits integrated verbatim.
  - jarfil 11y ago
    If people generate N-1 commits with a final one to clean things up, maybe people should learn about git stash and making some WIP branches, then squashing commits themselves or better yet, keeping their own history clean, instead of submitting PRs full of crap. I know, it might be too much to ask of people... oh well.
    - hinkley 11y ago
      The Mikado method is sadly underemphasized. Just because you smash away at code for hours doesn't mean that's how your commit history should look. You can revise history in ways that are beneficial instead of destructive.
    - cortesoft 11y ago
      What is the cost of just doing it at the last merge step? Makes it easier, doesn't it?
      - chris_wot 11y ago
        Yeah, but dreadful if you try to bisect a problem.
        rjayatilleka 11y ago
        Yeah, and that's why you squash all the intermediate commits first. If all your commits on master are useful and meaningful individually, bisect works great.
  - forrestthewoods 11y ago
    > Neither the project nor the author benefit from having those intermediate commits integrated verbatim. Sure they benefit. You can see the thought process that went into the commit. If there's something that seems weird or out of place you can see how it evolved into existence. That can be exceptionally useful.
- stingraycharles 11y ago
  I agree with what you're saying, good code review and good code writing aren't mutually exclusive. However, the truth of the matter is that code review is a far more clearly defined moment in a workflow than writing good code (which is more of a habit than an actual action). As such, it is far more efficient for a team to accept reality and squash after PRs (which is now first possible), instead of relying and keep correcting all your coworkers when they don't properly squash commits before pushing.
- voidlogic 11y ago
  >If that were true, the optimal solution is PRs with individual commits that all pass testing. I find it much easier to review a series of small changes for logical correctness than mashing them together into a single PR. This is too myopic and explains how you can end up with good code, but bad architecture. A good review ensures both. I like my PRs to be about high level goals, and I want them made of lots of commits I can review. The commits themselves can be the result of re-basing (which is fine within feature branches) and maybe not truly chronological.
WorldMaker 11y ago
«Making the code review the atomic unit of work, rather than the messy string of local commits, helps the project enforce common etiquette, commit formatting, and readable history.» I agree that code reviews should be important, and key to understanding the history of a project at a more useful timescale. I just disagree that they should be "atomic" and that a reviewer (even, or especially, a later code archeologist) may not have reason to inspect or dive into smaller units within code reviews. Where I think that we may agree is that I feel that even if they shouldn't necessarily be "atomic", I agree that code reviews should probably be first-class objects when talking about and dealing with source control. In git, you can use --no-ff merges today as a useful approximation of code review boundaries (especially with PRs and GitHub's default --no-ff and including linking PR #s). It might be nice to see code reviews or other aggregates of commits/commit graphs be truly first-class citizens of git in some manner.
- procrastitron 11y ago
  I agree with you, and I prefer the --no-ff merge option for submitting reviews, but I think the argument for it would be a lot stronger if 'git bisect' supported a --first-parent flag the same way that 'git log' does.
lotyrin 11y ago
Yep, the key to understanding git (why there's mutable history, etc.) is to understand that because of its history (designed by Linus) it's not designed to make day-to-day developers's lives easier, but to serve foss project maintainers and release engineers. People who review code for inclusion in a project, want to track meta-progress on issues, want to pin versions for release, etc., mutable history means they can squash fixups or fix your whitespace for you, rebase changesets onto other changesets, have history that reflects the project management strategy, etc.
Justsignedup 11y ago
Also it is very likely that my feature branch isn't really complete during all the little commits I make. The commits are useful to me, but don't often indicate intent too much. The whole thing put together may be easier to diff when a few years in the future someone tries to figure out what happened there.
sulam 11y ago
caveat: I was responsible for code review for 2000+ developers. We only allowed squash commits on master because of what you're describing. That is the level where history "made sense". However, for code review, we wanted to support both styles, because there is an advantage sometimes to seeing the sausage being made. For instance someone will refactor something -- maybe change a method name. Then they apply that refactoring at all the call sites. Very conscientious developers would break this into two commits. We didn't want the first commit on master, but it made sense to review this way, because it was easier on the reviewers: change, effect of change on everything else. I call this "telling a story" with your commits. There's a lot of value in that style if you have the time to do it. The other style of commit-by-commit reviewing, where I see all of the work in progress commits, I don't find valuable at all and I _definitely_ don't want to see on master.
- tedmiston 11y ago
  > The other style of commit-by-commit reviewing, where I see all of the work in progress commits, I don't find valuable at all and I _definitely_ don't want to see on master. This is the style I use for code reviews. There's not a lot of tooling to support it, at least with GitLab, but git-playback [1] is kind of interesting. [1]: http://codingfearlessly.com/2012/05/14/git-playback/ http://codingfearlessly.com/2012/05/14/git-playback/
- phasmantistes 11y ago
  Yep, in chromium-land we do something similar: You can upload multiple different versions of a single code review, and reviewers can diff both against the base and against previous versions of the review. This is helpful for showing "stories", responses to comments, and for "my original commit got reverted, so here I've reuploaded it, and then also uploaded the fix, so you can clearly see what's different this time".
  - chris_wot 11y ago
    Is that using gerrit?
itaysk 11y ago
Regarding your example - why would you commit any of the 5 little commits, If they didn't complete the task, and also are breaking the product? wouldn't it be better to commit only when you completed the task you were working on? (I mean, if the task is made out of sub-tasks, then I would also commit each subtask or milestone that I feel is important, but this is not the case here)
- Piskvorrr 11y ago
  Sometimes, having a snapshot is useful even if it's not a snapshot of a completely functional system. "Hmm, this doesn't seem to be a workable approach here; let's commit, checkout an earlier version and branch off that." It's not necessary to push all these experiments (or even keep them in the end), but it might be useful to have them, at least for the moment.
  - ghostwriter 11y ago
    That's what "git stash" is for.
    - Piskvorrr 11y ago
      Stash is supported as a first-class citizen by exactly zero tools that I've seen - even though a stash is a commit in all but name ;)
- deleted 11y ago
  [deleted]
deleted 11y ago
[deleted]

jldugger 11y ago

> It means that the code review process is more important that the code writing process. If that were true, the optimal solution is PRs with individual commits that all pass testing. I find it much easier to review a series of small changes for logical correctness than mashing them together into a single PR. Github recently added this as a feature, so I'm not in a completely invisible minority there. And then, when the review is over, having discreet commits makes git bisecting down to the commit that broke the system more granular.

phasmantistes 11y ago
Absolutely. If someone formulates their PR such that every commit in the chain is small, easily reviewable, and passes all tests, that's fantastic! That makes reviewing code, searching history, and bisecting all easier. Unfortunately, that's not the 90% case that I see. Most of the time a multi-commit PR contains N-1 commits of incremental development and one final one that fixes all the tests and typos and removes debugging print statements. Neither the project nor the author benefit from having those intermediate commits integrated verbatim.
- jarfil 11y ago
  If people generate N-1 commits with a final one to clean things up, maybe people should learn about git stash and making some WIP branches, then squashing commits themselves or better yet, keeping their own history clean, instead of submitting PRs full of crap. I know, it might be too much to ask of people... oh well.
  - hinkley 11y ago
    The Mikado method is sadly underemphasized. Just because you smash away at code for hours doesn't mean that's how your commit history should look. You can revise history in ways that are beneficial instead of destructive.
  - cortesoft 11y ago
    What is the cost of just doing it at the last merge step? Makes it easier, doesn't it?
    - chris_wot 11y ago
      Yeah, but dreadful if you try to bisect a problem.
      - rjayatilleka 11y ago
        Yeah, and that's why you squash all the intermediate commits first. If all your commits on master are useful and meaningful individually, bisect works great.
- forrestthewoods 11y ago
  > Neither the project nor the author benefit from having those intermediate commits integrated verbatim. Sure they benefit. You can see the thought process that went into the commit. If there's something that seems weird or out of place you can see how it evolved into existence. That can be exceptionally useful.
stingraycharles 11y ago
I agree with what you're saying, good code review and good code writing aren't mutually exclusive. However, the truth of the matter is that code review is a far more clearly defined moment in a workflow than writing good code (which is more of a habit than an actual action). As such, it is far more efficient for a team to accept reality and squash after PRs (which is now first possible), instead of relying and keep correcting all your coworkers when they don't properly squash commits before pushing.
voidlogic 11y ago
>If that were true, the optimal solution is PRs with individual commits that all pass testing. I find it much easier to review a series of small changes for logical correctness than mashing them together into a single PR. This is too myopic and explains how you can end up with good code, but bad architecture. A good review ensures both. I like my PRs to be about high level goals, and I want them made of lots of commits I can review. The commits themselves can be the result of re-basing (which is fine within feature branches) and maybe not truly chronological.

WorldMaker 11y ago

«Making the code review the atomic unit of work, rather than the messy string of local commits, helps the project enforce common etiquette, commit formatting, and readable history.» I agree that code reviews should be important, and key to understanding the history of a project at a more useful timescale. I just disagree that they should be "atomic" and that a reviewer (even, or especially, a later code archeologist) may not have reason to inspect or dive into smaller units within code reviews. Where I think that we may agree is that I feel that even if they shouldn't necessarily be "atomic", I agree that code reviews should probably be first-class objects when talking about and dealing with source control. In git, you can use --no-ff merges today as a useful approximation of code review boundaries (especially with PRs and GitHub's default --no-ff and including linking PR #s). It might be nice to see code reviews or other aggregates of commits/commit graphs be truly first-class citizens of git in some manner.

procrastitron 11y ago
I agree with you, and I prefer the --no-ff merge option for submitting reviews, but I think the argument for it would be a lot stronger if 'git bisect' supported a --first-parent flag the same way that 'git log' does.

lotyrin 11y ago

Yep, the key to understanding git (why there's mutable history, etc.) is to understand that because of its history (designed by Linus) it's not designed to make day-to-day developers's lives easier, but to serve foss project maintainers and release engineers. People who review code for inclusion in a project, want to track meta-progress on issues, want to pin versions for release, etc., mutable history means they can squash fixups or fix your whitespace for you, rebase changesets onto other changesets, have history that reflects the project management strategy, etc.

Justsignedup 11y ago

Also it is very likely that my feature branch isn't really complete during all the little commits I make. The commits are useful to me, but don't often indicate intent too much. The whole thing put together may be easier to diff when a few years in the future someone tries to figure out what happened there.

sulam 11y ago

caveat: I was responsible for code review for 2000+ developers. We only allowed squash commits on master because of what you're describing. That is the level where history "made sense". However, for code review, we wanted to support both styles, because there is an advantage sometimes to seeing the sausage being made. For instance someone will refactor something -- maybe change a method name. Then they apply that refactoring at all the call sites. Very conscientious developers would break this into two commits. We didn't want the first commit on master, but it made sense to review this way, because it was easier on the reviewers: change, effect of change on everything else. I call this "telling a story" with your commits. There's a lot of value in that style if you have the time to do it. The other style of commit-by-commit reviewing, where I see all of the work in progress commits, I don't find valuable at all and I _definitely_ don't want to see on master.

tedmiston 11y ago
> The other style of commit-by-commit reviewing, where I see all of the work in progress commits, I don't find valuable at all and I _definitely_ don't want to see on master. This is the style I use for code reviews. There's not a lot of tooling to support it, at least with GitLab, but git-playback [1] is kind of interesting. [1]: http://codingfearlessly.com/2012/05/14/git-playback/ http://codingfearlessly.com/2012/05/14/git-playback/
phasmantistes 11y ago
Yep, in chromium-land we do something similar: You can upload multiple different versions of a single code review, and reviewers can diff both against the base and against previous versions of the review. This is helpful for showing "stories", responses to comments, and for "my original commit got reverted, so here I've reuploaded it, and then also uploaded the fix, so you can clearly see what's different this time".
- chris_wot 11y ago
  Is that using gerrit?

itaysk 11y ago

Regarding your example - why would you commit any of the 5 little commits, If they didn't complete the task, and also are breaking the product? wouldn't it be better to commit only when you completed the task you were working on? (I mean, if the task is made out of sub-tasks, then I would also commit each subtask or milestone that I feel is important, but this is not the case here)

Piskvorrr 11y ago
Sometimes, having a snapshot is useful even if it's not a snapshot of a completely functional system. "Hmm, this doesn't seem to be a workable approach here; let's commit, checkout an earlier version and branch off that." It's not necessary to push all these experiments (or even keep them in the end), but it might be useful to have them, at least for the moment.
- ghostwriter 11y ago
  That's what "git stash" is for.
  - Piskvorrr 11y ago
    Stash is supported as a first-class citizen by exactly zero tools that I've seen - even though a stash is a commit in all but name ;)
deleted 11y ago
[deleted]

deleted 11y ago

[deleted]