Reorient GitHub pull requests around changesets(mitchellh.com) |
Reorient GitHub pull requests around changesets(mitchellh.com) |
Still many of the same problems occur.
Fundamentally, I think the git emperor has no clothes. Managing commits is just too tedious, but squashing them causes too much pain.
I find that I fairly often notice some minor issues, like a typo in a comment. I do want the submitter to fix those, but making them go through a full review cycles is an unfortunate waste of time as I usually trust them to be adult enough to just make the fix and submit.
So I find myself wanting to say "Approve modulo these minor issues". If a project wants to allow this, it needs to either not require approvals, or allow approvals to remain even after changes to a PR.
Github makes it easy to review just the changes since your last approval, a feature which I think obviates the need for changesets as in the OP.
I don’t do much reviews these days but I remember the confusion of GitHub PRs. Things just disappear, especially with the rebase workflow which is preferable for improving reviewer burden.
I think Mitchell is mostly right here: a changeset is the most natural data structure for maintaining code. However, is it also the most natural construct for storing code in repositories, ie “should git be replaced in the long term?”. Can the merkle tree of blobs be replaced by an analogous merkle tree of changesets? That I don’t know. But it’s a worthwhile idea.
Sign me up for change sets!
Sorry for the shameless plug!
PRs are good ways to defend your code base from bad code, and they were born in open source where you literally have no clue who the contributor is, but years of experience left me convinced that I don't want such a system where there's constant need to overview each other's work.
I want to work with a team where there is high respect and trust. A team where I know I won't like or love all the decisions others make, but I trust their judgement. Maybe they did indeed hack an ugly solution cheating the type system and automated controls. So what? What matters is if they have done so for good reasons (stuff was super urgent, a proper solution was just not worth the effort as the feature/fix was really not important for the business).
This made development speed skyrocket and I'm no longer bound to infinite code reviews as if we were sending rockets on Mars.
I also want to say, code quality is high, but this stems both from working with great individuals that can be trusted and from much higher interaction speed.
Reviewable tracks code by revisions (aka changesets), not just the state of the git branch like GitHub, and many more improvements both to the broad strokes and to the small things.
Want to immediately see which PR's you should review? Want to avoid getting pinged during the day to review PR's you could have reviewed on your schedule? Want to see the status of a PR right when you open it? What about keeping your comments on the right line even as new changesets come in so you never have to review the PR from scratch again?
All handled by Reviewable to make you a better engineer: fewer interruptions, no repeated work, and generally respond to reviewers faster
but yes, i liked perforce too.
It's distressing to me that Github spends so much money making Git worse. (There's some good parts of their whole product lineup, but the Git integration is supposed to be the centerpiece.)
Yes, there's git format-patch, git send-email, and git am.
But what I would really like to see in that link you shared is links that go directly to commit hashes that I can git fetch locally to see a patch or patch set in context; and links between different versions of a patch set; and so on. After all, git am does sometimes fail, e.g. because you have an incompatible base revision. And being able to push with confidence the version that you had in the email is also a plus.
They often are to specific blobs; those are recorded. So if they don't apply, use am -3.
Specific revisions are Junio Hamano's job, not a patch submitter's.
Might apply to huge products making millions $ every day. Sure, delivering a bug will be expensive.
Might apply when you can't trust your colleagues (not skilled, reasonable or experienced enough).
Might apply if your code is niche but mission critical (maybe some safety system on a car or a dangerous tool, or surgery equipment).
Of course I agree with all you said.
But you're writing some Java/TS web app which isn't raking much money, or has still to be launched, or your biggest focus is time to market to beat competitors and you're wasting time on code reviews the author hasn't requested?
In this scenario (my current client) I want to have a team I can trust and gets stuff done. This does not imply that CRs don't happen or design isn't discussed, but it happens when it brings value or it is needed. And I like it that way, where we can build stuff, rather discussing how to build it.
- Doesn't consider an edge case (NPEs being the most common)
- Doesn't match up well with the "standard" way we do things (and, all things being equal, using the same approach in every place improves maintainability)
- Has a simpler approach
- Doesn't have a comment where it's not clear _why_ the code is doing something (that the author of the code sees as obvious, because they wrote it)
- Has misleading method/class/field names
These are all things that I've seen from both myself and others on my team. Having one other team member take a look at my code before it goes to QA can save a lot of time later.
Who cares about an edge case no user's gonna see?
Who cares about an edge case for a non business critical feature?
Who cares if there was a better way to implement something if it's not a priority?
Our work it's not meant to give us devs intellectual challenges but solve people's problems.
All the things you list are reasonable if you're making money or they are core to the product or they impact user's safety in any way.
Otherwise they don't matter at all.
And half the things you list are solved by having high hiring standards and paying people well.
Stuff may not be perfect, maybe there will be better ways, maybe it will be inconsistent at times but it will be more than good enough and move the business forward at much higher speed. Also, just to reiterate I've never stated that we don't have standards or make architectural decisions or don't give feedback. I merely stated that reviewing PRs for us is an exception, not a rule and that I prefer a system of trust.
I have been on teams where we simply merged things when complete and where we required code review before merge. In every case, the latter had better overall speed because we weren’t (usually) introducing regressions because no one had seen the code. (Pairing is a form of real-time code review, but the team I worked on that used pairing heavily still needed code review; I remember a regression was introduced even though the code had been pair-developed.)
There is, however, a case where if one works in an industry where ISO270001 or SOC2 or PCI compliance is required, recommended, whatever — you will have to have a policy on code review in order to pass.
However, the approach of few/no code reviews doesn’t work for large open source projects that accept external contributions (the author of the post is mitchellh who is a founder of HashiCorp, which has/had several large open source projects).
Edge cases matter sometimes matter and sometimes do not. They particularly matter in cases where, if the happen, the leave the system in a bad state (corrupt data, etc). Having a second pair of eyes take a look at the code and think "where can this go wrong" doesn't cost much, but can save a lot of pain later.
Doing things in a simpler way and/or a way that matches the way we generally do things makes it easier to maintain. And it's fairly common for code maintenance and/or further development on the same code, to be more common than writing brand new functionality (code that doesn't touch existing code). This is directly related to the "grind to a halt" I mentioned earlier.
Literally nothing I mentioned can be solved by hiring standards or pay. Once, because hiring people new to software development is a thing. Two, because everyone... EVERYONE makes mistakes. And EVERYONE can do better with the help of their peers.
I prefer that every PR get reviewed by someone, with the (uncommon) exception of those where the developer says it isn't worth a review. It generally takes very little time, and it adds to delivered code quality.
It'd be a useful thing to import without having to bring in the whole charade of using email and mailing lists (which most mail clients tend to be very unfavorable of in general nowadays) - there's real advantage to doing this in a web interface instead.
UX would probably be more difficult though. The current workflow of "submit branch, make PR, do changes on same branch, merge latest version through web interface" is a big part of the ease of the Github UX. Doing a merge outside of that just by pulling in the right remote branches has always been a crapshoot at best and a pain in the ass at worst. Not helped by the fact that Github's documentation on how to do it in git is obscure (intentional I'm sure; I know it's possible but the docs are scattered and all of it recommend just using their gh CLI tool at this point).
That was my first thought, too. Though perhaps a simple solution could be having GitHub, on a push, check for a new branch that matches an existing branch from a PR plus “-v2” (or “-v3”, etc.) and automatically consider that a new changeset in the PR.
Or perhaps even easier is once you’ve “released” your changeset in the GitHub ui, any pushes to the branch implicitly duplicate it and put it into a new v2 branch instead. That would be a decent ui from the pushers point of view, though there’s an and asymmetry between what you push and what ends up in the repo that I’m not sure I like.
This is a misconception. git itself has commands (git-format-patch and git-send-email) that automate the creation of patches and sending changeset email threads to the mailing list. The only thing one needs to do is set the appropriate configuration settings in their git config (which is a one time operation like setting your name and email address).
The actual interaction on the mailing list (responding to those who review patches and changesets can be done in any email client of one's choosing (though it's helpful to use an email client that supports threading using the Message-Id, In-Reply-To and Reference headers rather than one that only handles conversation view style replies).
I recently commented on a juniors code, and put in about four lines of code showing how I would improve the performance- but to do so I of course had to bring up a REPL and put the code in and run it and had more or less done a PR.
Make it faster and easier to inject and piece of code into the PR flow (branch pu?), make the whole code base simple to run from any point in memory, fully testrigged, so "just those highlighted lines" can be run and everyone can see.
What we are doing with this OP approach is making it easier for a human brain to imagine what the runtime will be like. That's the wrong approach - make it easier to have the runtime run over the chnaged lines of code and let people step through at PR time, and then make their changes alongside.
Or just leave a comment.
But we know what the rules are on comments on the internet
Another obvious way to fix OP’s issue is to request developers to break their PR’s down into smaller chunks, pair on the parts that need help, and slowly merge things from there. YMMV of course - but any 1000 line PR is going to be a headache regardless of what methodology you’re using to review code.
It would be wonderful if we could ditch "change these lines" type comments in favour of just letting merge requests with the changes be easily surfaced.
‘’’suggestion <code block> ‘’’
https://web.archive.org/web/20220831234107/https://github.co...
I predicted this the moment I saw the React dev tools icon going blue when browsing GitHub. My comment (which I can’t find right now, I’m on my phone) was along the lines of them going the “Reddit way”. A totally worse experience for the end user just for the happiness of the React fanboys working there.
I already can’t stand the code browsing UI, which randomly closes or open a sidebar as you navigate back, or the search input which doesn’t even look good to me. What a total shame they’re messing it up so badly. GitHub had one of the best UIs in my opinion, and they’re just messing it up for the sake of keeping some devs happy.
Apparently this was very common feedback and I know that at least five other people who were maintaining large scale open source at the time gave it that week too. It’s never gone anywhere though, and as a result I have to disable all workflows except “rebase and merge” for every repo…
We have a working copy, index, commits, branches, remotes, pull requests - all of these come into play when proposing even the simplest change to an open source repo today. The idea that adding yet another concept to the pile will make things better is something I can't agree with.
Will it enable more capabilities? Yes. Adding features generally does that.
Aside from the "it's already complex enough" argument, there's also the fact that 90% of changes I've seen in my daily use of git don't require this feature. This means the feature will be misunderstood, misused and often not used when actually needed.
I'm doing a lot of review work and changesets are a killer feature not to lost in comments. Even for small-ish 50 line reviews.
Changesets especially powerful with local stack based development tools like stgit which allows to completely remove branch management.
It was great for skilled users to navigate with the keyboard and easy to see when everything was resolved. But if used as intended, like fixing some commented chunks while debating others, the information displayed became unwieldy. GitHub PR reviews are simpler and that isn't a bad thing.
Edit: looks like it's still available and hasn't changed massively, I'm not surprised as probably a lot of licensees cancelled when GitHub added reviews (my company did). Check it out if the article speaks to you.
I work on a tool that includes a UI for diffing versions of a GitHub PR, but you can totally get the same thing via messing with GitHub.com URLs.
https://graphite.dev/docs/pull-request-versions
If people are interested, probably a thing we could do a technical blog post on.
However, we add tags to commits we're referencing, just in case someone at GitHub gets around to implementing garbage collection!
There's a free service (GerritHub) that lets you use Gerrit for your public GitHub project. The learning curve for it is pretty steep if you've never used Gerrit before, though, and it does have some reliability issues. But for a certain kind of project it can really transform the code review experience.
Yep, I'm going to do that!
> Work-in-progress commits towards addressing review feedback become visible as soon as the branch is pushed. This forces contributors to address all feedback in a single commit, or for reviewers to deal with partially-addressed feedback.
This point isn't really valid. It assumes that the contributor wants to push their new work addressing feedback to the remote as soon as they make each commit. That's OK, so do I (in case my laptop disappears or whatever). But, it takes 1 second to make a git branch, so you can just push your new commits there and then merge or cherry-pick onto the PR branch when you're ready.
To review I like branches. Bad commits are not pleasant to look at, but they carry information, too, e.g., revealing the need for discussion. It's a bit like asynchronous pairing.
I don't think looking at the code on a website is sufficient as review in many cases, so I typically have a copy of the branch anyway. If any feature is needed, it is the possibiliy to comment on unchanged lines.
It probably depends on the scope. If the reviewer works on a more abstract level and can rely on authors to get the details right, maybe github is not ideal. Where the reviewer is almost as deeply involved as the author, a branch works nicely.
But I don't understand how a contributor can feel pressure to address all of the reviewers' comments in a single commit: They can commit as many times as they want, and only push when they feel it's ready. And in the case where they may want to offer multiple different solutions for a reviewer to choose from, I like a small adaptation of a suggestion I saw by another commenter here, which is to make a PR for each option off their existing PR's branch: When one of those option branches is accepted and merged, GitHub will fast-forward the original PR's branch to include those commits, which is exactly what you would want and expect.
It is possible to comment on commits in github (you can do it by clicking on the sha1 of the commit and then making a comment on a line in the diff). But the comment won't show up in the main PR diff window.
> But I don't understand how a contributor can feel pressure to address all of the reviewers' comments in a single commit: They can commit as many times as they want, and only push when they feel it's ready.
Unfortunately, this leads to a lot of fixup commits in the branch that muddle up the history. A changeset consists of one or more commits where each makes one logical change where the what was done and why it was done that way are detailed in the commit message.
I don't yet see how that is different from just... a sequence of commits, which you can do now. (If you want to claim that you could quickly make a bunch of messy local commits and afterwards reorganise them into a more logical group of commits in a changeset -- you can already do that, without any new concept of changesets, by using `git rebase -i`.)
I’m anticipating some push back on this, because I didn’t notice it mentioned anywhere else, even though there are a fair number of comments. So, I may need take some time to understand these other tools. But, short of that, for me personally, keeping things small and relatively easy to understand is the only way to maintain my sanity.
"This changeset still has the problem I drew attention to in my review of the previous changeset. Please see the comment there."
? I'm just curious how that works; I haven't used changesets much but it seems like this would be one inconvenient aspect.
Reviewable does this with an algorithm that ensures the code context is similar, with a visual warning if the comment might be in the wrong place. (Shameless plug!)
Of course, everyone (including myself!) is probably biased to think that whatever they are used to is simplest, tbh.
It wasn't initially designed with code review in mind (unlike systems like phabricator, gerrit, review board, etc).
Data isn't well managed. Isn't version controlled well. The pull request is just another type of data.
We can keep improving each applications model. But some day, imo, the general project of computing needs to take data more seriously & develop general tools for managing data over time well & consistently across apps. PRs would just be one example of something that would be better tracked.
All supported by Reviewable, of course (sorry for the shameless plug!)
The main drawback is that contributors have to learn a proper mail user agent (Gmail is notoriously bad with patches).
* CodeApprove (codeapprove.com)
* Graphite (graphite.dev)
* Reviewable (reviewable.io)
* Axolo (axolo.co)
* Viezly (viezly.com)
* Mergeboard (mergeboard.com)
* Codestream (codestream.com)
* Pullpo (pullpo.io)
* ReviewPad (reviewpad.com)
* Planar (useplanar.com)
* Visibly (visibly.dev)
* Codelantis (codelantis.com)
I've also considered not allowing contributors to force push. Instead any changes would be pushed (possibly as fixup! or squash! commits) so all history of the merge request is easily accessible. To be rebased/squashed later, of course.
I have created a tool called Git Patch Stack, https://git-ps.sh which makes it easier to manage a stack of patches, request review of them, re-request review of them, and many other features.
Checkout the the site and the documentation as it explains a lot. But if you have any questions feel free to join our Slack group and ask away.
I have never tried those apps that build a top GitHub. I remember Facebook's sapling has a web client as well that does stack reviews.
> They're already a well-explored user experience problem in existing products like Gerrit and Phabricator.
It's possible that better tooling would help there, of course.
(A surprisingly common pain-point with Gerrit is when you've wound up with a semi-long-lasting stack of patches for some reason, and then you develop a branching tree of sub-patches and need to rebase them all when you make some change higher up. The answer of "don't let a stack last long enough that you need to do that" has an appeal, of course.)
They introduced batched review comments well over five years ago, and still haven’t fixed the obvious issues with it, like how if you’re the PR author and you’re replying to someone else’s comment, and hit Cmd+Enter, it starts a review of your own PR (requiring you to delete the comment and start over, fun!) rather than just replying. Glaring day-1 oversights of what should have been a simple feature, never fixed.
I think the main issue is that everyone has their happy paths through review, but those aren’t everyone’s happy paths. It’s why I like that having a variety of tools is possible. GitHub likely has to bias towards the beige middle ground that suits everyone well enough.
Here is a random series from DRM patchwork: https://patchwork.kernel.org/project/dri-devel/list/?series=...
Would you squash that? Hell no, of course not. You would be mixing atomic changes in different subsystems. It breaks bisect and makes blames a mess.
Honestly, squash merges are frequently a sign of people who lack mastery in Git itself or make no effort to produce high-quality independent commits.
In Github at least you can set the behavior to take the PR description by default as the squashed commit message. In fairness this is not the default. The default behavior for squash merges is to ask for a new commit message right as you hit the merge button, and the default is all of the messages from the commits being squashed together.
> make no effort to produce high-quality independent commits
I'm partial to sqaush merges when using github. I don't put much effort into the individual commit messages, instead I put lots of effort into the PR description (the thing reviewers will read, and what will eventually become the commit message in revision history). That said, one of my favorite features from gerrit at a past job was that the commit message itself could be reviewed.
So literally any open source project that accepts external contributions. For this reason we default to squash merge but will allow for exceptions if people ask for it and know how to structure their commits.
Otherwise known as "colleagues" :)
As soon as you start squashing commits, you throw away a very useful guarantee git gives you by default, which is that a given code change A is "in" a given branch B if and only if A is reachable from B. This guarantee is very helpful when debugging differences between versions, e.g., in figuring out which commits I've made locally are actually running in a shared environment like staging/UAT.
The short term and long term view of the work should be different.
Relatedly, does GitHub merge understand !squash and !fixup commands? I kind of gave up on those and just accepted a few trailing bugfix commits on PRs to not mess up peoples reviews (some projects also invalidate review on force push).
Having to bolt extra features on top of GitHub to make it work properly is a shortcoming of GitHub, it shouldn't be an opportunity to build more tooling the customer has to pay for on top of it. Granted, I can see that conversation would get us nowhere given your income relies on selling people features GitHub is languishing on - you have an obvious interest in keeping that feature shitty.
[0]: Phabricator was disabled in 2022, Phorge is the new fork.
I understand this linke of thinking might suit you but I fear it is not as convincing as it sounds to you. At least it's not to me.
I think therefore it's pretty much inevitable that if you need a more advanced code review tool you'll end up picking a third party one. Though admittedly, as the founder of Reviewable, that thinking does rather suit me too. ("It is difficult to get a man to understand something when his salary depends on his not understanding it" and all that. :D )
(OK, Saab did actually do that once but they're weird)
GitHub is fairly feature rich imo, _especially_ when compared to something like CodeCommit on AWS. I’ve been forced to use CodeCommit on client engagements and it’s absolutely horrid. Honestly if your tool supported CodeCommit I’d say the value proposition would skyrocket.
Also, there isn't only paying customers having this issue but large OS too.
Answer, just like with the non-changeset oriented workflow, it depends. If the new PR update deletes the associated lines of code, there is arguably a need for those types of comments to be manually added with the current interface, but not so with a changeset oriented interface.
I like the append only nature of a changeset oriented interface.
But I find this type of conversation quite fiddly to do in pure text without reference to examples and the actual use of both styles, so forgive me if this is still unclear.
The difference is a sequence of commits that implement a feature where each commit basically is a logical change that does a single thing (e.g., add a function and associated test, add calls to a new function, etc) versus a sequence of commits with additional commits that fix issues brought up during review. Those additional commits really should be amended to the corresponding original commit rather than being a completely separate commit.
Another way to look at it is submitting an assignment where you have one page per problem solution as opposed to submitting that and then several more pages at the end containing fixes to those problem solutions rather than incorporating those fixes in the original set of pages.
Not that this has any merit, as it could be seen from far away by anyone with > 5 years experience in this field.
And this just will just get worse, specially for end users. Let's see where we're in 10 months from now.
Reviewable lets you review your commit messages just like any other file, BTW! (Disclosure: I'm the founder.)
If you’re saying it’s better to batch together multiple replies to multiple people’s comments at once, fine, but that’s not a “review”. Why should you have to “approve” your own code (what does that even mean?) to do this?
Those should also invalidate on a "normal" push if any changes are made. But yeah, it'd be nice to have the option to keep reviews valid on force-push if diff is identic and the only difference is rearranging commits/messages while still invalidating on any actual changes.
It's not just a feature, the model is fundamentally different and much more conducive to delivery, feedback, and happiness.
I was at one point considering writing a tool which would checkout an MR, then let you just edit it as per normal, and then would submit the whole thing back to a Gitlab MR as a set of proposed changes. The point was to ensure that you could easily expand the MR beyond the diff of changed lines, which was frequently inadequate to review a patch properly since it omitted context.
If i must pay for GitHub and an external tool, isn't GitHub just an dumb overpriced git storage.
Not to mention that you get an easy way to spin up your CI/CD workflows in GH Actions (which of course definitely has its own problems, which there was another popular HN post about recently). There's a reason why it's the default for new companies -- if there was something much better, it wouldn't have the market share it does I think. Familiarity coming from OSS is also important.
The problem with squash commits is that they tend to make changes to more files and make changes to more lines in a given file. This makes it harder to revert changes after more changes have been applied to the main branch. In contrast, organizing commits into a set of logical changes necessary to implement a feature in a given branch makes it much easier to revert one of those commits even after other feature branches have been merbed into the main branch because the commits affect one or just a few files and not many changes in a file).
Really it just requires discipline to either make entire related changes as patches or pull requests, which doesn’t matter.
The result of a pull request should be the equivalent of having built a single patch to start with, just with better review tooling.
You shouldn't have to revert a lot of tiny commits if the bug is due to just one of those commits.
> The result of a pull request should be the equivalent of having built a single patch to start with, just with better review tooling.
In my experience, features take more than one commit to implement. The merge commit provides a way to group those multiple commits so that you can see all commits that went into implementing a feature.
If you're trying to make pull requests that are like small commits, then a feature branch becomes
first-commit
first-pr-merge
second-commit
second-pr-merge
third-commit
third-pr-merge
...
nth-commit
nth-pr-merge
Which basically introduces a lot of merge commits (effectively doubling the number of commits) where the first parent is the previous merge commit and the second parent is the single commit for that pull request. You have no way to really group related commits that were used to implement a feature since there's no merge commit that groups them all together. In that case, you could halve the number of commits by dispensing with merge commits entirely and just adding the #PR-number in the commit message to link to the PR discussion.
Note that you can bisect --first-parent if you want the "just find the topic branch that introduced this" behavior, without taking away the ability to find the actual commit!
You want small commits in main, so send small PRs. If you have a feature, you can do a feature branch and merge into it the small commits then merge (don’t squash) feature branch into main.
It just blows my mind as someone with a somewhat perfectionist approach to rebasing (to the extent I sometimes intentionally break up or reorder commits to make the changes more intentional or natural, even when it causes nasty local merge conflicts) :P
The UI doesn't know what to do with it, and most developers don't really understand it without spending a bunch of time explaining it.
When it works, it works great but the overhead is ridiculous since you get nothing inline on the UI: ideally something like the commits should pop up as comments, with accept/reject/discuss options.
git am < saved_email_file.eml |git am
Most people are using gmail, but when it comes to local clients, Outlook, Apple Mail and Thunderbird are by far the most common and as far as I know none of them support this without extra steps.