6 ms·

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 w

by 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?
  - sulam 11y ago
    Rietveld looks like. Which used to be the code review tool at Google, but has since been replaced. Not by Gerrit. ;)
andersonvom 11y ago
While this is "telling a story", the first commit will break your code for no good reason (i.e. if you rename a method, but not its usages, hell breaks loose). This make you lose one very useful features of git: the ability to binary search for the place where a bug was introduced - git bisect.
- sulam 11y ago
  Which is why that commit doesn't get on master.
- hinkley 11y ago
  But so does squashing, which is one of a handful of reasons I hate most uses of squashing. Only squash when it removes bug that only ever existed on your machine. Everything else should be recorded in the history. Forensics are important to the long term health of your project and you impoverish yourselves by scrubbing the crime scene.
  - jasonlotito 11y ago
    > Only squash when it removes bug that only ever existed on your machine. Everything else should be recorded in the history. You can squash into master without losing the history of the code review with git. This gives you the best of both worlds, a more accurate history than the one you propose, and a master that isn't broken.
    - Pyxl101 11y ago
      I've often wondered if it would be possible to design a source control system that could track this lineage. Kind of like, "inside" of a commit would be a hidden history that you could unpack if you wanted to. "This single commit is actually the squash of these other commits made by this guy". "This commit is actually the rebase onto mainline of this commit a guy made when his machine was behind". The commit would look like normal to the current source control CLI, by default, but would reveal this hidden level of detail if you inquired "How was this commit made?" - then you'd get the reflog. I've wondered if support for a feature like this could finally resolve the "debate" between merging and rebase. The people who like having the full history (whatever that means) would have it, and the people who care about logical changes would see it by default. The cost would be that the repo history would indeed have the full level of detail, but you could imagine supporting a cleanup operation that erases this "origin" information for commits that are sufficiently old, or else simply doesn't pull the info by default and fetches it only when necessary. (You can kind of simulate this using merge commits, by only looking at one parent, but it doesn't really work out well in practice.)
vcarl 11y ago
I try to shoot for meaningful individual commits, squashing to get the intent of each change (more or less) self contained. I've found it really helpful if I ever need to `git bisect` to find what introduced a bug, I get a small(ish) commit that only affects a single module. Very helpful when debugging code I wrote last year.
suls 11y ago
How did you - on that 2k+ developers project - deal with the fact that sometimes someone is working on a change in a feature branch (a) that the an other developer also needs for her feature branch (b)? Would you move the commits that need to be shared away from the features onto a seperate branch (c) and then .. ? I am really curious how you handled these sort of situations at that scale!
radarsat1 11y ago
In that sense it would be cool if git supported something more like "branch collapsing," which could be used instead of squashing a branch and rebasing that on master. It should be possible to "view" a branch as if it were a single commit on master, with its own comments etc (from the merge commit perhaps) and then expand it. Perhaps this is just a UI problem..
- WorldMaker 11y ago
  I do think this is mostly a UI problem. As someone else suggested in this thread sites like GitHub which show only a linear list of commits should probably default to something more like `git log --first-parent`. There also sounds like a need for `git bisect` to be smarter about DAG traversal, in that it seems to treat the branch as if it were a flattened linear list rather than taking advantage of the merge structure of a branch. This also sounds like something that should be relatively easy to make smarter.

by sulam 11y ago

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?
- sulam 11y ago
  Rietveld looks like. Which used to be the code review tool at Google, but has since been replaced. Not by Gerrit. ;)

andersonvom 11y ago

While this is "telling a story", the first commit will break your code for no good reason (i.e. if you rename a method, but not its usages, hell breaks loose). This make you lose one very useful features of git: the ability to binary search for the place where a bug was introduced - git bisect.

sulam 11y ago
Which is why that commit doesn't get on master.
hinkley 11y ago
But so does squashing, which is one of a handful of reasons I hate most uses of squashing. Only squash when it removes bug that only ever existed on your machine. Everything else should be recorded in the history. Forensics are important to the long term health of your project and you impoverish yourselves by scrubbing the crime scene.
- jasonlotito 11y ago
  > Only squash when it removes bug that only ever existed on your machine. Everything else should be recorded in the history. You can squash into master without losing the history of the code review with git. This gives you the best of both worlds, a more accurate history than the one you propose, and a master that isn't broken.
  - Pyxl101 11y ago
    I've often wondered if it would be possible to design a source control system that could track this lineage. Kind of like, "inside" of a commit would be a hidden history that you could unpack if you wanted to. "This single commit is actually the squash of these other commits made by this guy". "This commit is actually the rebase onto mainline of this commit a guy made when his machine was behind". The commit would look like normal to the current source control CLI, by default, but would reveal this hidden level of detail if you inquired "How was this commit made?" - then you'd get the reflog. I've wondered if support for a feature like this could finally resolve the "debate" between merging and rebase. The people who like having the full history (whatever that means) would have it, and the people who care about logical changes would see it by default. The cost would be that the repo history would indeed have the full level of detail, but you could imagine supporting a cleanup operation that erases this "origin" information for commits that are sufficiently old, or else simply doesn't pull the info by default and fetches it only when necessary. (You can kind of simulate this using merge commits, by only looking at one parent, but it doesn't really work out well in practice.)

vcarl 11y ago

I try to shoot for meaningful individual commits, squashing to get the intent of each change (more or less) self contained. I've found it really helpful if I ever need to `git bisect` to find what introduced a bug, I get a small(ish) commit that only affects a single module. Very helpful when debugging code I wrote last year.

suls 11y ago

How did you - on that 2k+ developers project - deal with the fact that sometimes someone is working on a change in a feature branch (a) that the an other developer also needs for her feature branch (b)? Would you move the commits that need to be shared away from the features onto a seperate branch (c) and then .. ? I am really curious how you handled these sort of situations at that scale!

radarsat1 11y ago

In that sense it would be cool if git supported something more like "branch collapsing," which could be used instead of squashing a branch and rebasing that on master. It should be possible to "view" a branch as if it were a single commit on master, with its own comments etc (from the merge commit perhaps) and then expand it. Perhaps this is just a UI problem..

WorldMaker 11y ago
I do think this is mostly a UI problem. As someone else suggested in this thread sites like GitHub which show only a linear list of commits should probably default to something more like `git log --first-parent`. There also sounds like a need for `git bisect` to be smarter about DAG traversal, in that it seems to treat the branch as if it were a flattened linear list rather than taking advantage of the merge structure of a branch. This also sounds like something that should be relatively easy to make smarter.