It would be nice if GitHub supported a Gerrit-inspired code-review process, where instead of having to choose between:
1) piling new commits onto the existing branch/PR, or
2) force-pushing and completely losing the old commits on the server
You could instead push into a "magic" ref-spec and the server would retain the original commits, but also the rewritten commits, such that a PR can have multiple revisions. This is somewhat hard to explain unless you're familiar with Gerrit and its "patch set" workflow.
Why? Because my preference is to rewrite history in order to address code review comments, such that what is finally merged leaves a clean history. But it is also valuable during the code review process to be able to look across the revisions to make sure everything has been properly addressed.
The only way to do both, today, is to create a new branch and PR for each "revision". i.e. new-feature, new-feature-rev1, new-feature-rev2. Then close the original PR and reference it from the new PR. A bit tedious.
Hey, I'm one of the devs for this new PR stuff. This flow is something we're hoping to improve as well. Force push support is pretty second class right now, any previous commits and discussion basically gets lost. It sucks.
So we already use a internal refspec for tracking the latest PR HEAD. You can actually manually fetch this under `refs/pull/123/head`. However, this is only the latest HEAD, not any previous histories.
My idea was to expand this to include tracking all historical HEAD refs. We came up with a clever/hacky idea to string together a chain of dummy commits pointing at all the historical HEADs. This fake commit chain would only be used internally, but it would ensure git's gc reachability checks don't sweep up any old PR HEADs. Its an alternative to creating a new ref for each previous HEAD or maintaining text reflogs for an entire repo network. Both have some performance issues for larger repos.
Its something we're still working on, but it still helps to vocalize your support for wanting improved force push support. :D
> Its something we're still working on, but it still helps to vocalize your support for wanting improved force push support. :D
YES! I wish for that feature everyday. Gerrit does it exactly right.
Related to this, I really, really hope that you will consider displaying commits
in a PR in the correct order, suppose that I have three commits on top
of master:
a->b->c->master
If I git rebase -i master and reorder my commits so that they look like this:
c->a->b->master
Then the commit list in the PR will still be displayed in chronological order
instead of DAG order, this is very confusing! The only workaround I've found is
to amend every commit in my PR so that the timestamp order match the DAG order,
this isn't very fun to do.
By the way, the post explicitly says:
> Some teams choose to use a commit-by-commit workflow where each commit is treated as the unit of change and is isolated for review.
I'm glad that you're acknowledging that. But then why do comments on commits
still get hidden when they correspond to an outdated diff? I would love to be
able to comment on individual commits, but if half my comments are hidden
because they correspond to lines that don't match the current diff, then I'm not
going to do it for fear of my comments being missed. You should not hide
important information that I'm trying to communicate!
Here's a better way to do it: Add a "Done" button like Rietveld/Gerrit for every
comment (this is a very useful feature on its own) and hide comments _if and
only if_ the "Done" button has been clicked.
Anyway, I'm glad that people are working on this stuff and that they're
listening to the community, thank you!
No, if you comment on the commits themselves rather than the PR 3rd tabs, they disappeared (at least before this change). The only place they were left is your emails, an hardcoded URL or your feed. This was (is?) bad...
they do disappear in one sense: the link to them sent out in the email telling me about them goes away. And if I have a bunch of collapsed comments, it's annoying as hell to find the right one. This is usually an issue when the developer says something like:
"ok, I fixed that, but what do you think about xyz?"
Now I have to go to the comments and expand 35 of them until I find the right one so I can respond to their question.
It's also sometimes the case that changing a line is not the same as addressing a comment, or maybe it only addresses part of a comment, or maybe they just changed whitespace because now all that code you commented on is inside an if...
I also wrote a Gerrit to GitHub PR proxy... I should ask $dayjob about open-sourcing it. And I seem to recall there's another as well implemented as a gerrit plugin.
bots like these seem to be a natural way to work around github's limitations. realistically, they won't be able to support everyone's use case, but they can provide a platform where lots of different tools can be integrated together.
unfortunately, bots like these are very annoying to write right now. at least one reason is that there is no github event for editing comments. if you try to use webhook events to mirror comments made in github to another code review tool, you will quickly drift out of sync if those comments are edited in github. instead, you have to implement polling, which comes with it's own set of challenges.
also, in general, the event formats are pretty inconsistent. as a random example, in the push event the repo owner only includes name and email, while in the commit comment event the repo owner includes all sorts of information about stars, followers, etc... there are inconsistencies like that all over the place, which means you might need to implement additional API requests for some events and not others.
on a somewhat related note, i've seen a lot of speculation in the open-source community that the reason open-source is getting so little attention from Github is that they're focusing on enterprise customers. i can say that as a somewhat-large enterprise customer, i have yet to get a single issue addressed. examples like above (add a comment-edited and PR-edited event) have been in the ask 6 months or more.
Count me in on those who really want better force push support. Losing all the conversation on old commits sucks, but what also sucks is not being able to see a diff between the old and the new versions. If GitHub can track the history of HEAD for the PR and show diffs between them that would be excellent.
The dummy commit approach you outlined sounds like a good idea. It might be nice to expose that externally similar to how `refs/pull/123/head` and `refs/pull/123/merge` work, so that way we can handle force-pushes to PRs better in our own tooling. I'd suggest something like `refs/pull/123/history`.
Even with these new changes are comments on commits that are force pushed over still lost? It looks like when commenting on individual commits now they appear as if you commented on the whole changeset itself.
I have a somewhat shonky shell script which maintains a fast-forward branch that tracks the HEAD of a rebasing branch over time. I use it for keeping the history of a set of patches.
Part of a good review should be to review how you decided to split your PR into separate commits (does every commit have a single purpose, or did you sneak unrelated changes in a commit? Could the commit message be improved?). Furthermore, sometimes you have to rebase your PR on top of master (for example, because things changed in master too much, or because your PR depends on a patch that was recently accepted in master to actually work), this shouldn't make review comments disappear.
Have you tried addressing the comments with "fixup!" commits? At the end of review, you can close the pull request, "rebase --autosquash --interactive" the branch, and merge manually.
This is the flow my team uses for addressing any comments, concerns, changes, bugs, etc. We push all subsequent commits to the branch under review and prefix the commit messages with either 'fixup!', 'squash!', etc. that address the comments. The team then reviews the fixup commits further making additional comments or giving a :+1 to the specific commit SHAs. Once everything, including all fixup commits get a :+1 as sign-off will be the only time the developer does a force push to the remote branch under review. Then the developer that owns the PR is responsible for the squash, merge and closing the PR.
Using this flow we never lose any comments during the PR process and can revisit the individual commits, diffs and files changed while the PR is open.
When I do fixup commits, I often think there will be no conflict when it gets rebased but it turns out there is. Do you not worry about mistakes entering at this (post-review) stage?
Yes. It has happened especially as the team has scaled in developer size and when we are in development modes where there's better chances for conflicts, for example iterating on core code during feature development, or when we're touching/adding to common configuration settings. But it hasn't been that often where a developer needed to fix a tricky conflict.
I encourage members on our team to rebase frequently as they're iterating prior to opening their PRs for their individual feature branches and I think this helps us avoid problems for our typical PRs.
Also as a rule, don't sign off on a PR until all conflicts are resolved, if there were any and review the final commit(s) carefully.
We also have the option because of some of the CI tools we've written to give our QA people access to specific SHAs that can be tested, so sometimes (rarely) QA sign-off is asked for as part of the PR.
I use fixup commits locally, and rewrite before pushing. I never considered pushing the fixup commits to the server. That's just another work-around, but an interesting one.
Phabricator does this too, but it works even if you force-push to the same branch - it lets you compare the base commit against all subsequent (force-)pushed commits onto the same ref.
Cool. Gerrit requires that each commit message have a "Change-Id: ...." footer so that it can pair up the new incoming commits with the old ones. And you have to push into its magic ref-spec namespace. Neat that Phabricator works by force-pushing over the existing branch, and just keeps track of its previous SHA1s, sorta like a server-side ref-log, exposed via its UI.
Specifically, I would love to see GitHub extend its searching capabilities, and its code review, to be able to encompass more of a Gerrit style approach.
For example, let's say you want to have two core reviewers give a positive review on a PR before it lands. Right now your best hope with GitHub is tags, and that is frail and complex. In Gerrit, it's just some configuration.
Or, being able to show yourself only PRs that you haven't reviewed yet, or maybe ones that haven't had any reviews yet. In gerrit, it's some search terms. In github, it's another big complex tag operation.
Why can't you do 1), but at the end of the PR, after everything is cleared and it is ready to merge, squash/modify the commits structure however you like?
I guess this is probably a subjective topic, but I quite like they way Assembla handles it's Merge Requests.
When you make a change, all your new commits show up in a new version, and you can switch between the different versions of the MR to see the old state of the code before each new version. Where a version is created after a push of new commits to the branch.
You still keep the entire commit history (which I much prefer), but the view of that history and discussion around it is much clearer in the MR.
Github's way of doing it makes it difficult to see a concise view of the history of the MR, which can contain very valuable information outside of the commit history.
Mercurial Evolve has a meta-history of which commit rewrites what other commit. It works great, but so far nobody has built a WUI on top of it for code review.
Reviewable (https://reviewable.io) has strong support for a rebase-centric workflow while still integrating pretty well with GitHub. Once somebody has reviewed a commit it never disappears, even if you rebase and force push (special refs keep them pinned in the repo). You can easily get incremental diffs between the old and rebased branch or individual commits.
I'll shortly be deploying a new feature where Reviewable will try to match up rebased commits with their predecessors and automatically pair them up when diffing. This is just a heuristic based on commit messages but it works pretty well for non-interactive rebasing. Otherwise, you can still manually diff any two revisions you'd like.
Reviewable does this really, nicely. We switched over our entire code review process to them (sits on top of GitHub) a few months ago and haven't looked back.
That button only appears when you add new commits to the existing branch/PR. If you amend any commits and force push, the rewritten commits are lost forever on the GitHub side of things. You can only find them in your local ref-log at that point.
Gerrit instead retains each rewrite of the "same" commit. It does this by requiring you to insert a "Change-Id: ..." footer into each commit message (it provides a repo hook to do this) and examines each incoming commit message to know whether that commit is a revision to a commit it's already seen.
Can you diff between multiple outdated diffs? One of the really useful things in the gerrit workflow is to check what's changed since you last reviewed a patchset.
This is what we're actually making inside RhodeCode, we already have explicit PR updated, and we actually version pull requests, and each update of it. Imho pull requests is a set of changes and you need to track how it evolved.
Does anybody else feel like GitHub has released more features in the last month than the last 6 months? I'm not sure if it's just a coincidence with all the attention they've gotten on HN, but these improvements are much appreciated!
Code review tooling has been in the works for a long time. It's sadly taking us this long to actually ship the first fundamental bits, but there's a ton that this unlocks for us. Expect to see more soon :).
(Source: I worked on code review at GitHub for awhile!)
In that case, is there any plans for your processes to change to get a more rapid release-cycle? Going dark for N months to implement a feature seems to fly in the face of the workflow GitHub tends to inspire.
Sometimes you're on a rapid release cycle making non-user facing changes that will allow you to release new user-facing features. I have no idea if this is what was going on at Github, but I've been at plenty of places where the code and/or infrastructure was so messed up, seemingly trivial updates would take weeks and months to complete.
It was probably in reaction to the Dear GitHub letter. People were considering migrating away from GitHub and so they got their hands out of their pockets. We all benefit, though, GitHub becomes a better platform for us and they probably become a more successful company.
People who defended them should take note. Complaints can lead to action. Cheering for software companies is as useful as cheering for sports teams.
If you go into defence mode whenever someone complains about your favorite VCS, OS, language, platform, editor, start menu, or whatever then you're probably doing it a disservice unless you're disputing factually incorrect information. Posting work-arounds and minimizing others people's complaints isn't helpful.
Home teams have an advantage in most sports, and crowd noise is a factor. More relevant to your point though: encouragement and moral support are important to many OSS projects, where burnout is a particular risk. Maybe not applicable to github per se? /random thoughts
I get what your saying, but the sports analogy might be a bit off. Most sports fans are most critical of their own teams and expect to lose every game.
The problem with github is that every time something about them comes up, everybody has their own ideas about what the "most critical" thing that Github "needs" to do is. Tracking and collecting those, doing their own user studies to find pain points, making sure you're not hurting existing behavior, all of this stuff takes a lot of time to get right. I'm not surprised that they have long fallow periods followed by quick bursts of updates.
My guess is the switch from zero management to a more top down approach made the top realize nothing was moving forwards. GitHub had been stagnating for a while until then. But I could easily be wrong.
None of what they just released impacts the GH Enterprise side of things. Maybe it will long down the line, but suffice it to say, none of this sells or keeps customers of GH Enterprise. The reality is, we've questioned our use of GH, and while we are still paying for GHE, it's more or less because it's just not expensive enough at this point to justify switching to something else. We'd have to update some tooling, and migrating would require time.
Basically, GHE is a glorified code repository viewer, and even then, it does a substandard job of that. Seriously, the ability to browse and search painful.
So yeah, if they are focused on GHE, these features don't suggest that.
Hey just an FYI, we (my company, not GitHub) are working on improving the browsing/searching situation for GitHub and GitHub Enterprise. You can see a list of the improvements at:
We've only really started advertising in the last 3 months, but what has been quite clear is, people really overestimate what GitHub and their API's are capable of. This is not a criticism against GitHub, but more of a high praise for how well they have cultivated the GitHub brand.
People really think GitHub not implementing commits search, or not being able to search forked repositories (which is pretty bad from an Enterprise point of view), is because it's not sexy, which is why they haven't implemented it yet. And when they see what we are capable of doing with GitHub, they just assume we are using GitHub's API, when in reality, the only time we use GitHub's API is to get somebody's avatar.
We obviously can't complain though, as it provides us with a business opportunity.
> So yeah, if they are focused on GHE, these features don't suggest that.
Then have a look at the GHE release notes. 2.5 introduces clustering to improve scaling of large instances, which is really nice for large orgs like my day job in SAP Cloud Infrastructure.
I'm guessing by "focus" on GHE is more on sales and marketing and closing enterprise deals, rather than actual software development. Honestly, I'm really not sure. It might also be perfecting how they deploy to different customers' data centers, how they integrate with their solutions/platforms etc.
I'm guessing they didn't realize how annoyed people were. There's a pretty big gulf between "we hear complaints about X" (every company is going to have complaints about their product) and "lots of people, very well known ones, are complaining about X, we should do something about it"
I'd love it if there was a way for me to queue my comments before submitting. I often keep my comments in a separate textedit/nv window and then go back to put them in since I want to keep track of questions that arise as I'm reading, but I don't want to pepper someone with comments that would be resolved 200 lines later in the diff.
Yeah this is my #1 feature request, and the main reason that a number of companies I've worked for/chatted with have switched to using Phabricator for their code review.
As you enter your comments just don't click the "comment" button. Github saves the text anyway as you go. I don't know how long it takes to save the text (so maybe don't refresh or leave the instant you finish writing a comment) but in general they should all be there. Then you just have to scroll through and click comment on each one.
Certainly not as easy as seeing them in a nice row for review but it's a workaround at least.
This is my biggest feature request here as well. I'll often write a lot of small comments inline in the diff, and a bigger comment on the PR itself tying everything together. It'd be nice to be able to submit it all at once, with the ability to reference inline comments in my main comment.
Can't you just enter your comment into the text box but not hit the "Comment" button until you've gotten through the whole PR? I just gave it a shot, and it looks like you can have multiple comment text boxes open simultaneously.
Reviewable (https://reviewable.io) is built around this workflow: it will keep your drafts, let you review them, then send them all at once (triggering a single notification email).
This is what I was expecting this announcement to be. It's the most often requested change for GitHub Issues that I've seen. I hope they follow up with this soon.
The line count of a diff has nothing to do with how focused the patch is. Some PRs are simply larger than others due to the language in use or the nature of the code being changed.
Presumably a codebase in an array-oriented language would have miniscule PRs, which is irrelevant (discussion of PL merits aside).
This is a great step in the right direction. I use GitHub for code review every day, and it has historically been very poorly designed for thorough reviews. These changes look great, I just hope that we get some sort of checking-off of review points, and accept/reject functionality.
I'd love a UI way to enable `?w=1` whitespace mode, especially for html/json/yaml files. I have a feeling a lot of reviewers don't know about it and spend an inordinate amount of time squinting to see what changes when it was a whitespace-only change.
Some similar option for enabling patience diffing as well would be amazing. Those two options in combination would certainly make a not insignificant amount of PRs much more readable.
I created a reminder for myself so that every six months I write an email to GitHub asking for someone to create a user option for this whitespace setting.
I wonder if there's a set of API hooks they could expose to allow third party applications to handle more advanced code review techniques? I would totally understand GitHub not wanting to implement a full Phabricator/Gerrit review feature set, but maybe they could make it easier to integrate other services for it.
GitHub has become the de-facto place to host open source projects. Improving the tooling around code merging, seems like a no-brainer, and a huge potential win for the projects using it, especially the larger ones.
It's interesting that you mention checking off of review points. I had begun to do that in an informal fashion by updating my pull request with notes in the form of a checklist:
- [x] Refactor _
- [ ] Rename variable x
- [ ] ...
Now that you mention it, it would be very nice to have something like Google Docs's ability to mark comments as resolved.
Unfortunately that doesn't work for us for 2 reasons -
1. There's a data loss bug when multiple people edit the PR descriptions. I've had that as an open issue with GitHub for ~6 months, my team experiences it several times a week.
2. We use the checkboxes to assign and track reviewers, and since there's only one count of checkboxes, it would mess with our "2 of 5 complete" kind of metric for reviews.
I'd like first-class support for the concept of people reviewing and accepting (or rejecting) so that it's taken out of the PR description.
You might be interested in taking a look at PullApprove (https://pullapprove.com/) -- basically you put a YAML in your repo that defines what code review looks like for your team (who, when, how many need to approve, etc. -- http://docs.pullapprove.com/). Approval/rejection can then be triggered by PR comments and it uses the status API so you know when the PR has passed review.
Code review "rules" can get pretty complicated since everyone works a little differently...not sure if GitHub will ever try to tackle that or not but we've got a lot of customers who are happy with how PullApprove fills the gap.
Folks who have this problem might want to check out Reviewable (https://reviewable.io):
1. Discussions are automatically treated as "issues" that need to be resolved before the review is complete. Resolving can be as simple as clicking "acknowledge" without following up, but there's also a rich system of "dispositions" that lets you block a discussion, resolve it unconditionally, etc.
2. You can set multiple assignees! (Only one will be reflected in GitHub, though.)
3. You can write a custom rule for determining review completion. One of the samples is "complete only when every assignee has sent an :lgtm:", giving the assignees explicit control over merge approval.
I know you've already decided against Reviewable danpalmer, but I figured others might still be interested. :)
1) jbrooksuk's answer, only one assignee.
2) Assignees are the person owning the changes, not the person reviewing.
3) There is no status associated with it, you can't say "checked" vs "unchecked", and certainly not unreviewed/accepted/rejected.
This is exactly how voting, and reviewers concept works in RhodeCode. You have accept/reject/under review states for each reviewer, and you have to make full voting in order to merge a PR.
This is always a deal breaker for thorough code reviews - it's always super hard to make sure that everything got resolved in a later commit. The only tool I've seen handle this well is SmartBear's Code Collaborator - you mark certain comments as defects, it does a decent job keeping them aligned with their context as revisions happen afterwards, and then you can close them out before marking the review LGTM. Unfortunately, Code Collaborator is horrible at basically everything else. It would be nice to have an easy-to-use todo list that's easy to close out and not lose context.
Yep, specifically Bitbucket allows you to create tasks from comments. Bitbucket Server also (optionally) prevents the pull request from being merged until all tasks have been resolved.
I've noticed some open source projects (particularly Angular 2) follow this convention where they never actually "merge" the PR, but rather rebase it into their main branch and have a message in the commit to close the PR.
The advantages seem to be that you get a cleaner git history, and you can keep all the "in-between" / WIP commits that tell the story. Is this done through an automated tooling or is someone manually rebasing it into master? It seems to be a really useful practice so I'm curious how people do it. It would be great if GitHub could offer this natively, as I think many power Git users appreciate the benefits of rebasing over merging.
Sometimes when I submit a PR and I get a lot of feedback I get lost making sure I've addressed every comment. My #1 feature request would be being able to mark comment threads as resolved. The outdated comment feature sometimes works for this use case but mostly it doesn't.
I didn't see it mentioned but it looks like you can now 'react' to comments, a la facebook. So you could choose one reaction, say, "Horray!", and use it as a marker that you resolved it.
Now I wonder if the commenter is notified on every reaction. The only thing that might make this more difficult.
That would also be missing a filter to show all unresolved comments, which is a huge use case. (Or maybe you can do that, if you can it is not very quick and intuitive).
I can no longer make comments on a given commit - the entry box at the bottom is gone. Looks like you have to scroll all the way back up to the top, switch to the "Conversation" tab, and then make a PR-level comment instead of a commit-level comment.
I hope I'm missing something here, because as it stands now it's a big step backwards.
They removed full-commit comments on PRs, as opposed to line-level comments on either commits or diffs, or full-commit comments on non-PR commits, but I don't think anyone used those—I haven't even seen anyone use something that wasn't a line comment for a long, long time.
Line-level comments trigger an absurd number of notifications, and there's no way to batch them. We abandoned that idea within a day.
PR-level comments lack any sort of context - there's no code at all unless you manually link it.
Commit-level comments were a sane, reasonable compromise, and now they're gone.
Your own workflow is obviously going to be different than mine, and it's good that you aren't affected by this, but hopefully you can relate to my disappointment with the way they've handled this. How would you feel if a product you were paying for suddenly dropped support for a feature you rely on without any notice or explanation?
It is very useful in work cultures where people don't do PRs all the time (particularly out of laziness or to do hotfixes), but you still want to comment on the change.
We do PRs for all changes, but strongly prefer to have comments about a particular commit within a PR be attached to the commit, and not just to the PR.
With this change, if we have comments about a given commit in a PR, we will now have to manually add a link to that commit in the comment so the developer knows which commit the comment is referring to.
It wouldn't be as much of an issue if PR-level code review was easier. As it stands now, the multi-commit view they just added (which is a step in the right direction) is still useless to us because it includes merges commits as well, which is not that developer's code.
So PR-level code review is still a non-starter, and commit-level code review just got clunkier.
I get that they can't support every possible workflow, but they could have at least given us a warning they were going to pull support for a feature we use (they do have those metrics) or at very least acknowledge it somewhere.
As a workaround you could just attach the comment to the very first or last line in the commit diff. Of course, this won't work for commits that consist entirely of non-diffable binary files, but it should work ok in most cases.
I'll plug my work as always when talking about the topic:
I wrote a small script for Chrome/Firefox that I found useful for reviewing big PRs on GitHub. It gives you possibility to expand/collapse files, mark files ok/fail in order come back to them later.
It works with mouse and keyboard (though I've noticed there are some issues with tab order after GitHub upgraded their code, and small UI glitches, I'll try to have a look at them soon)
It's a hack on top of GitHub so it needs maintenance every couple of months, but overally it does its job well IMO.
I don't have much time to hack on it anymore, but community contribs are very welcome. I wrote some potential ideas for improvements as GH issues in the repo. AMA if you're interested.
These are some nice changes, though there's still plenty of things I want GitHub to make better about code reviewing.
One of these changes, and one that annoys me every single day, is when I get an email about a comment, the email doesn't include any context (e.g. it should include the previous comments on that line and probably the hunk as well). And when I click "View on GitHub" to see it in context, if the commit is now on an outdated diff, I get taken to a page that doesn't show the comment at all. It takes me to the Files view, but the Files view doesn't show outdated comments. If the comment is on an outdated diff then it really should take me to the Conversation view with the comment in question expanded.
If next to every file, Github also listed its filesize, I would know exactly what file to begin looking at when I encountered a new repo (to a good approximation -- in any case, I would be able to intuit better decisions with a combination of filenames, the information in hypothetical README.md's, and filesizes than just READMEs and filenames alone..)
Maybe this doesn't scale or something? It's something I've felt necessary for a while though. Maybe their user testing has concluded otherwise?
I wish it were possible to put comments directly on a line of code in any commit / repo outside of a PR. Sometimes someone wants me to review their code that they aren't submitting anywhere.
My biggest pet-peeve with GH code review is that line-comments are automatically folded whenever that line of code is changed. So if the changes to that line didn't relate to your CR or if they didn't actually fix anything, then your comment will pretty much be lost to time.
Plus, it would be nice to see the discussion around a particular line without having to go through the entire PR and unfolding each conversation to see if it's the right one.
Then give Reviewable (https://reviewable.io) a try. :) Comments remain open until acknowledged / resolved and are automatically displayed at the nearest applicable line in every diff.
I can't say I'm a fan. If I select a single commit, instead of giving me a list with a single commit check-marked, it gives me only that single commit and an option to 'show all commits'. So to change the commit you are viewing requires clicking 'show all commits' (waiting for them to load) and then selecting the other commit you want to view (which should just be a check box).
Also it totally baffles me that these actions all require server-side requests. It's really a lot easier to go to the old commits tab, and to ctrl click an open tab for each commit then to use this new feature.
At one point Github offered the best and cleanest UI of all the alternatives - but I doubt this will be the case for long at this rate.
Code review is something all of us do, but all of us do differently. Anyone know of any nice frameworks, articles, or blog posts for code review? I'm particularly interested in the case where knowledge transfer is a high priority in the code review.
My current side project integrates feedback theory [1], to provide scaffolds and other cues to help and remind reviewers to give high quality feedback. Thus, my interest.
For knowledge transfer, my number one technique is pair programming. If there are more than two people involved, I add frequent pair rotation.
The only time I've ever felt comfortable going on vacation is on pair programming teams. That's a sign to me that knowledge really has been properly transferred.
I wrote this high-level overview of how my team does code review, with an emphasis on knowledge transfer: http://engineering.zesty.com/code_review/. Hope you enjoy.
Nice additions. Can't help but feel at some point you'll be able to edit and compile code directly on Github (with some sort of compiler/CL backend). Has Github ever discussed integrating Atom directly into the site?
Only related to review in a somewhat indirect way, but I really wish the commit view on github (as well as other git tools) had an equivalent of --left-only command line option to git-log. It's incredibly useful for viewing a high level of the history of a repo that uses merge bubbles, and lack of tooling around doing just that seems like that main reason people do things like squash their branches before merging to master (which is where I think it connects back to review).
Awesome to see GitHub stepping to the plate with these updates. IMO the biggest thing missing in the PR's diff view is a git blame column beside the changes, so if someone is writing good commits I can glance through the entire diff and see which changes were introduced together and why.
Just a feature request, and I know how annoying those can be on the receiving end. Great updates either way.
I've been pleasantly surprised at how fast Github has been moving since that critical open letter came out. For a giant company like Github they've been releasing developer tools very quickly.
I'd like block-level comments and maybe even an in-commit commit function (the ability to commit to a PR branch while in the PR). That's all I really care about.
1) piling new commits onto the existing branch/PR, or
2) force-pushing and completely losing the old commits on the server
You could instead push into a "magic" ref-spec and the server would retain the original commits, but also the rewritten commits, such that a PR can have multiple revisions. This is somewhat hard to explain unless you're familiar with Gerrit and its "patch set" workflow.
Why? Because my preference is to rewrite history in order to address code review comments, such that what is finally merged leaves a clean history. But it is also valuable during the code review process to be able to look across the revisions to make sure everything has been properly addressed.
The only way to do both, today, is to create a new branch and PR for each "revision". i.e. new-feature, new-feature-rev1, new-feature-rev2. Then close the original PR and reference it from the new PR. A bit tedious.