Hacker News new | ask | show | jobs
by u801e 988 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.
1 comments
akoboldfrying 988 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 988 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