Remix.run Logo
theshrike79 8 hours ago

Can you explain to me (an avid squash-merger) what extra information do you gain by having commits that say "argh, let's see if this works", "crap, the CI is failing again, small fix to see if it works", "pushing before leaving for vacation" in the main git history?

With a squash merge one PR is one commit, simple, clean and easy to roll back or cherry-pick to another branch.

seba_dos1 8 hours ago | parent | next [-]

These commits reaching the reviewer are a sign of either not knowing how to use git or not respecting their time. You clean things up and split into logical chunks when you get ready to push into a shared place.

theshrike79 7 hours ago | parent | next [-]

Why would the reviewer look at the commit messages instead of the code?

1. Open PR page in whatever tool you're using

2. Read title + description to see what's up

3. Swap to diff and start reading through the changes

4. Comment and/or approve

I've never heard anyone bothering to read the previous commit messages for a second, why would they care?

jfultz 5 hours ago | parent | next [-]

In some cases, reviewing PR diffs commit-by-commit (and with the logs as the narration of the diff-by-diff story) is a substantial improvement over reviewing the entire PR diff. Concrete examples...

* A method or function that has code you realize needs to be shared...the code may need to be moved and also modified to accommodate its shared purpose. Separating the migration from any substantive modifications allows you to review the migration commit with the assistance of git's diff.colorMoved feature. It becomes easier to understand what changes are due to the migration, and what changes were added for more effective sharing.

* PRs sometimes contain mechanical work that is easy to review in isolation. Added or removed arguments, function renames, etc. No big deal if it's two or three instances, but if it's dozens or hundreds of instances, it's easier for the humans to review all of those consistent changes together, rather than having them mixed in with other things one has to reason about.

* Sometimes a flow of commits can help follow a difficult chain of reasoning. PR developer claims that condition X can never occur, but the code is complex enough that it's difficult to verify. However, by transforming the code in targeted ways that are possible to reason about, the complexity might be reducible to the point where the claim becomes obvious. One frequent example I see of this is of function/method arguments that are actually unnecessary, but it wasn't obvious until after some code transformations.

seba_dos1 7 hours ago | parent | prev | next [-]

Because it's a useful abstraction. If you only look at PRs and don't ever care about commits, why are they even being sent to reviewer in the first place? Just send a diff file.

Having atomic commits lets you actually benefit from having them. Suddenly you don't have to perform weird dances with interconnected PRs with dependencies as "PR too big" is not such a problem anymore as long as commits are digestible; you can have things property bisectable; you can preserve shared authorship; you can range-diff and have a better view on what and how changed between review passes, and so on...

The unit of change is commit, and PRs group commits you want someone to pull. If you don't want or need any of that, you're just sending a patch file in a needlessly elaborate way.

Anon1096 6 hours ago | parent [-]

> If you only look at PRs and don't ever care about commits, why are they even being sent to reviewer in the first place? Just send a diff file.

This is in fact what hg does with amending changesets and yes it works far better. Keep PRs small and atomic and you never need to worry about what happens intra-pr. If you need bigger units of work that's what stacking is for.

seba_dos1 6 hours ago | parent [-]

Stacking is good for expressing dependencies, but isn't helpful when you need to make several distinct changes that aren't necessarily needed unless you take them all in. What's the value in having a separate PR that introduces a framework that you later use in another PR when you may not actually want to merge it if the latter one doesn't end up being merged as well?

A PR is a group of commits, just utilize that when you need it.

ipsento606 6 hours ago | parent | prev [-]

>Swap to diff and start reading through the changes

this forces the reviewer to view the entire diff at once, which can greatly increase the cognitive load vs. being able to view diffs of logical units of work

for tiny PRs it may not matter, but for substantial PRs it can matter a lot

croemer 8 hours ago | parent | prev | next [-]

What if the shared place is the place where you run a bunch of CI? Then you push your work early to a branch to see the results, fix them etc.

seba_dos1 8 hours ago | parent | next [-]

You can do whatever you want with stuff nobody else looks at. I do too.

I meant "shared place" as an open review request or a shared branch rather than shared underlying infrastructure. Shared by people's minds.

mr_mitm 8 hours ago | parent | prev [-]

You can always force-push a cleaned up version of your branch when you are ready for review, or start a new one and delete the WIP one.

croemer 7 hours ago | parent | next [-]

You can, but instead you can also just squash merge in one click. And avoid that people merge there dozens of fixes if you allow anything but squash merge.

theshrike79 7 hours ago | parent | prev [-]

I hate (and fear) force-pushing and "cleaning up" git history as much as other people dislike squash-merging =)

