15 ms·

Reorient GitHub pull requests around changesets

224 points by jamesog 3y ago

m3drano 3y ago
IIUC, he's referring to a workflow that Gerrit implements 1-to-1, isn't he?
- da39a3ee 3y ago
  Yes, he says so two and a half times.
- imran-iq 3y ago
  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.
  - tomasreimers 3y ago
    This. Every time people talk about how pull requests /should/ work, I point to them that's largely how they did work...
setheron 3y ago
Surprised he didn't refer to stack PR based systems like Gerrit for reference. 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.
- eyelidlessness 3y ago
  There are two mentions of Gerrit in the article. Granted neither goes into detail, but both reference it as prior art on the topic.
  - setheron 3y ago
    Oops. I must have missed the mentions.
- adm_ 3y ago
  I believe sapling uses https://github.com/ezyang/ghstack https://github.com/ezyang/ghstack under the hood for stacked PR.
- bittermandel 3y ago
  Doesn't he do just that? > They're already a well-explored user experience problem in existing products like Gerrit and Phabricator.
- jacobegold 3y ago
  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
da39a3ee 3y ago
When using changeset-based review, do you find yourself writing things like "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.
- dundarious 3y ago
  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.
  - almostnormal 3y ago
    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.
    - dundarious 3y ago
      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.
- fahhem 3y ago
  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!)
da39a3ee 3y ago
> 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.
- nmadden 3y ago
  I did wonder about that when I read it. The idea of changesets and versions sounds an awful lot like branches and commits.
- almostnormal 3y ago
  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.
- scubbo 3y ago
  Yeah, that one baffled me, too. The rest makes sense, though!
oldtownroad 3y ago
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.
- tmpX7dMeXU 3y ago
  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.
  - iimblack 3y ago
    Majority of people aren’t making atomic commits. In the off chance they are it’s not hard to switch from squash for those one-offs.
- huijzer 3y ago
  Oh man yes I dislike non-squash merges very much because they make it so hard to go from the Git blame to the pull request discussion
- Aeolun 3y ago
  I really don’t see the difference between force pushing and not when you are going to squash merge anyway.
  - eddythompson80 3y ago
    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
    - mbakke 3y ago
bhouston 3y ago
This is a great suggestion and likely not that hard for GitHub to implement. I am surprised this isn’t updated more than it is.
- ninkendo 3y ago
  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.
  - Waterluvian 3y ago
    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.
    - ninkendo 3y ago
      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?
  - halostatue 3y ago
    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.
nh2 3y ago
A proper review tool such as reviewable.io (which is on top of Github) addresses all points in his "problems" list.
- fahhem 3y ago
  Thanks, we heart you too :D
codeapprove 3y ago
I agree 1000%. I’m the creator of what I believe is a better review interface for GitHub (https://codeapprove.com https://codeapprove.com) but there are also many others: * 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 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!
- rat9988 3y ago
  "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.
  - piotrkaminski 3y ago
    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 )
    - allendoerfer 3y ago
      GitHub is Microsoft, which also does Excel, Azure, Windows, Teams and many other platforms, each bigger than a code review tool.
cerved 3y ago
More and more I'm starting to appreciate the email based PR
- mbakke 3y ago
  It's simple, scalable, and has none of the mentioned problems. The main drawback is that contributors have to learn a proper mail user agent (Gmail is notoriously bad with patches).
  - wiktor-k 3y ago
    I guess for sending parches using git tooling instead of an email client works best. At least according to https://git-send-email.io/ https://git-send-email.io/
- matheusmoreira 3y ago
  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.
lifeisstillgood 3y ago
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
- zerbinxx 3y ago
  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.
  - XorNot 3y ago
    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.
Aeolun 3y ago
Hmm, not saying Github is perfect, but I think there’s value in providing the simplest possible experience as the default.
- fahhem 3y ago
  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
- jacobegold 3y ago
  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.
  - u801e 3y ago
    > I am nowhere near convinced that GH's review interface is the simplest possible. It wasn't initially designed with code review in mind (unlike systems like phabricator, gerrit, review board, etc).
