I'm pretty sure this approach is somewhat similar to the mailing list git approach, where patches usually get submitted once ready and then changed depending on feedback as a wholly new submitted patch (as part of a broader conversation).
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).
> 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.
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.
> 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)
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).
No, it is not a misconception. How do I apply a patch from gmail to a specific git repo on my computer? (I would genuinely like to know the answer, but it must not involve mutt, gnus, or dovecot!)
Copy-paste may not work depending on how the email client renders the email. For example, if the email client doesn't preserve prefixed whitespace, then the resulting patch may be corrupted or not apply. Saving the actual email to a file on disk would avoid that issue.
OK thanks, that sounds fairly convenient for small patches. And I expect you'd say download the email for larger patches (as a sibling commenter suggests).
In the worst case you can copy paste a patch into a new local file in the repo and then apply it with git from there. I’ve had somebody slack me patches before and it is not a big lift.
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.
I am slowly convinced that comments on PRs are like comments on blog posts or youtube videos. Ephemeral, irrelevant and ineffective. If you really want to "reply", put up your own blog post or a "reacts" video. Same for code. unless it's simple typo fixes or improvements, deeper fixes come from writing code samples yourself.
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
One practice I’ve gotten into with juniors is checking out their PR, branching it, PR-ing to their PR, and then having the discussion about what I want them to change there. This is good because it lets us isolate certain issues (and truly resolve them) rather than comments getting pulverized by lots of little commits, only to have someone else come in and say “LGTM!” and merge it with unresolved issues.
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.
I like this approach too, but almost no one understands it (i.e. the idea it's possible is foreign) and UI support for it is non-existent in all the major products.
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.
I might be wrong but it sounds like you don't know about the GitHub PR feature where you select the lines, and GitHub allows you to edit them, and posts a diff when you're done, and if the author accepts it out becomes a jointly authored commit.
In situations where I'm doing PR reviews seriously in a multi-developer environment I'm usually on Gitlab. Gitlab definitely has some features for this sort of thing, but I've never been able to use them seamlessly or quickly.
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.
One way to do that is to open a second PR which targets the branch of the first PR. I think that's also probably the most natural way -- would you agree? I don't see it done much, but I've done it once or twice and I'd embrace it if it were more culturally standard.
That's what the parent comment is talking about though: and I agree it's better, but it just pops as another global PR.
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.
I left GitHub earlier this year after a decade. I’ve seen mockups, hack week projects and proof of concepts of this for the last 5 years (at least). A lot of engineers there knew this is the future that PRs need but GitHub at this point seems organisationally incapable of delivering these sorts of large improvements (Microsoft is perhaps partly but definitely not wholly to blame for this). Instead, they are midway through porting Rails views to use React, keeping most pages looking identical while introducing bugs and regressing previous usability improvements on a weekly basis. A real shame.
> Instead, they are midway through porting Rails views to use React, keeping most pages looking identical while introducing bugs and regressing previous usability improvements on a weekly basis. A real shame
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.
This would explain something else too, probably: a few years ago I did a call with some GH folks talking about the idea of making the commit message applied to a squash merge part of the review itself.
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…
Changesets seem like a UX nightmare. While I understand the motivation, the complexity of version control today is mind boggling.
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.
Changesets in Gerrit are more easier to manage from processes and UX standpoint. There is no branch/commit/comment/rebase/commit/resolve-staled-comment/fight-for-true-pr-merging-strategy dance.
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.
I agree as I used a tool Reviewable which is accurate about which version of the change is being commented on, which files you reviewed etc. It even supported rebases. And no comment was finally marked resolved until the original author marked it as such.
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.
GitHub actually stores all the data necessary to do this without changing your ref model as the article suggests — even if you force push, it never garbage collects the commits that the branch used to point to.
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.
The changeset workflow is what Gerrit uses. Gerrit is great.
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.
Reviewing code on GitHub is tough, especially with larger changes. However, Gerrit's user interface is so beginner/user hostile that I would still prefer it to Gerrit. It scares away new contributors. :(
> I'm sure I'm wrong about some detail about some of the points above. Someone is likely to say "he could've just done this to solve problem 5(a)"
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.
If an author is unable to produce readable commits on the branch of a PR the author is likely not able to produce readable change-sets either.
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.
It's surprising to me that GitHub currently doesn't attach review comments to specific commits, but only to timestamps, and I agree that it would be great to improve that.
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's surprising to me that GitHub currently doesn't attach review comments to specific commits
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.
>A changeset consists of one or more commits where each makes one logical change
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 don't yet see how that is different from just... a sequence of commits
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.
I don’t have experience with the outlined workflow, only with GitHub PR’s, but it feels like maybe the PR’s could too big if you have this problem?
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.
Fracturing a big commit to smaller parts doesn't help. Amount of work is the same, number of comments would be higher or same due to bigger context loss. GH PR UI is notoriously confusing while reviewing multi commit PRs.
If your tool moves comments forward to newer changesets (ideally with some sort of code ensuring it's in the right place) then that's done automatically for you.
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!)
Usually you can quickly click through the different changesets and see all the old comments. In my experience, it maybe needs one more click to follow the reference compared to similar comments referencing the current changeset.
Github can show a list of unresolved comments, and with a single click jump to the location in the state of the specific commit the comment was written at. No need to cycle though commits / state at each commit without comments.
I see, I focused on how to deal with such a "see previous changeset" comment, when the question was really about whether there is a need to make such comments.
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.
I was lucky enough to work at a company with a great code review tool at one of my first positions -- and I am nowhere near convinced that GH's review interface is the simplest possible.
Of course, everyone (including myself!) is probably biased to think that whatever they are used to is simplest, tbh.
I agree, you want a basic tool for most users that just need the ability to review the code. But, once your team is doing something where the code quality is important, you'll want to switch to a better tool for the job (like Reviewable!), just like when you switch from a whisk and bowl to a kitchenaid standing mixer when you start baking often
I agree. As a way to minimise the pain on GitHub today, we disallow force pushing and enforce squash merging. Force pushing is a nightmarish behaviour, once a Pull Request is opened the branch must be append only.
So you have endless "Fix a" "Typo" "fixup" "revert redo" "add y missed in z" commits and then the squash pushes all that crap into the commit message for whatever the final mess will be?
A Pull Request should represent one change to the system (whether that’s a new feature or a bug fix). A commit to the Pull Request branch only has value in the context of the Pull Request. If you want a Pull Request to contain multiple atomic changes then sure, you need to do what you’re describing… but that’s entirely optional (and straying even further from the idea of changesets).
> So you have endless "Fix a" "Typo" "fixup" "revert redo" "add y missed in z" commits and then the squash pushes all that crap into the commit message for whatever the final mess will be?
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.
> 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.
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.
> 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.
I don't understand the appeal of squash commits. AFAICT, the only advantage of squash commits is a nice "linear history", but you can get that from regular old merge commits just by adding --first-parent to your `git log` command -- and you still have that additional detail available on the second-parent commit chain if you need it.
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 point is it enables bisect workflows that don’t suck. If half the commits in a tree are “fix misc derp” or “test passes now”, that workflow isn’t possible.
The short term and long term view of the work should be different.
> The point is [the squash commit method] enables bisect workflows that don’t suck. If half the commits in a tree are “fix misc derp” or “test passes now”, that workflow isn’t possible.
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).
> Indeed, but trying to revert lots of tiny commits in one unit is no better.
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
with commits and their associated merge commits for other features interspersed with your set of commits.
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.
Because squash merging results in horrifically poor commit messages that are use to no one in future, because GitHub can’t be bothered to implement anything beyond a simple textarea for editing them.
Not only that, they also result in commits that change a lot of files and make many changes in each file. Try to revert one of those commits is difficult after other changes have been merged into the main branch.
Force pushes makes reviews hard to follow, especially if there are multiple rounds. GitLab handles it much better.
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).
> some projects also invalidate review on force push
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.
I’m assuming they mean squash merging into main once the PR is accepted. That way you have 1 commit on main that links back to 1 PR with N commits on it. Which is easier to follow on main branch rather than a million commits per PR
That makes "git bisect" and "git blame" fairly useless. Please don't do that. Better to keep "oops" commits than squashing a series of logical changes into one monster commit.
"oops" commits are bad for this too (harder to find the regression when there's unrelated broken states), but you're right that it's less bad.
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!
I don’t think there is a one size fits all answer here. “Oops” commits make blame useless too plus they make following any meaningful change very hard.
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.
I see now that the grandparent was enforcing squash commits, did not mean to single out your comment.
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
Well, I’d appreciate it if I collaborate on a repo with you. I end up giving up commit hygiene due to the absolute lack of interest from folks I work with unfortunately :)
I’ve been contemplating pulling the trigger on this one myself. I’m pretty much sold on it. I’ve been on board with squash merging for years. Best thing I’ve ever done for our project. I eventually came to the realisation that the majority of people who were against it were so because some purist greybeard had beat it into them.
Generally, it feels like a bit of a farce that source code is very well well version controlled, but nothing else is.
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.
It's the email flow that's existed before gerrit and phabricator. Both of which implemented because it makes sense. Then github came along and here we are.
I often stack PRs to emulate the practice described by Mitchell but it's not ideal as if you need to change an underlying PR l, you need to rebase all of the dependent PRs.
There are tools that solve this problem! I work on one (Graphite), but there's also plenty of others like git-branchless and Sapling. All three of these are inspired by Facebook's internal fork of Mercurial (with Phabricator/"Diffs" for reviewing) -- Google has a similar model with Piper/Critique CLs, with Gerrit as the open source result.
So Facebook is still using Phabricator? Somehow I suspected that they had abandoned it since they seemingly stopped being involved in the open source Phabricator project.
There's a ton of cli tools to automate maintaining a stack of PR's, with new ones coming out all the time. I've seen 'spr' for a long time, and recently there's graphite's and even aviator came out with 'av'.
All supported by Reviewable, of course (sorry for the shameless plug!)
Mailing lists are a bit arcane but I agree nonetheless. Learning how to use them is worth it. The changesets proposed by TFA are essentially what the Linux kernel has been doing with email for a long time.
I think in the end we should not expect GitHub to provide the best option here. We should expect them to provide a basic option (which they do) and for sophisticated consumers to pay more for a much better option. Everyone should be shopping for code review tools!
I disagree on this; GitHub should be building the best option here if they have the resources to do so. The fact that the review interface is as basic as it and so prone to accidentally marking the wrong comments as outdated is a major issue (one that other software like Phorge[0] already shows is possible if we're sticking to the realm of "non-mailing client reliant git servers").
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 think in the end we should not expect GitHub to provide the best option here. We should expect them to provide a basic option (which they do) and for sophisticated consumers to pay more for a much better option. Everyone should be shopping for code review tools!
"
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.
Here's how I like to think about it: GitHub is a generalist. They have a big platform with lots of features besides code review, so even though they also have lots of employees they won't be able to focus on code review as much as a dedicated company could. They also have a huge number of users to please so they can't afford to rock the boat too much or make the learning curve too steep.
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 )
How you do PRs is definitely important, and should be part of a company's consideration here, but remember that PRs are already a layer of abstraction over the SCM. Self-hosting Git is definitely not as easy as setting up a GH account.
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.
Most frequently, I think it's because you want a single platform to store all company code, but not all teams agree on using GitHub PRs vs a more advanced code review tool. Other potential reasons include having access to the other GitHub features: issues, actions, security stuff, the merge queue, etc. You could pull all these together from less-overpriced more-specialized alternatives, of course, but sometimes it's nice to have a single integrated platform even if you decide to replace one of its features.
GitHub is a platform with dozens of tools, I think of it as a basic toolbox. It's great, but if you're hammering nails all day, you should invest in a nailgun. Doesn't mean you don't use the hammer or the wrench in your toolbox, but when you care about one task a lot, you invest in the tool to do that task better
Question: does CodeApprove place related files in closer proximity during review? I would _love_ to have a class and its test next to each other instead of sorted alphabetically. I'm tired of jumping around all over the place, trying to traverse through my review thought process.
I can't speak for CodeApprove, but Reviewable has file grouping capabilities so users can groups files based on anything they want (using a javascript function to do it)
CodeApprove does not have that feature because we don't currently do any smart code parsing to understand what would make a file "related". I think Viezly (https://viezly.com/) does the best job at that.
That's like saying it's OK to expect (say) Ford to make a car with no steering wheel. GitHub basically define the baseline for the entire industry and have millions upon millions to blow on trying to do better.
(OK, Saab did actually do that once but they're weird)
The tail is not as painful as real engineering but as an industry we throw away billions in value every year to dumb processes. There is more to this than just the scar tissue.
It's not just a feature, the model is fundamentally different and much more conducive to delivery, feedback, and happiness.
> We should expect them to provide a basic option (which they do) and for sophisticated consumers to pay more for a much better option
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.
Large OSS feels like a different use case than the big company one, to be honest -- I think that what maintainers care about is vastly different than what companies do. GitHub feels optimized for the former (OSS) as it is today.
A lot of people mentioning Gerrit, but Gitlab seems to support this too. It also removes approvals for the new changeset in case any were already present (by default, you can now disable this). It seems to have supported changesets forever so I'm surprised GitHub doesn't.
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 am personally a big fan of a patch stack style workflow.
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 used to work at FB, and unfortunately the Sapling review client that shows GH PRs has nothing on the the actual internal code review tooling at Facebook (now called Diffs, formerly Phabricator). I miss that tool so much
I work with Gerrit in my job, and find a stack of patches to be a useful way to deal with things... but I've also seen that it definitely has a learning curve for people who're not used to it. There's something to be said for the GitHub pull-request "just smush together all the commits on this branch" model in terms of ease of understanding.
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.)
Better tooling is definitely the answer here -- I used to work at Facebook where rebasing dependent patches in Mercurial when you needed to adjust something felt like a first-classed flow, and I'm currently working on a tool that does the same thing, but on top of Git and GitHub.
At my previous job the cofounder introduced gerrit; I loved the model but it became immediately apparent that it was too complex for our team and we abandoned it after I spent a ton of time doing tech support for teammates.
GitHub seems to have no interest whatsoever in making the process of reviewing code any better or easier. It’s still essentially the same as it was 15 years ago.
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 review my own PRs regularly. It’s how I talk through all my changes with meaningful inline comment threads. I prefer the behaviour you’re commenting about because I want to do those things in a “review” rather than pollute inboxes with one-off comments.
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.
I’m talking about replies to other people’s comments. If someone asks you a question in a review, it seems crazy to me that you should submit your own review of your own code, complete with an approval (huh?) just to answer it.
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?
If you reply to someone else's comment from the discussion tab, it does not start a review of your own PR. In the code tab, I have seen an option to start a review or just make a one off comment. I generally prefer to make them as a review, because I am typically replying to comments in a batch.
This can go either way, and it's ultimately a social problem, not a technical one.
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.
Typical PR flow on my team for PRs to dev branch is reviewer approves with comments, then submitter addresses comments with new commits and merges (unless they judge the new commits are significant changes). This makes it lighter weight to offer comments since it doesn't mean another delay for a rereview. The reviewer gets emailed about any new commits that come in, so all changes do get seen.
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've noticed listed problems, they surely do exist. but stepping back a little - proposed solution sounds like: "lets make a brand new version control system for already existing version control system".
Not really. There are many mature tools outside of the GitHub monoculture that do changesets, both on top of Git as well as other VCSs.
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.
I’m in 100% agreement. It’s difficult handling reviews of juniors in the current model, as you have to address the fir usage first and how to avoid these type of issues ITA BeFORE getting to the review at hand.
I understand that you have to sell, but could you please do it somewhere else or take some decency and not to have your team to plug on every other comment here? Thank you!
We're actually a small company and only 2 of us posted here, most of the other plugs of us are by users. piotrkaminski is the only other user here from the company
I haven't used GitHub in a while but absolutely all of the author's issues are fixed in gitlab and I have a hard time believing GitHub is that far behind.
Jm2c, but by far the best place I've ever worked (my current client) we don't do code reviews at all unless the author wants feedback.
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.
I don't see code review as an overwatch but a good communication tool and a way to think about problems collectively. Code review reduce bugs because it permit to have people think about the problem from multiple angles. About decisions, I think important design decisions need to be taken prior to the code review steps ,and also reviewed.
I'm not sure what context you worked in that gave you that opinion about code review being a sign of distrust and I think the problem lay within the culture in those places not the code review practice it self.
I don’t mind code reviews so I’m not really defending who you’re replying to. There is another way to think about this tho and I actually have found this practice to be light years more effective than code reviews. Where I work we have a practice where before you start doing a ton of work on your card/ticket you should find someone on your team, explain the problem to them, and show them how you’re going to do the work. This gives your teammates an opportunity to give you feedback before anyone has done anything, and you can both poke holes in design decisions before anyone has done anything they feel strongly about. If you do this process 95% of code reviews are pretty much worthless.
I fully agree, we also do this and this is the design review I alluded to.
That said, I think both have different purpose. The design review is more about the approach and the high-level implementation. The code review is about the implementation details.
Also, a great aspect often overlooked around code review is that it's a great teaching tool if used right. Asking question go a long way and make both the reviewer and the reviewee learn.
Code review is often looked to as a senior watching over what juniors are doing. In my experience it should be done for all levels and even juniors should review seniors PR as they can bring novel perspective and at least learn from the code by reviewing it.
I'm gonna rephrase my previous point. What you say makes sense, but only if you understand that it takes away time, money and energies and after knowing it, and the impact of all this time drag you determine it's worth it.
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.
If we want to include the cost of a code review into the equation we should also include the cost of fixing a big that made it to production which is in most of the cases higher then code review. Skill is not a factor here, I work and worked with some of the best engineers in the world and everyone write buggy code sometimes. Software engineering is a complex practice.
If you are writing some prototyping code or some code that will never make it to production or in a very early stage of a startup, sure I can understand your point.
I respect the thoughtfulness with which you made your point, but completely disagree. As another commenter stated - Code Reviews are, for me, primarily a method for communication and knowledge dissemination, secondarily a means to get a second pair of eyes on my work to ensure it's legible and that I didn't miss anything, and only as a distant third a means to "protect" the codebase.
Not to mention the value of a diff/pull request/changelist/patch/merge request (so many names...) and its corresponding discussion as a historical artifact for your team to reference later on!
I trust and respect the people I work with. I also prefer to have code reviews, even (especially) of my own code. Code reviews can point out code that
- 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.
None of the things you said matters if your issue is time to market or you have little business.
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.
My goal is to build things that work, and can continue to be worked on and improved over time; where development doesn't grind to a halt in 6 months because nobody cares about quality.
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.
I have been in the industry for a fairly long time and I have seen code review tools pop up, and GitHub came fairly late to the table (IIRC, I was using a code review tool in 2007 against Perforce; GitHub wasn’t generally available until 2008 and I think I joined in late 2008 or early 2009).
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.
Same experience here. Productivity unlocks like crazy, and amazingly the sky doesn't fall. It does require stringent hiring practices though and a flat, open culture. Oftentimes I don't need a formal syntactical process like a review but rather just need to bounce ideas off folks to get a good plan for a larger feature.
Agree. The better your team members, the less need there is for code review.
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).
I truly believe that Reviewable is the best way to review code on GitHub, that's why I left a Head of Engineering post to come work here.
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
I just don't understand why it has to be remotely hosted (SaaS), as opposed to run locally. I hope this is reconsidered one day. I'm happy to pay with money, not with data, or having to rely on Reviewable for availability.
What's the pricing like for individuals and what payment options are you open for? I just saw "Enterprise - contact us" and assuming you were not interested in private individuals for this.
I'd be happy to chat, though we'd likely come up with something unique to individuals so we're not wasting each other's time every year. Reach out to support@reviewable.io and mention this thread
Corporate devs are still in perforce paired with some web review tools. It is a big cultural problem for a lots of devs out there that they are not familiar with open source flows and tools.
I love the amazing, advanced submission and review workflow you can see if you click anything with PATCH in it at https://public-inbox.org/git/. The best part is that it's fully integrated with Git!
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.)
I agree that this is how development should work. But let's be fair: The claim that this "is fully integrated with Git" is at least misleading.
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.
All I see is normal code review comments plus diffs, but as an email thread instead of a UI that lets comments be attached to the code they're discussing. And it's hard to read because no markdown.
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).