Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Git-appraise – Distributed Code Review for Git

github.com

Post reply on HN
Print view View original

131–140 of 173 posts

Re: Git-appraise – Distributed Code Review for Git

#131

Post by rvdginste »

mike_hearn wrote: post #68
jauco wrote: post #44

I have this "one weird trick" that I think is great, but somehow no-one else wants to use: At one point I worked with a fellow dev where we did PR reviews just as commits on top of the branch. So you'd check out their feature branch, run the equivalent of 'compare working tree with ' for your editor and then look through the changes. Because you're comparing the working tree you can leave comments... by typing source…

You're not crazy and not alone. At Hydraulic we do the exact same thing. It has other advantages you didn't mention: • It yields much more collaborative code reviews, because for small tweaks it's quicker to just fix the thing you want changed than ask someone else to do it. After all, you've already got the code checked out and in your IDE. This is especially true when working with easily refactorable languages like…

> Socially this arrangement is kernel-like. The tech lead of the project runs the repo from which releases are done and merge in from teammates, which in turn can merge in from their teammates and so on. Ownership is always clear because the act of merging is also the act of taking responsibility. You can't get a tragedy of the commons where juniors keep picking other juniors to review their work like you can in more conventional free-for-alls.

I think the way you work is very interesting. We once discussed about having a process that is more kernel-like.

The fact that everyone has their own remote clone of the (or all) git repositories, does that not introduce overhead? We did that long time ago when we started with git, but thought it introduced extra overhead without any benefit. I assume you do it to make the ownership clear? And I also assume that everyone in your team is very comfortable with git? (yes, I do think developers should know their tools inside-out, but that is sadly rarely the case)

Can you explain in more detail the steps for a code review process? Is it the author that creates and deletes the review branch in the clone of the reviewer? How does the author know that the reviewer finished the review? Is it always the author who pushes his code to the technical lead for merge into master?

Re: Git-appraise – Distributed Code Review for Git

#132

Post by palata »

darau1 wrote: post #125
palata wrote: post #61

Earlier quoted context omitted.

Have you ever tried the git e-mail workflow? I think you may like it, I'd be curious to have your opinion about how it compares to your workflow above :-). The idea is that you just send your patch by e-mail, and the reviewer just applies it with `git am `. So you don't even have to sync on a branch. Then the reviewer can comment directly on the patch. You answer with new versions of the patch, and get new reviews. I…

It always amazes me that these large companies insist on reinventing the wheel.

Because their wheel comes with lock-in :-)

Re: Git-appraise – Distributed Code Review for Git

#134

Post by avgcorrection »

chriswarbo wrote: post #87
epolanski wrote: post #79

Earlier quoted context omitted.

This is a very interesting point you make and I fully agree with: If a comment genuinely isn't actionable with a code change. In the case it is or it really needs an explanation it stays in the codebase, otherwise you take it, e.g. to GitHub issues or PR comments.

> In the case it is or it really needs an explanation it stays in the codebase, otherwise you take it, e.g. to GitHub issues or PR comments. Another thing that's easy, but often overlooked: putting relevant URLs in comments. The best comments explain why some approach was taken; linking to a discussion on JIRA, or the Stack Overflow answer we c̶o̶p̶y̶-̶p̶a̶s̶t̶e̶d̶ ̶f̶r̶o̶m̶ used as inspiration, is (a) quicker than w…

A lot of commits in Linux contain links to where they were discussed (Kernel Lore archive or something).

Re: Git-appraise – Distributed Code Review for Git

#135

Post by killthebuddha »

jauco wrote: post #44

I have this "one weird trick" that I think is great, but somehow no-one else wants to use: At one point I worked with a fellow dev where we did PR reviews just as commits on top of the branch. So you'd check out their feature branch, run the equivalent of 'compare working tree with ' for your editor and then look through the changes. Because you're comparing the working tree you can leave comments... by typing source…

A similar approach I've been using recently and am so far really, really enjoying is to put all "conversation" into a dedicated "devlog" directory in the project root. We put everything in it: meeting notes, project status updates, memos, changelogs for every pull request, reviews, etc. Every file is markdown and named with a date prefix, a "nonce" for uniqueness, and a category (date.nonce.category.md). So you might have something like:

  - 2023-08-10.00.notes.md
  - 2023-08-10.01.notes.md
  - 2023-08-11.00.changelog.md
  - 2023-08-11.01.changelog.md
  - 2023-08-11.00.review.md
  - 2023-08-11.01.review.md
  - 2023-08-11.02.review.md
  - 2023-08-11.00.project.md
The file naming includes a small amount of ambiguity and a small opportunity for merge conflicts but in practice it's just not a problem. The best part of this workflow IMO is that it doesn't create a whole new workflow for communication, it treats high-quality async communication as part of the core (pull, read, commit, push, review) development workflow.

Re: Git-appraise – Distributed Code Review for Git

#136

Post by avgcorrection »

jauco wrote: post #53
F-W-M wrote: post #49

Earlier quoted context omitted.

If they are in there own commits, you just drop them. Honestly the average level of git skills, doesn't allow such a workflow.

No, no git magic. Just source code changes. One commit with the whole review that contains fixes and maybe a few questions. Then a commit from the PR author on top of that that adds explanations and/or fixes for the questions. Maybe the key required for this is that you have reviews where, if code prompts a question, then more often then not the code should change. At the very least to explain briefly why this approa…