jauntywundrkind 3y ago
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.
- earthboundkid 3y ago
  I’ve said for a while that the problem with Git’s data model is that the branch information is not itself versioned. I want Git but for Git.
  - jacobegold 3y ago
    This is what Facebook did extremely well with Phabricator, which was then open sourced.
  - Noumenon72 3y ago
    Git reflog provides history and rollback, what else do you need?
    - earthboundkid 3y ago
      Oh boy an ephemeral log that doesn’t sync.
codazoda 3y ago
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.
- mvdtnz 3y ago
  You're absolutely right, the author's problem imply massive pull requests that I wouldn't accept in my workplace.
- bvrmn 3y ago
  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.
noirscape 3y ago
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).
- __david__ 3y ago
  > 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.
- u801e 3y ago
  > 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).
kemayo 3y ago
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.)
- jacobegold 3y ago
  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.
- llimllib 3y ago
  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.
ghthor 3y ago
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. Sign me up for change sets!
- fahhem 3y ago
  Then sign up for Reviewable, which supports this (we called them revisions) http://blog.reviewable.io/tracking-changes-in-a-code-review http://blog.reviewable.io/tracking-changes-in-a-code-review Sorry for the shameless plug!
  - vanous 3y ago
    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!
    - fahhem 3y ago
      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
prokopton 3y ago
It’s a slightly different methodology, but Git Patch Stack has worked well for us over the past year. The CLI is a huge help. https://git-ps.sh/ https://git-ps.sh/
juped 3y ago
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/ 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.)
- Noumenon72 3y ago
  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.
- atq2119 3y ago
  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.
  - juped 3y ago
    Patches aren't to specific revisions, that's the Github straitjacket talking. They're patches. 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.
epolanski 3y ago
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.
- neo2006 3y ago
  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.
  - duncan-donuts 3y ago
    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.
neo2006 3y ago
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.
- fahhem 3y ago
  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!)
- jacobegold 3y ago
  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.
  - 20after4 3y ago
    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.
fahhem 3y ago
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
- 3np 3y ago
  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.
  - fahhem 3y ago
    We have an on-prem edition that doesn't even phone home, so you don't have to rely on us for anything but updates.
    - 3np 3y ago
      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.
      - fahhem 3y ago
        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
jacobegold 3y ago
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.
- fahhem 3y ago
  I agree, we work on Reviewable, which uses those same blobs. However, we add tags to commits we're referencing, just in case someone at GitHub gets around to implementing garbage collection!
  - jacobegold 3y ago
    They must have refs on the internal git servers -- they need them for the "force pushed from X to Y" timeline events
    - fahhem 3y ago
      You're right, though I think that changed recently? I vaguely remember only the last force push showed up, but now I see more than one so :shrug:
    - bongobingo1 3y ago
      I've definitely clicked those links through to 404s.
- tomasreimers 3y ago
  Hi! I work with the commenter - for those wondering here's documentation on that feature (pulled off with pretty much only GH data): https://graphite.dev/docs/pull-request-versions https://graphite.dev/docs/pull-request-versions If people are interested, probably a thing we could do a technical blog post on.
Pxtl 3y ago
Azure DevOps has some of this, it has "pushes". You view the history of a PR per-push, so developers can commit early and often. 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.
facorreia 3y ago
It’s very weird that PR approvals remain “approved” after changes to the PR that was approved.
- wcedmisten 3y ago
  There's a setting to automatically revoke approvals as "stale" after new changes get pushed
- atq2119 3y ago
  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.
- Noumenon72 3y ago
  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.
akoboldfrying 3y ago
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.
- ezekg 3y ago
  I think this is what https://graphite.dev https://graphite.dev is trying to do.
