← Back to context

Comment by ivanjermakov

3 days ago

I find the idea of using git for code reviews directly quite compelling. Working with the change locally as you were the one who made it is very convenient, considering the clunky read-only web UI.

I didn't get why stick with the requirement that review is a single commit? To keep git-review implementation simple?

I wonder if approach where every reviewer commits their comments/fixes to the PR branch directly would work as well as I think it would. One might not even need any additional tools to make it convenient to work with. This idea seems like a hybrid of traditional github flow and a way Linux development is organized via mailing lists and patches.

5 comments

ivanjermakov

Reply
spike021  3 days ago

is github's PR considered read-only?

i've had team members edit a correction as a "suggestion" comment and i can approve it to be added as a commit on my branch.

  • ivanjermakov  3 days ago

    By read-only I meant that you can't fully interact with the code: run/debug it, use intellisense, etc.

teiferer  3 days ago

> I didn't get why stick with the requirement that review is a single commit

Yeah that is pretty weird. If 5 people review my code, do they all mangle the same review commit? We don't do that with code either, feels like it's defeating the point.

Review would need to be commits on top of the reviewed commit. If there are 5 reviews of the same commit, then they all branch out from that commit. And to address them, there is another commit which also lives besides them. Each commit change process becomes a branch with stacked commits beinf branches chained on top of one another. Each of the commits in those chained branches then has comment commits attached. Those comment commits could even form chains if a discussion is happening. Then when everybody is happy, each branch gets squashed into a single commit and those then get rebased on the main branch.

You likely want to make new commits for that though to preserve the discussions for a while. And that's the crux: That data lives outside the main branch, but needs to live somewhere.