More code review tools(github.com) |
More code review tools(github.com) |
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.
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
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 c->a->b->masterBy 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!
Example in action: https://github.com/grpc/grpc-go/pull/570
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`.
I don't always want to see my diff in alphabetical order.
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.
http://fle.github.io/git-tip-keep-your-branch-clean-with-fix...
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.
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.
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.
Disclosure: I built the tool.
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.
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.
Now I wonder if the commenter is notified on every reaction. The only thing that might make this more difficult.
I don't think the commenter is notified about reactions to their comments.
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.
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?
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.
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.
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?
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.
Disclosure: I built the tool.
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.
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.
1: https://scholar.google.com/scholar?q=%22The+Effects+of+Feedb...
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.
Just a feature request, and I know how annoying those can be on the receiving end. Great updates either way.
(Source: I worked on code review at GitHub for awhile!)
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.
This is pretty common among companies that make the consumer->enterprise transition. Also see Dropbox, Slack, etc.
Certainly not as easy as seeing them in a nice row for review but it's a workaround at least.
Disclosure: I built the tool. :)
Presumably a codebase in an array-oriented language would have miniscule PRs, which is irrelevant (discussion of PL merits aside).
Compare two views of the same commit (found this by searching for any commit on github with the word "indent" in it)
https://github.com/douglascrockford/JSON-js/commit/1e3869cb3... (133 additions and 127 deletions)
https://github.com/douglascrockford/JSON-js/commit/1e3869cb3... (30 additions and 24 deletions)
At present, the "official" collaboration tools in the GitHub integrations directory are here: https://github.com/integrations/feature/collaborate
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.
- [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.
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.
I would love to have more clarity on release schedules for enterprise in general
In our project we exclusively use comments on the Files tab precisely for that reason.
The difference between commenting on the commit page and on the "files tab" is pretty subtle though...
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.
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.
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:
http://gitsense.github.io/github+gitsense.html
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.
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.
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.
"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...
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.
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. :)