- u801e 3y ago
  > 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.
  - akoboldfrying 3y ago
    >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`.)
parentheses 3y ago
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.
- bvrmn 3y ago
  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.
- hahn-kev 3y ago
  Don't forget forks lol
- mmcnl 3y ago
  I completely agree.
- lozenge 3y ago
  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.
cryptonector 3y ago
IOW, something like stackable PRs.
Vendan 3y ago
Github had this planned in their old roadmap... But then they deleted it... https://web.archive.org/web/20220831234107/https://github.com/github/roadmap/issues/211 https://web.archive.org/web/20220831234107/https://github.co...
- mikemcquaid 3y ago
  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.
  - als0 3y ago
    Wow, why would they spend so much energy rewriting Rails code into React?
    - rapnie 3y ago
      Maybe to go all-in on Blocks. https://blocks.githubnext.com https://blocks.githubnext.com
    - 0xblinq 3y ago
      Fashion and trends followed by new hires, most likely.
    - lozenge 3y ago
      Someone likely saw it as a good chance to "take over" more of the tech team headcount.
  - 0xblinq 3y ago
    > 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.
a-dub 3y ago
it used to be worse. if you pushed changes that removed lines that had review comments on them, the review comments would simply disappear. but yes, i liked perforce too.
- SleepyMyroslav 3y ago
  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.
Lapsa 3y ago
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".
- klabb3 3y ago
  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.
TazeTSchnitzel 3y ago
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.
- janosdebugs 3y ago
  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. :(
danjc 3y ago
It might be less correct from a taxonomy perspective but why not just a different branch for each change?
globular-toast 3y ago
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.
intellix 3y ago
isn't this what Gitlab have? Instead of v1, 2, 3 you're able to see them at the state of each git commit hash
SergeAx 3y ago
This sounds like what they have in GitLab merge request flow: https://docs.gitlab.com/ee/user/project/merge_requests/versions.html https://docs.gitlab.com/ee/user/project/merge_requests/versi...
mvdtnz 3y ago
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.
choeger 3y ago
That approach works fine in gerrit. I always preferred it.
ngrilly 3y ago
100% this. It is the biggest problem I have by far with GitHub. I really hope they can follow this advice and do something closer to Gerrit.
josemanuel 3y ago
Gerrit works really well. Github could adopt that tool..
mmcnl 3y ago
I don't think this is actually an improvement. It's more complicated and most people don't need this. Don't let perfect be the enemy of good.
cyphactor 3y ago
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 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.

224 points by jamesog 3y ago

m3drano 3y ago

IIUC, he's referring to a workflow that Gerrit implements 1-to-1, isn't he?

da39a3ee 3y ago
Yes, he says so two and a half times.
imran-iq 3y ago
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.
- tomasreimers 3y ago
  This. Every time people talk about how pull requests /should/ work, I point to them that's largely how they did work...

setheron 3y ago

Surprised he didn't refer to stack PR based systems like Gerrit for reference. 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.

eyelidlessness 3y ago
There are two mentions of Gerrit in the article. Granted neither goes into detail, but both reference it as prior art on the topic.
- setheron 3y ago
  Oops. I must have missed the mentions.
adm_ 3y ago
I believe sapling uses https://github.com/ezyang/ghstack https://github.com/ezyang/ghstack under the hood for stacked PR.
bittermandel 3y ago
Doesn't he do just that? > They're already a well-explored user experience problem in existing products like Gerrit and Phabricator.
jacobegold 3y ago
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

da39a3ee 3y ago

When using changeset-based review, do you find yourself writing things like "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.

dundarious 3y ago
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.
- almostnormal 3y ago
  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.
  - dundarious 3y ago
    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.
fahhem 3y ago
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!)

da39a3ee 3y ago

> 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.

nmadden 3y ago
I did wonder about that when I read it. The idea of changesets and versions sounds an awful lot like branches and commits.
almostnormal 3y ago
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.
scubbo 3y ago
Yeah, that one baffled me, too. The rest makes sense, though!

oldtownroad 3y ago

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.

tmpX7dMeXU 3y ago
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.
- iimblack 3y ago
  Majority of people aren’t making atomic commits. In the off chance they are it’s not hard to switch from squash for those one-offs.
huijzer 3y ago
Oh man yes I dislike non-squash merges very much because they make it so hard to go from the Git blame to the pull request discussion
Aeolun 3y ago
I really don’t see the difference between force pushing and not when you are going to squash merge anyway.
- eddythompson80 3y ago
  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
  - mbakke 3y ago

bhouston 3y ago

This is a great suggestion and likely not that hard for GitHub to implement. I am surprised this isn’t updated more than it is.

ninkendo 3y ago
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.
- Waterluvian 3y ago
  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.
  - ninkendo 3y ago
    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?
- halostatue 3y ago
  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.

nh2 3y ago

A proper review tool such as reviewable.io (which is on top of Github) addresses all points in his "problems" list.

fahhem 3y ago
Thanks, we heart you too :D

codeapprove 3y ago

I agree 1000%. I’m the creator of what I believe is a better review interface for GitHub (https://codeapprove.com https://codeapprove.com) but there are also many others: * 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 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!

rat9988 3y ago
"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.
- piotrkaminski 3y ago
  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 )
  - allendoerfer 3y ago
    GitHub is Microsoft, which also does Excel, Azure, Windows, Teams and many other platforms, each bigger than a code review tool.

cerved 3y ago

More and more I'm starting to appreciate the email based PR

mbakke 3y ago
It's simple, scalable, and has none of the mentioned problems. The main drawback is that contributors have to learn a proper mail user agent (Gmail is notoriously bad with patches).
- wiktor-k 3y ago
  I guess for sending parches using git tooling instead of an email client works best. At least according to https://git-send-email.io/ https://git-send-email.io/
matheusmoreira 3y ago
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.

lifeisstillgood 3y ago

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

zerbinxx 3y ago
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.
- XorNot 3y ago
  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.

Aeolun 3y ago

Hmm, not saying Github is perfect, but I think there’s value in providing the simplest possible experience as the default.

fahhem 3y ago
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
jacobegold 3y ago
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.
- u801e 3y ago
  > I am nowhere near convinced that GH's review interface is the simplest possible. It wasn't initially designed with code review in mind (unlike systems like phabricator, gerrit, review board, etc).

jauntywundrkind 3y ago

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.

earthboundkid 3y ago
I’ve said for a while that the problem with Git’s data model is that the branch information is not itself versioned. I want Git but for Git.
- jacobegold 3y ago
  This is what Facebook did extremely well with Phabricator, which was then open sourced.
- Noumenon72 3y ago
  Git reflog provides history and rollback, what else do you need?
  - earthboundkid 3y ago
    Oh boy an ephemeral log that doesn’t sync.

codazoda 3y ago

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.

mvdtnz 3y ago
You're absolutely right, the author's problem imply massive pull requests that I wouldn't accept in my workplace.
bvrmn 3y ago
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.

noirscape 3y ago

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).

__david__ 3y ago
> 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.
u801e 3y ago
> 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).

kemayo 3y ago

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.)

jacobegold 3y ago
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.
llimllib 3y ago
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.

ghthor 3y ago

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. Sign me up for change sets!

fahhem 3y ago
Then sign up for Reviewable, which supports this (we called them revisions) http://blog.reviewable.io/tracking-changes-in-a-code-review http://blog.reviewable.io/tracking-changes-in-a-code-review Sorry for the shameless plug!
- vanous 3y ago
  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!
  - fahhem 3y ago
    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

prokopton 3y ago

It’s a slightly different methodology, but Git Patch Stack has worked well for us over the past year. The CLI is a huge help. https://git-ps.sh/ https://git-ps.sh/

juped 3y ago

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/ 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.)

Noumenon72 3y ago
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.
atq2119 3y ago
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.
- juped 3y ago
  Patches aren't to specific revisions, that's the Github straitjacket talking. They're patches. 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.

epolanski 3y ago

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.

neo2006 3y ago
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.
- duncan-donuts 3y ago
  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.

neo2006 3y ago

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.

fahhem 3y ago
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!)
jacobegold 3y ago
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.
- 20after4 3y ago
  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.

fahhem 3y ago

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

3np 3y ago
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.
- fahhem 3y ago
  We have an on-prem edition that doesn't even phone home, so you don't have to rely on us for anything but updates.
  - 3np 3y ago
    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.
    - fahhem 3y ago
      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

jacobegold 3y ago

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.

fahhem 3y ago
I agree, we work on Reviewable, which uses those same blobs. However, we add tags to commits we're referencing, just in case someone at GitHub gets around to implementing garbage collection!
- jacobegold 3y ago
  They must have refs on the internal git servers -- they need them for the "force pushed from X to Y" timeline events
  - fahhem 3y ago
    You're right, though I think that changed recently? I vaguely remember only the last force push showed up, but now I see more than one so :shrug:
  - bongobingo1 3y ago
    I've definitely clicked those links through to 404s.
tomasreimers 3y ago
Hi! I work with the commenter - for those wondering here's documentation on that feature (pulled off with pretty much only GH data): https://graphite.dev/docs/pull-request-versions https://graphite.dev/docs/pull-request-versions If people are interested, probably a thing we could do a technical blog post on.

Pxtl 3y ago

Azure DevOps has some of this, it has "pushes". You view the history of a PR per-push, so developers can commit early and often. 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.

facorreia 3y ago

It’s very weird that PR approvals remain “approved” after changes to the PR that was approved.

wcedmisten 3y ago
There's a setting to automatically revoke approvals as "stale" after new changes get pushed
atq2119 3y ago
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.
Noumenon72 3y ago
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.

akoboldfrying 3y ago

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.

ezekg 3y ago
I think this is what https://graphite.dev https://graphite.dev is trying to do.
u801e 3y ago
> 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.
- akoboldfrying 3y ago
  >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`.)

