[js2]

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.

[joshpeek]

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

[gmartres]

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

[cdcarter]

Seriously -- disappearing comments is very sad.

[scoot]

Disappearing comments mean they've been adressed. And they don't actually disappear, they're just minimised.

[Elv13]

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

[wanderr]

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

[steveklabnik]

Yes, this is my largest pain points with PRs: I'd like to be able to collapse comments that were resolved, but weren't made on a line that changed.

[anarazel]

Fwiw, you can ease the pain by doing git rebase --ignore-date

[bradfitz]

Btw, I've been gluing Github PRs and Gerrit together with https://github.com/LetsUseGerrit

Example in action: https://github.com/grpc/grpc-go/pull/570

[js2]

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.

[igor47]

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.

[eridius]

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

[tokenizerrr]

I'd also like to voice my interest in force push support. The workflow with Gerrit is very pleasant.

[Evan-Purkhiser]

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.

[viraptor]

Another YES! Gerrit does things right and I'd love GH to support that idea. (lived in gerrit for the last few years)

[fanf2]

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.

https://github.com/fanf2/git-repub

[bravura]

I'd love simply to re-order the files in the PR so they were in the most logical order to review them.

I don't always want to see my diff in alphabetical order.

[scoot]

It's your workflow that's broken. Add commits, then squash before merge when the review is done.

[gmartres]

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.

[zachberger]

Do you know when these changes will come to GitHub enterprise?

[merlinsbrain]

Also interested.

I would love to have more clarity on release schedules for enterprise in general

[chei0aiV]

git really needs a `branch history` feature.

[kderbe]

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.

[wmij]

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.

[Myrmornis]

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?

[wmij]

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.

[js2]

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.

[deevus]

I didn't know about this. For those that are looking:

http://fle.github.io/git-tip-keep-your-branch-clean-with-fix...

[frutiger]

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.

[js2]

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.

[PuercoPop]

A middle ground could be to add the change Id as a git note? But that would probably result in tooling needed to get a good UX flow.

[cmsj]

+1 came here to talk about Gerrit too.

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.

[vvanders]

Yup, of all the git based CR tools I've used Gerrit is by far and away the best.

[chinhodado]

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?

[ehnto]

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.

[jordigh]

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.

[piotrkaminski]

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.

Disclosure: I built the tool.

[sytse]

We're thinking about adding this feature in GitLab in https://gitlab.com/gitlab-org/gitlab-ce/issues/13524

[apinstein]

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.

[nightpool]

isn't this what the "view outdated diff" button does, or am I misunderstanding?

[js2]

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.

[sjrd]

Only comments on the commits are lost. Comments on the Files tab of the PR itself are kept and you can "View outdated diff".

In our project we exclusively use comments on the Files tab precisely for that reason.

[EdiX]

Well, I guess this explains why sometimes I saw comments on PRs disappear and sometimes they didn't, thanks.

The difference between commenting on the commit page and on the "files tab" is pretty subtle though...

[kragniz]

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.

[marcinkuzminski]

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.

[ngrilly]

Exactly what I want. I basically abandoned GitHub's pull requests because of that.

[gfunk911]

I didn't know about this and I love it.