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.

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.

OK. So would you support an IDE that generated one individual commit per keypress? When I type "hello", that's five individual commits each changing a single letter.

That is the true history of what happened, and it's typically recorded in your IDE's undo log in memory, such that you can step back and forward through it. Are you OK with pushing this commit history to your project? That's the true history of what happened, after all, and it could be forensically useful.

When we commit code to share it with other people, we recognize that only a certain level of detail is relevant to them. The physical sequence of letters that I pressed isn't typically relevant to someone else. Rather, the logical set of changes is what affects them.

If you agree that a single commit per letter of keypress is undesirable then you agree with me in principle. There is a finite level of detail that makes sense to share, practically, with current source control systems, and what we're arguing about is how much to share.

I feel like this is important to point out because people frequently make this argument about the "true history" of source, and all that, while neglecting the fact that the commits they choose to ship are already an arbitrary simplification of the "true history".

That would actually be awesome, if the tools supported it. Imagine how easy it would be to find bugs with a bisect if it can drill down to the actual keystroke that introduced a bug.

Realistically, you'd want to back out a few notches though. Say, every time the dev hit Shift+Ctrl+B (or tabbed over to the browser or whatever signifies "build the project" in the environment in question), so that you get an indication that the current state of things was meant to at least compile and run.

But yeah, that's the value of source control. Being able to dig back in history to the exact spot where a bug was born. I can't understand all why so many people here would want to scrub that away.

Apparently introducing interesting problems when people create a log file one byte at a time, up to several hundred megs. (just 100000000 files for one little project, oops)

Not OP but - conceptually? Yes. I've written file formats which preserve undo/redo history, for example. The caveats:

1) I don't want to inadvertently leak my password. This is an issue with things such as local bash command history buffers as well, and unreviewed autocommits in general. You could say it's too useful - to the wrong people!

2) My tooling needs to be built around a different level of granularity as the default. I'm not OK with single letter commits cluttering up my git log, for example. Having them around to drill down into if I need them? Sure.

Per-letter detail is so granular that even undo/redo systems will often squash history states together. Observing your exact typing Cadence / the extra evidence of initial authorship is niche enough that I'm quite willing to sacrifice that level of detail for the sake of performance, maintainability, or basically any and every other excuse you can think of.

> When we commit code to share it with other people, we recognize that only a certain level of detail is relevant to them. The physical sequence of letters that I pressed isn't typically relevant to someone else. Rather, the logical set of changes is what affects them.

I have never seen a codebase with perfect commits. The ones that always give me trouble wrapping my head around are the squashed "logical set of changes", where the set size is way too damn big. Reverse engineering a saner overview from a series of tiny commits is way easier.

And the physical way something was done does matter at times. I'm going to pay way more attention to "whitespace cleanup" commit done by a human than "whitespace cleanup" commit done by vetted tools, for example. In whitespace significant languages, the former may trigger a full code review. Similar concerns with a lot of refactorings, actually.

> If you agree that a single commit per letter of keypress is undesirable then you agree with me in principle.

Per the above, I can only agree with you in practice :)

> There is a finite level of detail that makes sense to share, practically, with current source control systems, and what we're arguing about is how much to share.

Agreed. But I haven't found a single, solitary codebase, where I'd ever argue "less". I can't even recall a single, solitary commit where I'd have ever argued "less". Commit directly to mainline to fix a single character typo? I'll be annoyed if your changelist description was too terse! I want to see:

  Fix the build:  Fix typo 'baz' -> 'bar'
  Fix documentation typo: 'baz' -> 'bar'

A coworker of mine shared he'd collected stats on who made the most commits/day to test a hypothesis. Due to some outliers, he'd decided against it, but thought I'd find the stats amusing. Out of ~50 people (~15 programmers), I (the most recently hired programmer) topped the chart. Second place? The build server account (thanks to nightly build scripts.) I was indeed amused.