parentheses 3y ago

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.

bvrmn 3y ago
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.
hahn-kev 3y ago
Don't forget forks lol
mmcnl 3y ago
I completely agree.
lozenge 3y ago
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.

cryptonector 3y ago

IOW, something like stackable PRs.

Vendan 3y ago

Github had this planned in their old roadmap... But then they deleted it... https://web.archive.org/web/20220831234107/https://github.com/github/roadmap/issues/211 https://web.archive.org/web/20220831234107/https://github.co...

mikemcquaid 3y ago
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.
- als0 3y ago
  Wow, why would they spend so much energy rewriting Rails code into React?
  - rapnie 3y ago
    Maybe to go all-in on Blocks. https://blocks.githubnext.com https://blocks.githubnext.com
  - 0xblinq 3y ago
    Fashion and trends followed by new hires, most likely.
  - lozenge 3y ago
    Someone likely saw it as a good chance to "take over" more of the tech team headcount.
- 0xblinq 3y ago
  > 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.

a-dub 3y ago

it used to be worse. if you pushed changes that removed lines that had review comments on them, the review comments would simply disappear. but yes, i liked perforce too.

SleepyMyroslav 3y ago
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.

Lapsa 3y ago

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".

klabb3 3y ago
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.

TazeTSchnitzel 3y ago

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.

janosdebugs 3y ago
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. :(

danjc 3y ago

It might be less correct from a taxonomy perspective but why not just a different branch for each change?

globular-toast 3y ago

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.

intellix 3y ago

isn't this what Gitlab have? Instead of v1, 2, 3 you're able to see them at the state of each git commit hash

SergeAx 3y ago

This sounds like what they have in GitLab merge request flow: https://docs.gitlab.com/ee/user/project/merge_requests/versions.html https://docs.gitlab.com/ee/user/project/merge_requests/versi...

mvdtnz 3y ago

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.

choeger 3y ago

That approach works fine in gerrit. I always preferred it.

ngrilly 3y ago

100% this. It is the biggest problem I have by far with GitHub. I really hope they can follow this advice and do something closer to Gerrit.

josemanuel 3y ago

Gerrit works really well. Github could adopt that tool..

mmcnl 3y ago

I don't think this is actually an improvement. It's more complicated and most people don't need this. Don't let perfect be the enemy of good.

cyphactor 3y ago

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 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.