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