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