211–220 of 278 posts
'git rebase -i' meets this need and more. Everyone using git should learn to use it eventually. With it you can squash, fixup, reword, or delete commits interactively.
'git rebase -i' meets this need and more. Everyone using git should learn to use it eventually. With it you can squash, fixup, reword, or delete commits interactively.
Going back in time to fix the past just to sort your changes into the "correct" bin or vicinity of changes just seems unnecessarily complex. I am also unaware of what use-cases this supports. Are teams out there forcing commits to have a stricter scope than just "anything in the codebase"? Perhaps it's a monorepo pattern? I have no idea.
Either way, you have to `git push --force` to modify your PR/MR. May as well use the stock workflow and learn how to clean up your branch.
I also endorse `git commit --amend` for anyone that identifies as a "micro-committer" but doesn't need to preserve intermediate changes.
Edit: re-reading comments - it sounds sort of like some teams prefer to bin changes like this in order to make a PR/MR browsable by commit, rather than inspect one big broad diff. Is that the case?
Earlier quoted context omitted.
> At the risk of sounding judgemental, I think this preference for always squashing PRs comes from a place of either not understanding atomic commits, not caring about the benefits of them, or just choosing to be lazy. In any case, the loss of history inevitably comes at a cost of making reverting and cherry-picking changes more difficult later, as well as losing the context of why a change was made. 1) Why are you e…
> The PR is the thing that gets signed-off on, and the thing that goes through the CI build/tests, so why wouldn't that be the thing kept as an atomic unit? Because it often isn't. I don't know about your experience, but in all the teams I've worked in throughout my career the discipline to keep PRs atomic is almost never maintained, and sometimes just doesn't make sense. Sometimes you start working on a change, but…
In my experience at least, PRs are atomic in that they always leave main in a "good state" (where good is pretty loosely defined as 'the tests had to pass once').
Sometimes you might make a few small changes in a PR, but they still go through a review. If they're too big, you might ask someone to split it out into two PRs.
Obviously special cases exist for things like large refactoring, but those should be rare and can be handled on a case by case basis.
But regardless, even if a PR has multiple small changes, I wouldn't revert or cherry-pick just part of it. Just do the whole thing or not at all.
> Oh, plenty. For one, when looking at `git blame` to determine why a change was made, I hope to find this information in the commit message. This is what commit messages are for anyway. If all commits have this information, following the history of a set of changes becomes much easier. This is helpful not just during code reviews, but after the merge as well, for any new members of the team trying to understand the codebase, or even the author themself in the future.
Yeah but the context for `git blame` is still there when doing a squash merge, and the commit message should still be relevant and useful.
My point isn't that history isn't useful, it's that the specific individual commits that make up a PR don't provide more useful context than the combined PR commit itself does.
I don't need to know that a typo was fixed in iteration 5 of feedback in the PR that was introduced in iteration 3. It's not relevant once the PR is merged.
The negativity in the comments here is unwarranted in my opinion. I've been using `git absorb` for years and it works amazingly well. I use it in addition to manual fixups. My most common uses of git-absorb, but definitely not the only, are when I submit a PR with multiple commits and it fails CI for whatever reason. If fixing CI requires changes across multiple commits (say, lint violations), then git-absorb will al…
What is wrong with simply pushing a "Fix linting issues" in a new commit? It's self-contained and very well describes the (single) purpose of the commit. I share the sentiment about the "logical small commits", and hence I don't see that adding a new fix commit is problematic as long as it is self-contained and purposeful, but perhaps I don't understand what is the problem that this tool is trying to solve. It says >…
You could perhaps add some kind of filter to git blame to automatically skip commits whose message is “fix lint” but at some point the cure is worse than the disease.
I also see people argue that merge commits make git bisect harder than squashing in the first place but there is a third option: rebase and fast forward. If every commit in your branch is clean and stand-alone that’s viable. Linter fix commits break that paradigm.
Earlier quoted context omitted.
Yeah. But for that we can squash everything into a single commit on merge. Instead of spending a bunch on time making every single commit in an MR perfect both in isolation and all-together. And if squashing everything in the MR causes a problem with the git history being too coarse, then that is almost certainly because the MR itself should have been split up into multiple MRs. Not because of how you organized the i…
The goal isn't perfection. Splitting PRs has overhead, especially depending on what code hosting platform you're using. Do you advocate for making code easier to read and understand by humans? What would you do if someone told you, "no I don't want to waste my time trying to make it perfect." It's the same misunderstanding. I try to treat code history like I treat code: I do my best to make it comprehensible to human…
If the resulting squash is too big / touches too many things, it’s because I didn’t split the MR where I probably should have.
Because to me, the bigger waste of time is fiddling with all of the commits in the MR just to make it so that the MR can be merged without squashing.
If someone needs fine-grained history about how exactly a specific feature or bug fix came to be, they can always go to the MR itself and look at what happened there.
Earlier quoted context omitted.
If those are all different commits, now imagine the fun of interactively rebasing them on top of a main branch where some other thing changed "Hello, World" to "Greetings dear Globe."
Why is someone else writing my feature? Do we not have a ticket system? ;-) More importantly, when using a feature branch one should really rebase master into the feature branch regularly, and any other branches that might touch the same places, it takes care of little surprises like this.
When you need to rebase that onto main/master, there would be 5-6 stops to fix conflicts in a 3-way diff, and I don't think the kind of work in that example is compelling enough to justify that as opposed to having one (squashed) commit that simply states that you improved the method in several ways.
P.S.: Sometimes more commits can make refactoring easier instead, often when it comes to splitting file rename/move operations from changing their content.
Earlier quoted context omitted.
The goal isn't perfection. Splitting PRs has overhead, especially depending on what code hosting platform you're using. Do you advocate for making code easier to read and understand by humans? What would you do if someone told you, "no I don't want to waste my time trying to make it perfect." It's the same misunderstanding. I try to treat code history like I treat code: I do my best to make it comprehensible to human…
I make multiple commits in the PR and squash it all on merge. If the resulting squash is too big / touches too many things, it’s because I didn’t split the MR where I probably should have. Because to me, the bigger waste of time is fiddling with all of the commits in the MR just to make it so that the MR can be merged without squashing. If someone needs fine-grained history about how exactly a specific feature or bug…
Sometimes I do that. But sometimes I want the history I've curated in my PR to be preserved without the overhead of creating separate PRs (which then often need to be stacked, introducing problems of its own). In which case, I avoid things like "fixup lint" commits. And that's where git-absorb shines. And saving time is exactly the point! git-absorb helps you curate history faster.
> If someone needs fine-grained history about how exactly a specific feature or bug fix came to be, they can always go to the MR itself and look at what happened there.
That's a lot more work than just reading the commit log. But I agree, one can do this ifneedbe. But it's nice to avoid when possible.
Earlier quoted context omitted.
I’ve been using autofixup for this and it’s been ok but not great, it can be quite slow as things grown, and it doesn’t say anything when there was no match so it’s easy to miss. How does absorb surface that? > Perhaps partially due to how GitHub works. That’s definitely a major factor, I’d like to use stacked PRs they sound really neat, but GitHub. Also even with stacked PRs I figure sometimes you’re at the top of t…
The term Stacked PRs already sounds like a term that was invented specifically in order to communicate in a GitHub-influenced context. Because Stacked PRs are just a reinvention of being able to review a commit at a time (the stack part is straightforward).
That additional layer allows finer grained history and to mostly avoid (unreviewed) rebasing. Many teams find those properties valuable.
Earlier quoted context omitted.
It sounds like you have never heard of the Stacked Diffs workflow. Here is an overview: https://newsletter.pragmaticengineer.com/p/stacked-diffs It’s a perfectly reasonable preference to eschew this workflow, but definitely worth understanding the pros and cons.
Every explanation of stacked diffs I've seen (including this one) makes me think there is some secret magic not being shared that will make the whole idea "click". If the problem is devs having to wait too long for PRs to be reviewed, breaking tasks up in to smaller diffs should exacerbate the problem, wouldn't it? If diff 2..n follow diff 1 then a review of 1 blocks everything else you're doing, eh? At least the tra…
> If diff 2..n follow diff 1 then a review of 1 blocks everything else you're doing, eh? At least the traditional route, branching everything straight off of main, means you can merge 2..n and rebase 1 once it passes, leaving you less blocked.
I am a little confused about the scenario you're describing here: with the PR based flow, you can't merge the second half of the PR while waiting on some change to the first commit.
So let me try to suggest a scenario. We're working on a web app. We're going to add a new feature. This requires some backend changes, and then some front end changes that use those back end changes. For the immediate purposes here, we're going to presume these are in the same PR. I'll talk about them as two PRs in a moment. We're also going to assume for the sake of simplicity that there's two commits in this PR: one for backend, one for frontend.
With the PR workflow, you're asking for review from two people: a backend expert, and a frontend expert. Both are going to request changes, and you're gonna respond to all of them, and eventually things get to a good place and the PR is merged. Let's say that process takes a week.
There's a few possible issues here: the first is, both people are going to get notifications for every change in the PR, even if it's not in the code that they need to review. This creates additional review fatigue for both reviewers. They also may need to check what's happened since the last time they reviewed things, and github doesn't always make that easy, though they do help sometimes in some circumstances. Another issue is, say that the backend changes are good to go, but there's more frontend work to do: because it's in the same PR, the backend change will not be integrated until the frontend change is done. And the backend reviewer will still be getting those pings. Notably, if the frontend change is ready first, they'll also be getting pings from more backend reviews too.
In the stacked diff flow, and using those tools, each reviewer only needs to review their half individually. They will not get pinged for changes to the other half. They will be presented with only the diffs that affect the parts that they need to review. And, because you can land each diff in the stack whenever it's at the top (or bottom, depending on how you visualize things, I guess) of the stack, as soon as the backend commit is good to go, it can land, and the frontend commit can land at a later time. We've popped it off the stack. And no matter who finishes first, they won't get review pings for the other part's work.
So, you might say: yeah, that's exactly why I'd make them as two separate PRs. And that may be a good idea. But to do that, you have two choices: wait until the backend stuff is fully done, and then start on the frontend stuff. That can work, sure, but maybe what you want to do needs the frontend work to influence the backend work, so treating it as one logical change before any of it hits master makes sense. So to do that, you make a frontend PR that, instead of branching off of master, branches off of the backend PR. Well, now you've reinvented stacked diffs, but you don't have any of the tooling to make that nice! Once the backend lands, you'll have to rebase the frontend off of master, stuff like that.
... does any of that make sense?
Earlier quoted context omitted.
I make multiple commits in the PR and squash it all on merge. If the resulting squash is too big / touches too many things, it’s because I didn’t split the MR where I probably should have. Because to me, the bigger waste of time is fiddling with all of the commits in the MR just to make it so that the MR can be merged without squashing. If someone needs fine-grained history about how exactly a specific feature or bug…
> I make multiple commits in the PR and squash it all on merge. Sometimes I do that. But sometimes I want the history I've curated in my PR to be preserved without the overhead of creating separate PRs (which then often need to be stacked, introducing problems of its own). In which case, I avoid things like "fixup lint" commits. And that's where git-absorb shines. And saving time is exactly the point! git-absorb help…
Yeah, that’s true. I suppose that we may be working in somewhat different environments.
For example, when someone has big public projects that get a lot of eyeballs on them, like your ripgrep and other projects, it makes a lot of sense to spend extra time making the git log a thing that can be read on its own completely offline.
For the things I work on at my job, there are just a few of us writing the code itself and those coworkers that work on the repos I do will usually check every MR from everyone else. And anyone else in the company working on other repos is usually likely to browse code via the GitLab anyway, if they are interested in actually looking at any of the code that are in the repos of my team. Onboarding new coworkers on the team is also mostly centered around the docs we have, and talking with other people in the team and the company, rather than digging through the git history.
And for me when I am looking to understand some code from our internal repos it’s usually that way too, that I use the GitLab UI a lot, and I look at the MRs that were merged, and I check the associated Jira ticket, and I might ask some coworkers on Slack to explain something.
Most of the time, no one outside of our team is very interested in the code of our repos at all. Discussions revolve around other aspects like the APIs we expose, and the schema of the data we store in tables in the databases, and the Kafka events we produce. That sort of thing.
And everyone at my job will necessarily need to be online when they are working anyways, so that they can use Slack etc. So we are one click away from our GitLab instance when we work. On the off-chance that someone is trying to do some work offline while being on the go and without Internet access available, they probably have some specific kind of work in mind that is sufficiently independent of specific details that they can do without that history.