It just feels wrong to force push, destroying stuff that used to be there.

And I don't have the time or energy to bisect through my shitty PR commits and combine them into something clean looking - I can just squash instead.

seba_dos1 7 hours ago | parent [-]

Nothing is destroyed by a force push. It just overwrites a single pointer, and even keeps its old value in reflog.

Things that aren't referenced by anything anymore will eventually get garbage collected and actually destroyed, but you can just keep a reference somewhere to prevent that from happening if you need. Or even disable garbage collection completely.

Looks like people's fears about git come just from not knowing what it does.

Noumenon72 4 hours ago | parent [-]

You can't use the remote reflog to revert what you force pushed, can you? But I agree that having your local reflog means you're never totally lost. I still just make a branch before major edits so I can go back.

zaphirplane 8 hours ago | parent | prev | next [-]

What are examples of better ones. I don’t get the let me show the world my work and I’m not a fan of large PR

duskdozer 8 hours ago | parent [-]

if you mean better messages, it's not really that. those junk messages should be rewritten and if the commits don't stand alone, merged together with rebase. it's the "logical chunks" the parent mentioned.

it's hard to say fully, but unless a changeset is quite small or otherwise is basically 0% or 100%, there are usually smaller steps.

like kind of contrived but say you have one function that uses a helper. if there's a bug in the function, and it turns out to fix that it makes a lot more sense to change the return type of the helper, you would make commit 1 to change the return type, then commit 2 fix the bug. would these be separate PRs? probably not to me but I guess it depends on your project workflow. keeping them in separate commits even if they're small lets you bisect more easily later on in case there was some unforseen or untested problem that was introduced, leading you to smaller chunks of code to check for the cause.

orsorna 7 hours ago | parent [-]

If the code base is idempotent, I don't think showing commit history is helpful. It also makes rebases more complex than needed down the line. Thus I'd rather squash on merge.

I've never considered how an engineer approaches a problem. As long as I can understand the fundamental change and it passes preflights/CI I don't care if it was scryed from a crystal ball.

This does mean it is on the onus of the engineer to explain their change in natural language. In their own words of course.

seba_dos1 6 hours ago | parent [-]

Commits don't show "how an engineer approaches a problem". Commits are the unit of change that are supposed to go into the final repository, purposefully prepared by the engineer and presented for review. The only thing you do by squashing on merge is to artificially limit the review unit to a single commit to optimize the workflow towards people who don't know how to use git. Personally I don't think it's a good thing to optimize for.

orsorna 3 hours ago | parent [-]

Preserving commit history pre-merge only seems useful if I had to revert or rebase onto an interstitial commit. This is at odds with treating PRs as atomic changes to the code base.

I might have not stated my position correctly. When I mean "squash on merge", I mean the commit history is fully present in the PR for full scrutiny. Sometimes commits may introduce multiple changes and I can view commit ranges for each set of changes. But it takes the summation of the commits to illustrate the change the engineer is proposing. The summation is an atomic change, thus scrutinizing terms post-merge is meaningless. Squashing preserves the summation but rids of the terms.

Versioned releases on main are tagged by these summations, not their component parts.

yokoprime 8 hours ago | parent | prev [-]

Haha, good luck working with a team with more than 2 people. A good reviewer looks at the end-state and does not care about individual commits. If im curious about a specific change i just look at the blame.

tasuki 7 hours ago | parent | next [-]

> A good reviewer looks at the end-state and does not care about individual commits.

Then I must be a bad reviewer. In a past job, I had a colleague who meticulously crafted his commits - his PRs were a joy to review because I could go commit by commit in logical chunks, rather than wading through a single 3k line diff. I tried to do the same for him and hope I succeeded.

theshrike79 7 hours ago | parent | next [-]

And then someone comments on a thing, they change it and force-push another "clean" history on top and all of your work is wasted because the PR is now completely different =)

mgfist 6 hours ago | parent | prev | next [-]

Why are those not just separate PRs? Or if they really needed to be merged at once - they should still be separate PRs but on a feature branch

seba_dos1 6 hours ago | parent [-]

Why have PRs - groups of commits to pull - then if all you need is a single patch file?

mgfist 5 hours ago | parent [-]

You can, but most of us work in Github and having a PR to dump commits to is very easy and convenient. Then, when you get some feedback on your PR, you can dump more commits and it's very easy for the reviewer to see what has changed since the last time they reviewed it.

I feel like what you're arguing is that you should clean up your commits before anyone else sees them. Fair. But you could also clean it up right before merging to main. It's not that different, except the latter is much less annoying, particularly when going back and forth with people.