> No, no git magic. Just source code changes. One commit with the whole review that contains fixes and maybe a few questions. Then a commit from the PR author on top of that that adds explanations and/or fixes for the questions.

Reviews on some projects will ask you to make a commit/patch in between your third and fourth (out of seven). And to fix a typo in the fifth commit message. And do fix something in the second commit that leads to conflicts in the next commits. And then to end up with for example seven commits that each “do one thing” or whatever. So if the expectation is that you end up with one “fixed up“ commit at the end of the review process which encapsulates the whole PR then this won't work.

With your approach I imagine going in several rounds where

1. Reviewers leave comments

2. I address them

3. I publish that as review round X

4. I “clean up“ the history (get rid of comments/all review history) and address things like “make a doc commit before commit number 4”

5. Publish that as the “blank slate” (no comments) for the next round

6. Invite the reviewers to the next round

And that could work.

Something more dynamic/less rigorous (with rounds) and you might end up with an excessive amount of history rewriting and conflict resolution.

Re: Git-appraise – Distributed Code Review for Git

#137

Post by OkayPhysicist »

Cthulhu_ wrote: post #59
jauco wrote: post #44

I have this "one weird trick" that I think is great, but somehow no-one else wants to use: At one point I worked with a fellow dev where we did PR reviews just as commits on top of the branch. So you'd check out their feature branch, run the equivalent of 'compare working tree with ' for your editor and then look through the changes. Because you're comparing the working tree you can leave comments... by typing source…

I'd say the biggest drawback of that approach is churn; loads of extra commits and code changes for code review comments. Of course, that becomes a non-issue if you use a squash merge approach, instead of retain all commits (which in this approach have a lot more of a "WIP" feel to it). Do code review comments have value after merging? I would also posit they're easier to find if they're in an external location (like…

I've never gotten the point of squash-merge. The normal "merge into release branch, never fast-forward" workflow retains more useful information. Every release commit has 2 parents: the previous release commit, and the former head of the feature branch it got its changes from. If you want a chain of releases, you go down the path of commits labeled "RELEASE". If you want information on the changes, go down the other path.

I regularly use "git blame" on lines of code to try and figure out what the specific reasoning was on a line of code. I'm going to have a very different interpretation depending on if the resulting commit says "implement feature xyz" vs "fix issue abc on feature xyz".

Squash murders that added context. At best you're optimizing for the wrong thing, and worst it's outright vandalism.

Re: Git-appraise – Distributed Code Review for Git

#138

Post by OkayPhysicist »

globular-toast wrote: post #98
Cthulhu_ wrote: post #59

Earlier quoted context omitted.

I'd say the biggest drawback of that approach is churn; loads of extra commits and code changes for code review comments. Of course, that becomes a non-issue if you use a squash merge approach, instead of retain all commits (which in this approach have a lot more of a "WIP" feel to it). Do code review comments have value after merging? I would also posit they're easier to find if they're in an external location (like…

I assumed the code review commits would be squashed before the merge. But you raise a valid point about the code review process being lost. One way to get around that is to keep the feature branches (or maybe just tags) forever and do all the squashing on the integration branch. This does require some kind of convention that records on which branch/tag the code review for a particular feature can be found. The advant…

Or just don't squash. There's no need. In fact, if you're doing a review process like this, it would be active vandalism to to squash, since there is a big difference between "code said x, commented on by reviewer, changed to y" vs "code says y".

Re: Git-appraise – Distributed Code Review for Git

#139

Post by doctorpangloss »

jauco wrote: post #44

I have this "one weird trick" that I think is great, but somehow no-one else wants to use: At one point I worked with a fellow dev where we did PR reviews just as commits on top of the branch. So you'd check out their feature branch, run the equivalent of 'compare working tree with ' for your editor and then look through the changes. Because you're comparing the working tree you can leave comments... by typing source…

> You can fix typo's just by, well, fixing the typo... You're more inclined to engage with the code rather then just reading it...

This is all the value, isn't it?

Anything that forces the "reviewer" to become an "editor" suddenly massively increases the value of the "review."

People like the GitHub workflow because... then they don't have to do that work.

This is fine. I'm not sure how worse developers become better ones, except by fixing their mistakes. So if you're a huge corporation and you have the luxury to have a lot of redundant, in-training employees, many fresh out of school with no useful experience, great, reviews by senior people, so long as the junior person's code isn't actually written for them, can transmit some training value.

> However, people tend to look at me like I'm crazy for suggesting something like this

If you're a tiny startup and you have a junior developer, you've sort of already screwed up haven't you? Either you want to train them, and that's a huge mistake because you don't have the bandwidth for that. Or you do their work for them, which is like, why have them around at all then?

Re: Git-appraise – Distributed Code Review for Git

#140

Post by qudat »

tuckerpo wrote: post #22

Grain of salt: I've become a curmudgeon w.r.t software complexity over the last decade, but git is already distributed. What's wrong with patch email chains? Or the current existing wrappers around git that allows for distributed review chains (i.e. git(hub|lab), etc.)?

I love the idea of using email to send patches, but the UX is not well suited for the business requirements.

For me, being able to edit the patch request without spamming everyone on the email list would be nice. Making changes to the patch request requires convention to link them together.

The patch requests are completely decoupled from the git repository so now you need something else to generate the mailing list.

When comparing a PR tab in GH to mailing list patch requests, it should be obvious which one is better at reading/searching/linking previous patches.

Post reply on HN