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

81–90 of 173 posts

Re: Git-appraise – Distributed Code Review for Git

#81

Post by nurbl »

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…

We did that at a previous job too. I also thought it seemed a bit weird at the beginning (too simple?), but I loved it. Being able to do reviews from the comfort of magit instead of having to deal with a janky web GUI was great.

As long as there's an agreed upon comment format, it's really easy for the reviewee to find all the comment and adress them one by one.

I don't remember any drawbacks really, but we were a small team and the reviews were mostly "one on one".

Re: Git-appraise – Distributed Code Review for Git

#82

Post by worldsayshi »

palata wrote: post #61
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…

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…

Don't you lose a bit of important information when you create a patch? Like which commit it started from.

Re: Git-appraise – Distributed Code Review for Git

#83

Post by palata »

withinboredom wrote: post #75
palata wrote: post #67

Earlier quoted context omitted.

> you share the review by pushing to the branch. How do you deal with conflicts between multiple reviewers?

The same way you deal with any other conflicts, you won't be able to push without pulling first.

Which I can imagine could be annoying in some situations :-). But no workflow is perfect.

I think I like the idea that in an e-mail, I can really comment on some patch, and then the author of the patch can decide what to do with it. Maybe the author modified the patch in the meantime, but they will know if my comment is still valid or not. Without me having to handle the conflicts.

I'm thinking mostly about leads who have a lot to review. They probably don't want to have to fix conflicts in comments all the time.

Re: Git-appraise – Distributed Code Review for Git

#84

Post by palata »

worldsayshi wrote: post #82
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…

Don't you lose a bit of important information when you create a patch? Like which commit it started from.

You don't send a diff, you send an e-mail formatted by git. So you get all the commits you put in it, it's really just like seeing the commits in a GitHub PR.

So the reviewer receives the commits, with their descriptions, authors, everything. Just like pulling a branch, really.

Re: Git-appraise – Distributed Code Review for Git

#85

Post by okl »

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…

I think your approach to code review is in general a good idea but don't underestimate the differences in skill level in the average project. I often work with domain experts who are not developers or beginners who are still learning basic git concepts and how to navigate the code base.

Re: Git-appraise – Distributed Code Review for Git

#86

Post by palata »

jauco wrote: post #80
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…

You know, I never really tried that. But I will do so when the opportunity arrives!

It is really easier than it looks! Took me a while to try it, but then I realized it's really super easy: run the git command to create the e-mail, send the e-mail, and on the reviewer side just run the command to apply it.

This is a nice tutorial, too, but I would recommend reading Drew Devault's posts first (and watching his short videos):

https://git-send-email.io/

Re: Git-appraise – Distributed Code Review for Git

#87

Post by chriswarbo »

epolanski wrote: post #79
mike_hearn wrote: post #76

Earlier quoted context omitted.

A couple of ways to mitigate that: 1. `git log --first-parent --pretty=oneline` will show you merge commits but not the commits that were merged. If your merge commits have non-default messages, this effectively makes for a "flat" log in which CR commits don't appear. Unfortunately not all git GUIs support this (e.g. github). 2. If a comment genuinely isn't actionable with a code change take the discussion to another…

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 writing a whole explanation, and (b) gives better evidence of what's going on (e.g. that the CTO explicitly asked for this-or-that; or that the SO question now has a better answer; etc.)

Re: Git-appraise – Distributed Code Review for Git

#88

Post by chriswarbo »

Etheryte wrote: post #63
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…

The core problem with this workflow is that it's rigid around doing things The One True Way. The fact that you have a specific comment delimiter is a good example of that. I feel like this doesn't really have any upsides compared to, say, reviewing code on Github. If you want you can always check out the branch and inspect it locally, but you also have the option of doing it all on the web without stashing your chang…

> without stashing your changes

You can do reviews in a separate clone, to avoid interrupting any changes you're making (reading the diff only needs a 'git fetch', so won't interfere with anything; but making changes is easiest with a full check-out).

You could use 'git worktree' to manage that; but I'm happy with 'git clone' and 'rm -r'.

Re: Git-appraise – Distributed Code Review for Git

#89

Post by palata »

goku12 wrote: post #43
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.)?

> What's wrong with patch email chains? The fact that it's email. There are two categories of problems with it: 1. It's been extended unnecessarily and implemented poorly. The biggest one in this category is HTML email. It's really hard to write mails using common clients without messing up mailing lists. Another problem is how badly email threading is displayed in these clients. Email UI is still abysmal. 2. Many fe…

> Another problem is how badly email threading is displayed in these clients. Email UI is still abysmal.

Fair point. However, given that the current alternative is "use another service entirely (e.g. GitHub)", I think it would be fair to assume that devs could choose a good e-mail client and learn how to format such e-mails correctly. It works for Linux, for instance. I started using Aerc, and I love it:

https://aerc-mail.org/

> Many features of emails are an afterthought.

I agree that e-mail is not perfect, but... how is GitHub better? With e-mail, you can send your patch encrypted (e.g. with PGP) if you feel the need, but otherwise it will go over HTTPS (same as GitHub). I honestly don't really know about attachments (I guess you would encrypt them with PGP as well?), but again, attachments on GitHub are not E2EE...

I tend to believe that people use GitHub for different reasons, for instance:

1. It is the first thing they see when they join the industry

2. Cargo-cult, a little bit

3. Devs like new shiny toys, and e-mails are old technology ("we should have something better than e-mail in 2023", which I disagree with).

4. Nobody takes the time to try the e-mail workflow (even though it's really two git commands). Actually it is common to not really take the time to learn the basic tools, I feel.

Re: Git-appraise – Distributed Code Review for Git

#90

Post by palata »

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

> Grain of salt: I've become a curmudgeon w.r.t software complexity over the last decade

Same here :-). I have come to think that old and simple technology was nice, and the trend nowadays is to pile up a lot of complexity for no apparent reason.

> What's wrong with patch email chains?

I genuinely believe that most developers (who I believe have less than 5 years experience) have never seen the e-mail workflow in their life. And there is a tendency to believe that newer is better, so they don't even consider e-mail.

Post reply on HN