I know this is a very github centric workflow, but that's where most engineers work now, and it's nice and easy. This wouldn't work for eg: contributing to linux, but that's not what most of us do.

KptMarchewa 7 hours ago | parent | prev [-]

Split the PR rather than force me to wade through your commit history. Use graphite or something else that allows you to stack PRs.

jlokier 4 hours ago | parent [-]

Why is it "wade through" if there are 10 clearly distinct but dependent commits, but comfortable if it's 10 stacked PRs instead? They are basically the same thing, presented ever so slightly differently.

I think in most teams I've worked with, the majority of developers (> 85%) barely undestand what Git is doing or what things mean inside GitHub, have never seen commit history as a graph, have never run something like "git log --oneline --graph --decorate" or "--format", and have never heard of "git range-diff" which is very useful for following commit/PR/unit changes.

Personally I review using "git" itself, so I see the graph structure either way, and there's little difference between stacked PRs, commit chains in a single PR, or even feature branches, from that point of view. Even force-push branch updatea aren't difficult to review, because of the reflog and "git range-diff". The differences are mainly in what kinds of behaviour the web-based tooling promotes in the rest of the team, which does matter, and depends on the team.

I agree with you if you're using Graphite instead of GitHub. Having a place to give feedback and/or approval on the individual "units" (commits in a PR, or PRs in a stack) is useful, grouping dependent but distinct changes is useful, and diff'd commit evolution within each unit PR in response to back-end-forth review feedback is useful in some collaborative settings. Though, if you know "git range-diff" and reflog, that shows diff'd commit evolution quite well.

In GitHub, people are confused by stacked PRs both conceptually and due to the GitHub UX around them. Most times when I've posted a stacked PR to a GitHub project, other people didn't realise it was stacked, and occasinally someone else has merged the tip of a stack made by me, and been surprised to see all the dependent PRs merged automatically as a side effect. Usually before they get to reviewing those other PRs :-)

People understand commit sequences in a PR, though I've rarely seen people treat the individual commits as units for review when using GitHub, unfortunately. In the Linux kernel world where Git was born, the PR flow is completely different from GitHub: Their system tends to result in feedback on individual commits. It also encourages better quality feedback, with less nitpicking, and better quality commits.

jfengel 6 hours ago | parent | prev | next [-]

Sometimes I have to go back and fix a bug that appeared during another branch. Having the original commits helps me bisect it.

Not often, but given that it costs me nothing to have it all in my tree, I'd rather have it than not.

hhjinks 8 hours ago | parent | prev | next [-]

You review code not to verify the actual output of the code, but the code itself. For bugs, for maintainability. Commit hygiene is part of that.

seba_dos1 8 hours ago | parent | prev [-]

I have no troubles working on big FLOSS projects where reviews usually happen at the commit level :)

theshrike79 7 hours ago | parent [-]

So if a PR consists of 20 commits, they review every single commit linearly without looking at the end result first?

seba_dos1 6 hours ago | parent [-]

Yes, and in some projects 20 commits is not even a big PR, more like "regular sized". The LKML's first page is now full of PRs with around 20 commits, here's a random one as an example: https://lore.kernel.org/netdev/20260408121252.2249051-1-dhow...

And here's a slightly smaller one which isn't about "miscellaneous fixes": https://lore.kernel.org/netdev/20260408122027.80303-1-xuanzh...

Some of these commits even get reviewed by different maintainers before being merged, which is common when a patchset touches several subsystems at once.

Aachen 8 hours ago | parent | prev | next [-]

If someone uses git commits like the save function of their editor and doesn't write messages intended for reading by anyone else, it makes sense to want to hide them

