Comment by joshpeek | Hacker News Reader

Comment by joshpeek | Hacker News Reader

Parent Full thread

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!

Seriously -- disappearing comments is very sad.

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

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.

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

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

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

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

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

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.

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

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

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.

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

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.

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

Also interested.

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

git really needs a `branch history` feature.

Reply on news.ycombinator.com