Maybe I am being to much of a purist, but retroactively modifying commits and history? Why? Stuff happens, so do mistakes. Fix the mistakes, make another commit, and go on with your life.
Logical change 1, implement Hello World:
puts "Hello, World!"
def hello_world
puts "Hello, World"
end
def hello_world(greeting)
puts "#{greeting}, World"
end
def hello_world(greeting, recipient)
puts "#{greeting}, #{recipient}"
end
def greet_recipient(greeting, recipient)
puts "#{greeting}, #{recipient}"
end
def greet_recipient(greeting="Hello", recipient)
puts "#{greeting}, #{recipient}"
end
def greet_recipient(greeting="Hell", recipient)
puts "#{greeting}, #{recipient}"
end
def greet_recipient(greeting="Hello", recipient)
puts "#{greeting}, #{recipient}"
end
Personally, I make numerous commits in a feature branch, (transparently, too, my typos included - I'm not proud:) and before requesting review I'll clean up using rebase into as few logical commits as possible, and upon acceptance either squash it all to one or leave it as is, depending on the team's culture/needs.
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.
Maybe we do, maybe we don't. Maybe there are two tickets with a mutual dependence.
Even without that, if you add a parameter to a method, and later someone uses it in a totally different PR, then you effectively can't revert the creation of the parameter. How are you going to track that dependency. I've heard the arguments about logical commits and the ability to cleanly revert them. I just haven't seen any code where that would actually work, even if the trouble was taking artisanally curate the commit history.
If I'm in a workplace like that then I wouldn't be surprised to find a poor git flow.
> How are you going to track that dependency.
The commit graph (which is linear due to rebasing). The extra parameter commit comes before the commits using it. If you want to remove that parameter then you have to revert the features that rely on it or refactor them, there's no magical way to keep dependent code if you want to revert the code it depends on. Having logically atomic commits makes this easier, not more difficult.
> Maybe there are two tickets with a mutual dependence.
Then rebase on each other's work until its ready for review (or, even better, don't work on dependent features in the same sprint). Again, logical, atomic commits would make that easier.
The real point is, what do you do when someone introduces a parameter in two functions, one is related to your work but the other isn't, and they bundled both changes in one commit, and the changes aren't even related. Now that's a mess.
If we use commit ancestry to determine logical dependence then this "revert the bug commit" thing won't work unless the bug just so happens to be in the very last commit.
As I've already pointed out, I'm not sure what you're arguing against but it's none of the points made in this thread.
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.
It messes up the team's ability to use tools like git-bisect to pinpoint behavior changes, and is generally more noise than signal.
At a bare minimum, those spans should get squashed before they leave the PR or feature branch.
Unless of course your changes were merged to common code already - then they become not touchable - just make a new commit with fix don’t try to be smart, common code rolls only forward and fixes are new commits.
Reviewing a clean set of commits is much easier than a branch full of mistakes and wrong paths taken.
Useful when we optimize for reviewing and good history for future maintenance. This has been important and useful when I’ve worked on big mission critical backend system, but I also understand it might not be the most important factor for a new project’s success.
Debatably, if you’re making changes during a PR review, it could be helpful to make those changes in relevant commits. That way if someone goes through them during the PR, they get one clean “story”, rather than see the pre-PR commits and the conversation after.
The primary obstacle to doing this for me is, if I'm honest, not having automated it sufficiently that I can't forget to do that.
It's tricky to review individual commits on Github and Gitlab and they don't even allow you to leave comments on the commit messages.
In areas where people do review individual commits they tend to use tools that support that workflow. Git uses email patches where each commit is sent as a separate email. Tools like Gerrit do code review directly on a commit by commit basis and the usual strategy to get it into trunk is to either "fast forward" (advance the HEAD of the trunk branch to that commit) or to cherry-pick it.
In this workflow you review every commit and not just the branch diff. Each commit is crafted carefully, and a well crafter series of commits can make even very large changes a brief to review.
It takes a certain skill to do this well. As the page above says > Crafting patches is one of the core activities in contributing code to the kernel, it takes practice and thought.
This is in contrast to using git more as a distributed filesystem where you don't care particularly much about the history, and you typically squash commits before merging etc. It's simpler and easier to work this way, but you lose some of the nice attributes of the linux kernel workflow.
What I don’t like about the Git documentation as I’ve read it is that they go between “patch” and “commit” in some places without stopping and explaining what the difference is. It makes sense to them. It’s obvious. But it isn’t necessarily obvious to most people.
A patch is a patch proper plus a commit message encoded in a format that git am understands. That’s fine. And the core developers understand that you cannot transmit a commit snapshot via email (or you shouldn’t). But I prefer to mostly stick to “commit” in the abstract sense, whether that to-be-commit is from a pull or from an email (or: it’s in the form of an email and it could be applied as a commit).
git rebase talks about “patch series” I think. Without explaining it. Why not “commit series”?
Sometimes it seems like talking about your changes by the way it happened to be transmitted. It’s like talking about “attachments” instead of commits because you happened to send them via email as an attachment (instead of inline).
Then you now have “stacked diffs” or “stacked commits”. Which are just a series of commits. Or a branch of commits (implicitly grounded by a base commit). For a while I was wondering what stacked diffs/stacked PRs/stacked patches and if I was missing out. When it just turned out to be, as you explain, essentially the Linux Kernel style of being able to review a commit in isolation. But in a sort of context that pull request inhabitants can understand.
I prefer to mostly talk about these things as “commits”.
(At several times writing those paragraphs above I wondered if I would be able to string together them in a coherent way)
However, sometimes the details matter. For example, a 'patch' (diff + description) tends to be small enough to transfer conveniently, and human friendly. Patches do not describe their relationship to other patches, so it makes sense to talk about a series of patches which must be applied in sequence to accomplish some larger goal. 'patch' doesn't imply any particular storage format, so sometimes saying 'attachment' or 'patch file' or 'email body' is an important distinction. Git documentation assumes you know what you want. It might help to think of patches as "outside time", though of course any particular version of a patch will only apply to a subset of all snapshots.
A 'commit' (snapshot + description), on the other hand, tends to be large in that it describes the entire state of the codebase and contains (implicit or explicit) metadata describing its predecessor(s) -- possibly a large graph of them. People talk about a 'commit series' all the time, but use the terms 'history' and 'branch' instead.
Stacked PRs are built on top of commits and branches to gain the advantages of a patch series while retaining the advantages of snapshots. There's no industry consensus on if they are preferable, let alone how to best implement them (eg. rebase, rebase+squash, merge-squash, merge+first-parent, etc.), so different people have different ideas about what they look like. It isn't correct to say they are just a series of commits, because sometimes they are implemented as a series of (implicit or anonymous) branches. One of the few agreed-upon features of stacked PRs (or its other names) is that it is a sequence of 'changes' which are ordered to both satisfy change dependencies and broken into smaller pieces which "tells a story" to reviewers.
The Linux Kernel is heavily built around reviewing each commit individually via patchsets sent as emails and rewriting history to make it cleaner.
Git history is incredibly valuable if its curated, at my previous employer we used Gerrit and pretty much every change would have a well crafted multi-line commit message that explained what was being changed and most importantly _why_. If I wanted to understand why a bit of the codebase had been authored in a particular way it was incredibly easy because I could just git blame the line and there'd be a detailed commit message explaining the rationale.
In the Pull Request world I do agree with what you say - but only if you squash merge. Commits that say "fix typo" or "oops" or "wip" aren't useful long term.
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.
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 traditional route, branching everything straight off of main, means you can merge 2..n and rebase 1 once it passes, leaving you less blocked.
There must be something I'm missing because there are a lot of people promoting stacking diffs.
I find small diffs to be much easier to review and as a result I review them much practically as soon as they are posted. And I'm less likely to miss something.
> 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?
As I understand it, based on what's in the article anyway, diff 2 is started from diff 1 even when they are totally different tasks, so you'll be blocked in the sense that you'll have to rebase or merge if the review finds a problem.
I figure you might as well branch 2 off of main while you wait for the go-ahead to merge 1 into main. Stacked diffs don't seem to meaningfully change this flow.
> 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.
I guess part of it is I have never worked in an environment where the frontend and backend work didn't need to be closely coordinated anyway, so those pings aren't a big deal. Everyone needs to know what's going on, to minimize rework.
> ... does any of that make sense?
I dunno. Maybe my brain isn't compatible, heh.
Ah, so sure, if you're talking about two separate pull requests, then that makes sense. But the equivalent with stacked diffs just simply wouldn't be stacked. These discussions are inherently on work that is dependent on previous work.
> so those pings aren't a big deal.
Yeah I wasn't sure if my specific suggestion would be too specific. Examples are hard.
> Maybe my brain isn't compatible, heh.
It's all good, I felt the same way for a long time. Maybe if you are bored and play around with Gerrit, (which is admittedly a bit janky) it'll make more sense. Or maybe not :)
When getting merge approval takes at least one or two business days (say because of time zones or your colleagues are doing deep work which they don't want to drop every fifteen minutes to do a review) the pipelining is necessary to be unblocked.
Especially when you follow best practices like splitting refactors from functional work; the canonical stacked diff for me is “refactor to make the upcoming change easy; add some tests for the existing functionality I want to protect; small diff to actually make the change”.
1 & 2 don’t generally require caffeine to review. 3 probably requires engaging the brain. If you put everything together you need to be on high alert for the whole review.
There's zero value in retaining mistakes that were never merged into `master`. In fact there is negative value because it makes the history harder to follow.
For example would you rather see "review fixes" in Git blame, or the actual useful commit message?
You can argue that you can't be bothered to make your history nice; fine. But you can't say it's wrong to do that.
(Personally I think you should squash commits for a MR; if it's too big to review as one commit it's too big for one MR - except for branches that multiple people have worked on over an extended period.)
Not to be too pedantic about this, it sounds like you mean flawed commits that were merged into master, but never had a chance to be the HEAD of master? (In other words, they always arrived along with a fix-commit too, so that nobody checking out code could've hit the bug.)
> In fact there is negative value because it makes the history harder to follow.
It also hampers your ability to use tools like git-bisect to detect when behavior changed, especially if some of the bad commits won't compile or pass unit tests.
Yeah exactly.
I've used it several times to understand why things are done the way they are.
The discussion leading to this design is encoded in the revisions of the patch series, browsable on patchworks.
I agree to your point that the git history has to be taken care of to be useful.
"What did John do on Tuesday morning" is not a good reason to keep history. Neither is "This change was made as part of a sprawling "fix" commit that happened on Thursday afternoon".
The important point is if you are going to keep history at all it needs to be kept in a state that is actually useful, meaning useful for the first two things, not the last two. Otherwise it's just a waste of disk space. The git history is meticulously maintained which is what makes it actually useful.
- Thread: This helps you make a useful Git history
- Counter-point: Why are you using a version control system to make a useful history? Why does the history matter? I have been developing software for eighty years and neither I nor anyone I’ve met have cared.
And one key to good git history is people splitting up big PRs in multiple commits.
But if not even reviewers are looking at commits I question whether a PR should be chunked in commits at all - why not squash all commits into a single one, so that every PR is composed of exactly one commit? Could maybe separating the changes in commits convey some information?