Hacker News new | ask | show | jobs
by akoboldfrying 998 days ago
It's surprising to me that GitHub currently doesn't attach review comments to specific commits, but only to timestamps, and I agree that it would be great to improve that.

But I don't understand how a contributor can feel pressure to address all of the reviewers' comments in a single commit: They can commit as many times as they want, and only push when they feel it's ready. And in the case where they may want to offer multiple different solutions for a reviewer to choose from, I like a small adaptation of a suggestion I saw by another commenter here, which is to make a PR for each option off their existing PR's branch: When one of those option branches is accepted and merged, GitHub will fast-forward the original PR's branch to include those commits, which is exactly what you would want and expect.
2 comments
ezekg 998 days ago
I think this is what https://graphite.dev is trying to do.
link
u801e 998 days ago
> It's surprising to me that GitHub currently doesn't attach review comments to specific commits

It is possible to comment on commits in github (you can do it by clicking on the sha1 of the commit and then making a comment on a line in the diff). But the comment won't show up in the main PR diff window.

> But I don't understand how a contributor can feel pressure to address all of the reviewers' comments in a single commit: They can commit as many times as they want, and only push when they feel it's ready.

Unfortunately, this leads to a lot of fixup commits in the branch that muddle up the history. A changeset consists of one or more commits where each makes one logical change where the what was done and why it was done that way are detailed in the commit message.

link
akoboldfrying 998 days ago
>A changeset consists of one or more commits where each makes one logical change

I don't yet see how that is different from just... a sequence of commits, which you can do now. (If you want to claim that you could quickly make a bunch of messy local commits and afterwards reorganise them into a more logical group of commits in a changeset -- you can already do that, without any new concept of changesets, by using `git rebase -i`.)

link
u801e 997 days ago
> I don't yet see how that is different from just... a sequence of commits

The difference is a sequence of commits that implement a feature where each commit basically is a logical change that does a single thing (e.g., add a function and associated test, add calls to a new function, etc) versus a sequence of commits with additional commits that fix issues brought up during review. Those additional commits really should be amended to the corresponding original commit rather than being a completely separate commit.

Another way to look at it is submitting an assignment where you have one page per problem solution as opposed to submitting that and then several more pages at the end containing fixes to those problem solutions rather than incorporating those fixes in the original set of pages.

link