For other cases, you lose the information about why things are this way. It's too verbose to //comment on every like with how it came to be this way but on (non-rare in total, but rare per line) occasion it's useful to see what the change was that made the line be like this, or even just who to potentially ask for help (when >1 person worked on a feature branch, which I'd say is common)

seba_dos1 8 hours ago | parent [-]

> If someone uses git commits like the save function of their editor

I use it like that too and yet the reviewers don't get to see these commits. Git has very powerful tools for manipulating the commit graph that many people just don't bother to learn. Imagine if I sent a patchset to the Linux Kernel Mailing List containing such "fix typo", "please work now", "wtf" patches - my shamelessness has its limits!

Aachen 7 hours ago | parent [-]

Seems like a lot of extra effort (save, add, commit, come up with some message even if it's a prayer to work now) only to undo it again later and create a patch or alternate history out of the final version. Why bother with the intermediate commits if you're not planning for it to be part of the history?

seba_dos1 6 hours ago | parent | next [-]

Git is a version control system. It does not care about what it versions.

When I work on something, I commit often and use the commit graph as a undo tool on steroids. I can see what I tried, I can cherry-pick or revert stuff while experimenting, I can leave promising but unfinished stuff to look at later, or I can just commit to have a simple way to send stuff to CI, or a remote backup synced between machines.

Once I'm done working on something, it's time to take a step back, look at the big picture, see how many changes my experiments have actually yielded, separate them, describe and decide whether they go to review together or split in some way, as sometimes working on a single thing requires multiple distinct changes (one PR with multiple commits), but sometimes working in a single session yields fixes for multiple unrelated issues (several PRs). Only then it gets presented to the reviewer.

It just happens that I can do both these distinct jobs with a single tool.

thi2 6 hours ago | parent | prev | next [-]

Because I might want to go back to this current messy state but I don't want to commit it like this (hardcoded test strings, debug logs, cutted corners to see if something works, you name it).

I simply commit something like "WIP: testing xy" and if its working and properly implemented i can squash/rebase/edit the commit message and force push it to my feature branch. Using a Git client like Gitkraken makes this incredibly easy, takes seconds.

This way I can leverage version control without committing bogus states to the final PR.

skydhash 6 hours ago | parent | prev [-]

If the team is using a PR workflow, the PR is a working place to produce one single commit. The individual commits are just timestamped changes and comments. Think of it as the equivalent of annotated diff in mailing list conversation.

BeetleB 3 hours ago | parent | prev | next [-]

Trivial and not too silly example:

Part of new feature you had working in an intermediate commit, but broke somewhere along the way and is not working in your last commit when you squashed.

If you catch it early enough, I suppose it's in your reflog, but otherwise you're screwed.

It sounds like a silly example, but I bet most developers have run into this at some point.

With mercurial/jujutsu, you get the best of both worlds: The "argh, let's see if this works" commits are what I call "microcommits", and the squashed versions are the real/public commits. With jujutsu, you get both. Your log shows only the "real" commits (equivalent of squashing all the commits between that and the prior "real" commit). But if you want to drill down into the microcommits, the information is always there.

Let's acknowledge the reality. Many people use git not just for version control, but for backup ("Let me commit this so I don't lose it"). Let's ensure the VC tool supports both and doesn't force you to pick one over the other.

tasuki 7 hours ago | parent | prev | next [-]

You gain the extra information by having reasonable commit messages rather than the ones you mentioned. To fix CI you force push.

Can you explain to me what an avid squash-merger puts into the commit message of the squashed commit composed of commits "argh, let's see if this works", "crap, the CI is failing again, small fix to see if it works", and "pushing before leaving for vacation" ?

theshrike79 7 hours ago | parent [-]

The squashed commit from the PR -> main will have a clean title + description that says what was added.

Usually pretty close to what the PR title + description are actually, just without the videos and screenshots.

Example:

feat(ui): Add support for tagging users

* Users can be tagged via the user page * User tags visible in search results (configurable)

etc..

I don't need to spend extra time cleaning up my git commits and force-pushing on the PR branch, losing context for code reviews etc. Nor does anyone have to see my shitty angry commits when I tried to figure out why Playwright tests ran on my machine and failed in the CI for 10 commits.

5 hours ago | parent [-]
[deleted]
joshstrange 5 hours ago | parent | prev | next [-]

> "argh, let's see if this works", "crap, the CI is failing again, small fix to see if it works", "pushing before leaving for vacation"

These are all bad commits IMHO. Aside from the CI one, I understand that message. I have commits like that on personal projects but for professional projects I'd be frustrated if people were committing messages like that.

Personally I'm a "one commit" type of guy, I don't like committing things in a broken state even on a side branch unless I have to (to share the code or test a CI). Occasionally I will make multiple commits at the very end to make review easier or once I have everything working but I want to try something different but I have a bunch of options of saving code that don't involving committing:

- Stash

- Shevle (IDEA)

- Backblaze

- Time Machine

- Local History (IDEA)

The idea of committing WIP before leaving for a vacation just feels so wrong to me.

I once worked for someone who wanted developers to commit code before the end of every day as a safety measure. His reasoning was in case the developer's computer died or similar. I found that silly at the time and still do now. That's what backups are for, I dislike when people use git as a backup like that in a professional setting.

7 hours ago | parent | prev | next [-]
[deleted]
thi2 6 hours ago | parent | prev | next [-]

Why are those commits ending in the PR? Just unprofessional to work like that.

psalaun 4 hours ago | parent | prev [-]

git bisect gets more useful because it will pin a smaller set of changes