https://cgit.freedesktop.org/libreoffice/core/log/

(Don't look at the OpenOffice.org years, they literally took a whole bunch if development work from SVN branches and then merged them in as a single commit and put in single line descriptions with internal tracking numbers and odd project management codes... utter disaster! And of course the branches are now all lost...)

I like the scope of a lot of those commits, although a few are still chunkier looking than I'd like - take that with a grain of salt, though, as I don't have a good enough feel for the codebase to reasonably estimate how much more they could be chunked up. Pretty much everything has a review link, which is nice. I'd expect more back and forth in the comments, but perhaps that's handled out-of-band.

I have lots of nitpicks with the actual changelist descriptions where I'd want things to improve. "Clean up" could mean just about anything - I must go to diff (and expand the context) to understand e.g. https://cgit.freedesktop.org/libreoffice/core/commit/?id=945... properly. I'd be inclined to instead write:

officeipcthread: Cleanup RequestHandler::Enable: Early bail, remove aDummy (just use aUserInstallPath directly), move declarations.

Now I know scope, and the types of changes (refactoring worth reviewing if looking for breakage, not just ignorable whitespace / comment changes.)

No gripes with the overall style on this one, although interacting with a security component, I'd want multiple reviewers: https://cgit.freedesktop.org/libreoffice/core/commit/?id=2a9... .

I'd lean towards linking a screenshot of at least the new version of UI when the file being modified "isn't human readable" (read: is modified with something other than a text editor, even if I can totally read it) which would apply to e.g. https://cgit.freedesktop.org/libreoffice/core/commit/?id=72c... .

EDIT: Formatting, + rationale RE: changelist description.

Thanks for the info, that's really quite insightful. Appreciate you taking the time to give it!

I must be blind - somehow I only saw one the first time around, but they're clearly both there.

> Thanks for the info, that's really quite insightful. Appreciate you taking the time to give it!

I'll take your word for it ;). And no problem.

And that would be nice to have, a vcs that allows you to fold and unfold commits to different levels of granularity.

Wanna have only a straight line history with all feature branches squashed? There you have it. Just need some of them like that? Sure enough. What if you want every time the file was saved? Not a problem, just configure it to commit automatically.

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.

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.)

FWIW, the OpenOffice.org guys had this insane workflow where they squashed all commits into one commit and then out a summary of all the changes into the commit message. Then they scrubbed the private branches.

That methodology is an extreme example of where squashing does NOT make sense.

This is otherwise known as a merge. A single atomic change to master, referencing only the final files, with the first parent referencing the previous master that was also "not" broken.

It has only one change: A second parent, referencing the more accurate history.

If we start without M, and start with master=R2, temporary-feature-branch=R5, I expect the following outcomes:

"squash" will leave us with master=M, and the graph R1 -> R2 -> M. Once temporary-feature-branch is deleted, R3-R5 are forever lost.

"merge" will leave us with master=M, and the graph R1 -> R2 -> M, + R2 -> R3 -> R4 -> R5 -> M

"rebase" will leave us with master=R5, and the graph R1 -> R2 -> R3 -> R4 -> R5. Once temporary-feature-branch is deleted,

The first case loses the history of the code, and thus can't be what we're talking about.

If you call the second case "squash" I find that very confusing. It's also not what github is referring to when talking about "squash your commits" in the parent article, they're talking about the first case. Now, git confuses the issue a bit too - perhaps I should be labeling the second case a "non-fast-forward merge", but I'd still find that clearer than "squash without losing the history."

I had to find when a bug was introduced, and guess what? it was inside a single massive commit touching hundreds of files. I prefer atomic commits with good messages, thanks.

[0] https://git-scm.com/docs/git-bisect

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".

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/

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.

I am really curious how you handled these sort of situations at that scale!

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.

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.

I know, it might be too much to ask of people... oh well.

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.

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.

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.

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.

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.