A Request for Comment on Code Review Process #73
Comments
TL;DR - new users can contribute to the project using tooling that they are already familiar with. Clang got a really cool feature because some non-LLVMer (not part of the core community) did some modifications and then our community was fortunate enough that someone within the community took the time to pull the GitHub PR for it over into Phabricator - I'm talking about -time-trace. From talking to people outwith the core LLVM community - the number one reason that people don't attempt to push legit fixes/features back to LLVM is that the code review process is a) non-normative, and b) scary as a result. I think moving to GitHub PRs, or at least offering them as an option, would greatly help onboarding people to the project. |
|
[ For context: I vastly prefer GitHub's PR flow to Phabricator ]
GitHub's PRs make it very difficult to provide a patch set that can be reviewed independently. The only way that this can be done at all is:
This is very error-prone. The root problem with GitHub PRs is that the target branch cannot be changed after the PR is created. If it could, then we could create PRs to merge into the next branch in the sequence but then merge them into main in order and reset the next PR to try to merge to main (or merge it into the intermediate branch if reviewers are happy with the roll-up). |
|
GH Bad:
GH Good:
Phab Bad:
Phab Good:
|
|
@davidchisnall @rengolin
When you merge PR1, and delete |
|
Also, a PR can have the base changed manually: https://docs.github.com/en/github/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request |
|
@kparzysz-quic yes that works but is has two drawbacks I believe:
|
|
In 2019 at the Women in Compilers and Tools Workshop before the LLVM Developers meeting and again this year at the Community.o summit in March, we held workshops discussing barriers for new contributors. In both workshops utilizing GitHub pull requests was identified as a way of lowering the barrier for contributions. Good, bad, or otherwise, git has become the dominant revision control system for software, especially open source, and hosting services like GitHub, GitLab, Bitbucket and Azure DevOps have made the pull-request model the standard workflow for the industry. Whatever warts GitHub's pull requests may have, they are the most widely used model for contributing code and providing code reviews. LLVM going against the grain is an unfortunate detriment to our community. |
|
While I'm unhappy with github PRs, @llvm-beanz point is more important than my personal preference. While I'm also unhappy with Phabricator, the reverse is also true. To me, the best course of action is to find a set of guidelines for people that are used to Phabricator to do the same on pull requests, and keep that document up-to-date. I want to make sure we don't alienate existing contributors for the sake of new ones, but that we also don't make it hard to have new ones at all. My personal view is that existing contributors know more about llvm and our community and would be more comfortable adapting to a new paradigm, for the time and love invested in the project. But that's my personal view and in no way takes into consideration other people's difficulties, which could be many, and serious. |
|
@joker-eph Yeah, we could end up with hundreds of branches in the main repo. I guess we could agree that branches that are merged should be deleted, and, if left undeleted, can be deleted by anyone (unless they are explicitly made exceptions). The force pushes are probably inevitable given that PRs work on commits, not just diffs... |
|
I would suggest that all series have to be managed in the developers repos, and only one of their branches to be requested against the official repo's main branch. So we can all have different branches and a meta branch that Cherry picks from those, probably even have a script for that, and then when the metà branch is pushed to origin, the PR is updated as expected. |
|
To me, what tool I use for code review is one of the most important technical decisions that impacts me as a community member. Last time I gathered data, I participate in approx 80-100 distinct reviews a month in the clang and clang-tools-extra projects, so the choice of code review tools impacts me basically all day, every day.
Once you get used to the GitHub workflow, creating patches from PRs off a branch is quite easy. I'd note that some people use arcanist to simplify creating a Phab patch the same way (I'm not one of those folks, I use Phab directly through the web interface). I also like how easy it is to link to issues with the GitHub UI compared to Phab, but this is a lesser benefit to me (copy-pasting a link to bugzilla is not a ton of effort). The more full-featured markdown abilities in the editor are a nice touch, but not a critical thing to me.
The code review interface has far less surprises and annoyances. I'm sure this comes somewhat from the amount of time I've used Phab (many years) vs the amount of time I've used GitHub (~2 years), but I've never found GitHub's code review workflow to be particularly ergonomic. It hides the most important files in a review because there are "too many changes" in it, too many comments on a review slows down the web interface, etc. It's basically the same usability concerns that have been brought up by myself and others every time this topic comes up. Having used both for a few years now, I still find I am productive in Phab in ways that I'm not productive in GitHub, but it's not easy to boil it down to a succinct "if you fix this, my issue goes away" issue so much as an overall workflow within the tool issue. Anecdotally, it seems to take me about 25% longer when reviewing in GitHub compared to Phabricator, so I'm worried whether I'll be capable of reviewing at the same pace I currently do if we switched.
None that matter to my day-to-day work, at least that I'm aware of.
Herald-like subscriptions where I can be added as a required reviewer for changes with a file-level granularity. (At least, I've not found the functionality if it exists.)
My personal preference is to continue to support Phabricator (whether through Phorge, our own fork, whatever). However, I understand the desire to not host our own solutions or have to maintain them, and there's a lot of allure for using GitHub because of its popularity. But I'd like to reiterate my concern about the danger of switching tools (regardless of what we switch to). The Clang frontend already struggles with review velocity, as can be seen by the number of time people need to "ping" a review thread. This isn't because reviewers are struggling with the tools, it's because reviewers who know the internals of the product well enough to make a decision about a patch are hard to get time from. Increasing the number of PRs the community receives while decreasing the amount of qualified reviews is a very real possibility and is not the kind of experience we want people new to the community to have. I don't know how to address that, but I do know that for myself, I definitely see a decreased amount of productivity when working in GitHub compared to Phab and I know that my review queue already sometimes fills up faster than I can perform the reviews today. |
Having hundreds of branches in the main repo because of PRs and patch sets is unmaintainable. Anyone deleting any branches is dangerous, as GH doesn't really check much before deleting. I don't think that's a viable solution. |
|
IMHO we should prioritize switching from Bugzilla from GitHub over Phabricator. Reasons:
Given point 3 and 4, there seems no immediate reason to switch away from Phabricator, especially when compared to the state of our bugtracker. |
|
I wish that was true. While most focus of PR vs Phab is around patch sets, GH issues have a lot more shortcomings than bugzilla (by design, I assume), so there are a lot more things to worry about than PRs. However, I'd also refrain from commenting about bug tracker on a code review thread. We can only handle so much controversy at a time. :) |
|
I think having a bot enforce that presubmit tests run, with documentation on how downstream consumers of LLVM might help set up bots to help participate in the CI presubmit, might be nice. The bot could work as a "merge queue" in that patches must pass CI before being merged, and that the bot alone is responsible for merges, of patches passing CI in FIFO. Also, having some dedicated reviewers could help replace "herald" from phab, AND help new contributors get reviewers auto assigned. (I suspect many newbies don't know who to ask for code review of their initial patches; having a bot analyze |
This was duplicated between ExportVerilog.cpp and BlackBoxReader.cpp, but it is a generally useful utility function.
|
I've used I don't know how many code-review systems over 40 years. They all fail in one way or another. Moving to GitHub PRs will if anything simplify my life, as I'm already used to GH Enterprise for my downstream repo. Migrating to GitHub PRs (and Issues) is inevitable. It's how the open-source world works nowadays; let's go ahead and bite the bullet. But let's also not shoot ourselves in the foot.
|
|
@pogo59 that sounds like a good plan. I also support moving to github, both issues and pull requests, but the work to get there isn't trivial and I would hate to rush this and make things worse in the end. |
|
I think that in moving to pull requests the bots sending mass emails problems should get easier. Most PR testing systems support bots commenting in the PR instead of emailing so the audience is just people subscribed to the PR. Also with the expansion of pre-merge testing the number of failures post-merge should drop dramatically. |
|
I strongly support moving to GH PRs. We've been using them in the CIRCT project from the beginning. Yes, there are downsides mostly stemming from GH's desire to simplify things; but, I personally haven't come across anything which I needed to do which just isn't possible. Our project is far smaller and far less complex than LLVM, so mileage may vary.
Some downsides:
Overall, I think the network effect is extremely beneficial and probably the best reason to switch. If there's a feature we need, we could likely add it via either a GH workflow or a bot. I've also noticed a significant acceleration in GH feature velocity over the last few years, so things are just going to get more featureful. To that end, GH does publish their roadmap: https://github.com/github/roadmap/projects/1. |
For what it's worth, GitHub actually has documentation on the special |
|
I'm strongly against moving to PRs at this time, for reasons I'll detail below. I don't think any of my concerns are unsolveable necessarily, but if we move without addressing them, I'll really not be happy, and my ability to review contributions will decrease. This is particularly important on both counts, because I'm currently not even assigned to an LLVM-related team in my company, so I can't justify spending more than a relatively small amount of time reviewing things upstream, and I'm also one of about 2 people who currently reviews changes in most of the LLVM binutils, so my review bandwidth is critical to development in this area. Also, for context, my company uses Github Enterprise internally, and has done for a number of years. I feel like I have about equal experience in both GHE and Phabricator, and the latter is my preferred review system by an absolutely huge margin.
|
PRs will get reviewers added automatically for specific files/folders if you have a CODEOWNERS file in the repository. |
Within LLVM, there's a big difference between code owners and reviewers. I'm automatically subscribed as a reviewer to several areas of code, but I don't consider myself a code owner of these. Also, IIRC within our GHE instance at my company, when trying to add reviewers to a list that had been prefilled with many code owners, several of the existing reviewers were removed (or new ones not added) due to there being a relatively low limit. |
|
A negative against arcanist: A newcomer may have to learn both the idiosyncrasies of git (which I think most would agree is not straightforward) compounded against those of Phabricator and arcanist. One specific harm of this is that it makes it harder to pull down a patch from Phabricator without mangling something or ending up in a strange git repository state. Simple enough for an experienced person to fix, but a barrier to entry for a newcomer. Phabricator's tooling, documentation and ethos is playful in admitting that it is 'arcane' but this is not practical and utilitarian when it comes to growing a community. Hosting on GitHub is better in this regard since it exposes pull requests as part of the usual 'git push/pull' mechanism that everyone learns if they use Git anyway. |
|
As a fairly newcomer to the project I vastly prefer GitHub Pull Requests because of the ease of use and the familiarity. Phab was a huge obstacle and learning curve for me. I have not been able to install arcanist since it depends on PHP. |
Well that depends on how you interpret "code owners". In GitHub it just means someone that normally reviews changes to a particular file, folder etc. I would be added for the Arm backend, for example, as my colleagues add me as a reviewer to their Arm backend patches. |
Right, but my point is that the LLVM definition doesn't match GitHub's (see https://llvm.org/docs/DeveloperPolicy.html#code-owners) - code owners perhaps should be reviewers on all patches (even that's debateable), but not all people who want to always be a reviewer are code owners. |
|
While I always hated the GitHub interface for reviewing code and still do (for reasons others have mentioned so I won't repeat), I will of course adapt to whatever tool the community chooses. One thing that seems quite concerning to me though is that I have seen several mentions that "force push will be inevitable". Although I don't understand why that would be, I always think that using the force option is an indication that something has gone terribly wrong and needs to be fixed. So I am honestly concerned/afraid of any process that routinely requires using the force option. Also, I find branches to be a very useful concept when there is a very limited number of them (i.e. main and release branches that we have now). However, once everyone needs to push a branch to get their code reviewed in a project as large as LLVM, the number of branches is essentially guaranteed to be so high that in my opinion, the whole concept of branches becomes rather useless. But of course, this is just my opinion and many people on many projects have found ways to work effectively in the presence of so many branches. |
I frequently encounter situations where a proposed change has portions split out and committed separately (and possibly with slight changes). Rebasing after the split portion lands is reasonable and probably recommended (if not required).
While I generally experience more instances of missed-and-unresolved comments in GitHub than in Phabricator, it has not been caused by rebases (but by how GitHub manages inline comments and replies to such). Something that I have only seen happen with GitHub is the author forgetting to pull the PR branch back to their local copy before doing a rebase (and thereby blowing away all of the applied suggested changes). |
My experience of development is that staying up-to-date with To stay up-to-date you need to either rebase on top of Also: did you get some good experience in mixing this with the dependent changes that are meant to be reviewed independently? (aka "stacked PRs") |
In my experience rebasing is a lot less painful when the workflows are entirely git based (i.e. operating on git branches). With Phabricator I find myself re-resolving the same rebases over and over again. When I update a change early in a stack of dependent reviews, updating all the downstream changes is... unpleasant. (I realize there may be some magic workflow that I don't understand with arcanist/phabricator, and would love it if someone would tell me there is some magic easy way to rebase commit stacks). In a fully git branch based workflow the rebase resolutions build on each other. If I rebase the first commit in the stack, I can rebase the next commit off the first, and I don't have to re-resolve the changes over and over again. Or if I want to short-cut rebasing, I can reset the second branch to the rebased first and cherry-pick the second commit on top of that. It is really painless (
I don't know why you seem to think rebasing bad or difficult. In my experience most rebases you resolve conflicts once and they sort themselves pretty well, even if you're touching the same code over and over again the resolution is often automatic or at worst usually mechanical.
Yes, the final merge can always be reduced to a "squash and merge", which is more or less turning a PR into a single commit on the target branch.
I have done a few dependent PRs in different projects. Swift has some infrastructure to manage dependent PRs, but it always seemed overly complicated. When I've done them on other projects I make the first PR target the intended target branch, the second PR target the first PR's branch, the third PR target the second PR's branch (and so on). As each PR merges you can re-target the next PR in the chain to the right branch. Minimal rebasing required. I find rebasing git branches way easier than rebasing Phabricator reviews, and strongly prefer workflows that keep me in git. |
My experience is that arcanist is completely braindead with commit stacks. phabricator is horribly obfuse. Fortunately, moz-phab handles commit stacks in a sane way from the command line and as a result, dealing with commit stacks is one of the few places where phab is better than PRs IMO. Basically, you rebase your patches as normal and Steve |
|
Thanks @stephenneuendorffer for pointing out moz-phab. Since Mozilla has forked Phabricator in reaction to the winding down announcement, and is continuing to maintain it, could we just use their fork? |
I handle Phab no differently than any git-branch based flow. I have a stack of commits, each corresponding to a single Phab review. I use interactive rebase and
While this is true for dependent changes in separate Phabricator revisions (as long as they are dependent from a semantic point of view, but the actual diff aren't conflicting with each other), I don't see how this extends to your earlier suggestion of addressing review comments with separate commit instead of amending. See for example a pull-request sequence of commits that would look like:
Now you have to rebase on main and it turns our that function
I don't know how you're doing Phab review, but |
|
A made up repo with a concrete example here: https://github.com/joker-eph/test-rebase-PR/pull/1/commits |
|
@joker-eph have you tried rerere ? |
|
@joker-eph @llvm-beanz I have recently worked on a sizeable implementation to Clang-Tidy, which involved 7 highly interconnected, but at the end of the day, linear pathces. (See http://reviews.llvm.org/D69560 and its stack.) My workflow was that I created, for each patch, a separate branch in my repository, and worked on those. Because the Git history was not connected to the review process per se, I allowed myself to go wild with having a lot of commits on each branch. And let's say something came up that required changing something in patch 2 out of 7. I did the following steps:
(Alright, this is a bit exaggerated, because I did now always upload EVERY changed diff to Phab. I.e., if I changed something in patch-2, I updated patch-2 to signal the reviewer that I want them to look at my fix, but I didn't update patch-3, etc. on Phab just because of this fix. Only once the review started touching those patches.) In fact, I created an alias, Now the positivity of this workflow was that I used Git's history tracking as intended - every little bit of detail was recorded verbatim. And I do not remember having to deal with merge conflicts at all (because I never rebased -- might have been lucky with the fact that the API I used wasn't pulled with the rug out from under me...), but I definitely do not remember having to deal with the same conflict multiple times. I did not have to, because once a committed change got merged into the branch of the next patch conflict-less, the subsequent merges into the following patches had to be conflict-less by induction... Now if we contrast this with the Git(Hub PR) workflow:
For full disclosure, I do need to admit that after several rounds of executing these steps, the history of my work "locally" looks like the better part of a rail freight terminal, but had I not posted this comment, I am sure the people who reviewed my work would've never gotten to know this fact.
By using GitHub PRs that tie my hands by hard coupling the pull request contents to the contents of the branch that is submitted, I either have to administrate things myself across separate branches (which is parallel to submitting "detached" patch files to Phab), or force the reviewers to observe content that they are not interested about (my granular patches), or force myself to create big incremental changes, which might work until the point it doesn't, and if in between two commits that you push to a PR something breaks, and because the diff between those two commits contained several independent(-ish?) changes, you cannot really bisect a potentially introduced bug anymore. Both of these forces actually defeat the purpose of Git being a distributed VCS and every user having full authority over the contents of their repositories and clones. (I.e.: it is my private implementation detail that I created several small commits, the public contract I give to you, reviewer, is bigger incremental updates.) Fuller disclosure: I have never, ever, used Arcanist. My work computer is incapable (or unauthorised?) of executing |
I review multiple-commit PRs on GHE all the time. What the reviewer sees by default is the final state of the branch compared to the target branch. I don't care about all your crappy intermediate commits. The only reason to be needing them is if people insist on looking at how one revision differs from a previous revision, which it sounds like some people do want; and then I think people in general will be kind enough not to point and laugh. So I don't think this point is really a concern. |
|
On a related note though, although @pogo59 is right that the default is the diff of the head of the branch versus the base, I regularly find that the view I'm looking at is not this view, but rather changes since the last time I reviewed, and I have no idea why this happens sometimes, but not others. Whilst it's a useful piece of functionality to be able to view the changes since my last review, I don't want it to be as apparently arbitrary as it is - I want to be able to make the conscious decision direct from my email notification, without having to remember to click the appropriate menu in github. In Phab I can do this: either click on the review link in the notification, or the "new changes" link. |
The link from the "pushed N commits" notification e-mails only shows the changes from the range of commits. |
People won't be pointing and laughing, just frustrated by the process of locating the potentially-new (due to rebase) version of the commit that they left off at in a slew of minor commits with similar subject lines. |
And that clicky thing in the notification gets broken the moment a branch is force-pushed. For example, I just recently received two emails to a patch I'm subscribed to, outside this project. sha Email 1
Email 2
If I click either of those "View on GitHub" links, I end up on a page that says "Commit range not found". Even though the latest e-mail notification is in fact the state of the request's head-branch right now -- i.e. if I then click "Commits" on GH's interface, I get a list of 5 commits, in the following order:
So I am not sure what it is trying to compare with and why, but I have to click "Files changed" to receive the full diff. It has to be mentioned that the "discussion feed" of the PR explicitly mentions force pushes:
Both of these "force-pushed" messages have their own "Compare" button, which do bring me to a comparison... a comparison which shows minimal (2 lines) of change. While the abstract example cannot fully convey the meaning here, I have to say that as someone who knows the project these notification emails had come from, those 2 lines of changes are way too small compared to what the patch itself contains. So I still have no idea what was fixed between these force pushes and changes (Is this only what is shown by the "Compare" button here? It's not very intuitive, even though I would like to think I know a few things about Git...), and if I imagine I was reviewing this patch, I would still not be too sure as to what the patch author had changes (outside of the "marked as resolved" discussion threads). On Phabricator, you can directly compare two "snapshots" or "updates" of the same patch. I am sure with the right SHAs I could do the same on GitHub's interface (or after a recursive fetch, locally...?). It feels as if I had these two options:
|
|
Something that occurred to me just by reading up the replies since my last comment... If you click Quote reply to make a reply, it will only copy (with
And if you already opened the interface, to find the context of the replied comment (for example if someone only replied to a single paragraph or a single sentence?), you need to use "find in page" instead of clicking some cross-reference and getting to the replied post immediately. This is a feature that the most trivial web forum engined had 20 years ago, and yet we do not have it here. Note: this is not specific to PRs, however, and could be an easy fix from GH's developers. |
|
Since there's been some discussion about linear history: I love that LLVM has a nice (mostly) linear history where each commit represents a sensible unit of work and has clear titles and descriptions, which makes it easy to examine the changes to particular files and directories, and also makes bisecting work much better in my experience. Compare LLVM's history to Swift's and Rust's; this is what the three look like right now. LLVM: Swift: Rust: Swift and Rust have so much noise. I imagine I think Phabricator plays a huge part in LLVM's clean history, because it has the clear model that each review on Phabricator will correspond to a commit in the repository, and reviewers can then easily enforce good commit hygiene. In particular, stacked reviews are a great tool in enforcing a clean history with sensibly-sized commits, and stacked PRs on GitHub seem like a pain (which would discourage their use): every reference I've found says that you need to maintain a separate branch for each commit, and use a third-party tool like One other point: when I last used PRs, I couldn't formally assign reviewers to them unless I already had write access to the repository; I just had to CC people and hope they saw it. It seems like that's still the case, and that seems decidedly more beginner-unfriendly than Phabricator, which lets you always add reviewers. |
|
TLDR: We need to have a discussion about what we want the commit history to reflect: 1) a nice, clean history that is easy to follow or 2) a detailed, meticulously accurate history that will preserve every version anybody creates. A lot of this new discussion has appeared to centre around how we want PRs and PR branches to be managed & merged. There are many different approaches for this, each with some advantages and disadvantages. However, perhaps this is far enough removed from the original topic of this issue that it should be considered separately? A few things we should talk about there:
I will also say that I think much of the disagreement in the comments above stems from people using git in different ways and not really communicating how or why they use it that way. There are many ways of using git, and no one 'right way' of doing that. It depends on what you want out of it. The biggest issue I usually see at the core of this discussion that often goes unmentioned is what do you want the history to reflect (and why): 1) a nice, clean history that is easy to follow or 2) a detailed, meticulously accurate history that will preserve every version you create. Both option 1 and option 2 come with many benefits.
A few benefits of option 2:
I personally lean HEAVILY towards option 1. I find the benefits to debugging (both short and long term) are huge, and the ability to use git to organize my thoughts is almost essential for me (I get very lost when I'm forced to use another version control system). Anytime I want to avoid potentially losing work I can either rely on the reflog, or create a tag at the commit before performing a rebase or whatever, then delete those tags once the feature has landed and/or I'm confident I no longer need them. If our goal is to re-create as closely as possible the phabricator workflow, something between option 1 and option 2 is probably best, and we can get extremely close if we want. However, I don't think blindly imitating what we already have is the best choice. Phabricator didn't have this range of options, so we didn't have a choice before, but now we do, so we should have a conversation and make that choice ourselves instead of preserving whatever choices it made for us. But, again, I will re-iterate what I said at the start, I think this subject should be discussed in a separate issue.
There is absolutely no such "intended" way of using git's history tracking, and you will find no mention of such anywhere in any documentation for it. It sounds like a lot of the complaints you have stem from using git this way, and there's absolutely no reason you need to. Interactive rebase, squash and |
|
I like to introduce the concept of Github PR workflow playgrounds. Somebody with the right permissions forks the llvm-project as llvm-project-playground and enables PRs with any level. The explicit statement is: the fork will be deleted in finite but unknown time. Then we can iterate and document the LLVM GitHub PR workflow. |
I think it's premature to document a GitHub PR workflow before the community has consensus we want to use GitHub for reviews. If this is something that's hidden and is an experiment to see how well GitHub PR workflow would work with our repo, that's a different thing entirely. But if this is something we expect code reviewers to participate in as part of our daily workflow, I'm a strong -1. |
|
My bad. You can use the playground as an experiment. You could have a playground with e.g Bors. IFF the community decides to move to GitHub PRs, then they could use the playground concept to develop the LLVM workflow. |
|
Ah, thank you for the clarification (and the initial suggestion), that makes more sense now. :-) |
|
The way I see it: we need to document a proposed GitHub PR for the community to be able to clearly build consensus around a migration to GitHub PR. |
|
The LLVM hosted playground will draw more attention and more traffic. There will be more PRs. Are you going to setup Bors for your fork? |
|
Who’s attention and what kind of traffic are you looking for? |
|
The advantage of doing something like this in the LLVM organization is that we get access to the custom runners provided by GitHub that are not available for all users. We also already have a sort of PR playground that we use for the release branch: |
|
(I don't know where to record / broadcast this) |
It seems there are various issues under https://github.com/orgs/llvm/projects/11 where the open question has an answer insofar as the Carbon project is concerned. It seems they have a setup to support stacked PRs via branches in the (non-fork) repo, enforced by naming conventions: https://github.com/carbon-language/carbon-lang/blob/trunk/docs/project/pull_request_workflow.md#stacking-dependent-pull-requests. It seems they allow rebases of PR branches and non-squash merge: https://github.com/carbon-language/carbon-lang/blob/trunk/docs/project/pull_request_workflow.md#stacking-dependent-pull-requests. So, insofar as multiple-commit reviews are concerned, they still have multiple ways to go about it. |
|
Sorry for chiming in late here, but I realized that something else I rely on pretty frequently is the As far as I know, GitHub PRs don't have any equivalent. If we were merging them you could figure out which merge brought in a commit, but we're not (which I fully agree with, given how messy history would get otherwise). What's the standard Github workflow for figuring out the pull request corresponding to a commit then? |
|
The tile in a github commit will include the PR number like (#2257) at the end of the title. Then you can go to
The tile in a github commit will end with the PR number like (#2257). Then you can go to |
and others
Proposal
The LLVM Foundation Board of Directors is seeking comment on the current state of Code Review within the LLVM Project and its sub-projects. Phabricator is no longer actively maintained and we would like to move away from a self-hosted solution, so our goal is to determine if GitHub Pull Requests are a good alternative to our current code review tool: Phabricator.
Specifically we are looking for feedback on:
Where to Direct Feedback
Please provide feedback on this Infrastructure Working Group ticket. This will make it easier to collect and consolidate the responses. At the end of the comment period the Infrastructure Working Group will collect the feedback for further analysis and summarization.
Timeline
The timeline for this RFC will be as follows:
The text was updated successfully, but these errors were encountered: