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

111–120 of 173 posts

Re: Git-appraise – Distributed Code Review for Git

#111

Post by palata »

goku12 wrote: post #102
palata wrote: post #89

Earlier quoted context omitted.

> 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…

> I agree that e-mail is not perfect, but... how is GitHub better? Please look at my comment again. I prefer email to locked in forges. > Devs like new shiny toys, and e-mails are old technology There is one aspect where such forges have an advantage over email - a better user experience. Aerc and the likes all good - but Github and others provide a good user experience over a tool that everyone uses - the web browse…

> We really should have something better than email.

Maybe, but e-mail is here, the whole world knows and uses it, and even if it is not perfect, I think it does the job pretty well (put aside the fact that managing a server is hard).

> However, this is an additional barrier to entry.

Agreed. I just feel like we keep pushing the barrier down, but at some point, professionals need to learn the basics. I don't feel like we need more software in our world. We need better software. And if people can't learn their tools, I don't know if we can seriously expect them to write good software...

Re: Git-appraise – Distributed Code Review for Git

#112

Post by bstpierre »

Maybe it happens a lot and nobody has mentioned it here yet, but I find live, out-of-band reviews are often very effective. By this I mean looking at a patch and asking questions or raising issues with the author. I do this fairly regularly for PRs at work where the “live” aspect is just slack (I work remotely).

A dozen or so years ago I worked at a startup using svn and all reviews were like this: generate a diff, post a link to the diff on jabber, someone will comment/question. There’s room for immediate back and forth as needed. Revise, approve, done. Very lightweight, yet effective. However we were all colocated and all of the team members were high functioning so I’m not sure if it generalizes. (Also, being colocated meant that occasionally the jabber conversation could move to f2f with whiteboards if things got complicated.)

Re: Git-appraise – Distributed Code Review for Git

#113

Post by thfuran »

cdogl wrote: post #109
chriswarbo wrote: post #87

Earlier quoted context omitted.

> 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…

I prefer clean code that doesn’t have comments except as doc strings for various parsers on declarations and in areas that are particularly surprising. If I want to know why something was done, I expect to be able to use git log or git blame on the file or line and get find a commit message, issue tracker link or pull request reference that I can pursue.

But then a single minor refactor means you have to dig through history after puzzling over something for long enough to decide that it's definitely no obvious instead of just seeing an inline explanation (or at least a link to one).

Re: Git-appraise – Distributed Code Review for Git

#114

Post by mike_hearn »

brandall10 wrote: post #97
mike_hearn wrote: post #68

Earlier quoted context omitted.

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…

"it's quicker to just fix the thing you want changed than ask someone else to do it" Sure, but I feel like there's a big disadvantage here in that the person who did the bad thing isn't actively involved in fixing it. They lose out on acquiring that muscle memory. If this is a Sr. reviewing another Sr.'s PR and it's just a few minor oversights, fine. But if this is a Sr. reviewing a Jr.'s PR and they fix like 10 thin…

You can certainly pick which approach to use based on your wider goals, sure. Sometimes though changes are a matter of style, or something is easier to show rather than tell, or you realize whilst reviewing the requirements weren't quite right (on you) etc.

For example, if a junior isn't familiar enough with the standard library and misses a trick, you can try to explain in words what the better way is, or you can just fix it and then ask them to study the diff. I don't know what's better for long term learning, but at least you have the option.

Re: Git-appraise – Distributed Code Review for Git

#115

Post by xvilka »

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…

The biggest drawback is the inability to use on a computer without email setup, e.g. not from the main computer, or from mobile devices - smartphone, tablet. In my case it's not uncommon to do a quick review of some PRs using this Web-based workflow. Now with newer GitHub application it's even easier.

Re: Git-appraise – Distributed Code Review for Git

#116

Post by xvilka »

d4rkp4ttern wrote: post #110
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…

Along the same lines: when I want to locally review someone’s PR, I want to see their changes as “dirty” relative to main, so that the changes show up nicely highlighted in PyCharm in the “gutter” (left side). This has to be such a common need, but I am surprised that I have to do convoluted things like: create a new branch, switch to it, and then: git merge --no-commit --no-ff pr-branch Am I missing an easier workfl…

I recommend trying b4[1] tool.

[1] https://b4.docs.kernel.org/en/latest/index.html

Re: Git-appraise – Distributed Code Review for Git

#117

Post by indymike »

cdogl wrote: post #109
chriswarbo wrote: post #87

Earlier quoted context omitted.

> 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…

I prefer clean code that doesn’t have comments except as doc strings for various parsers on declarations and in areas that are particularly surprising. If I want to know why something was done, I expect to be able to use git log or git blame on the file or line and get find a commit message, issue tracker link or pull request reference that I can pursue.

Issue trackers, vcs and employees change. A meaningful comment stay as long as it is meaningful.

Re: Git-appraise – Distributed Code Review for Git

#118

Post by goku12 »

palata wrote: post #111
goku12 wrote: post #102

Earlier quoted context omitted.

> I agree that e-mail is not perfect, but... how is GitHub better? Please look at my comment again. I prefer email to locked in forges. > Devs like new shiny toys, and e-mails are old technology There is one aspect where such forges have an advantage over email - a better user experience. Aerc and the likes all good - but Github and others provide a good user experience over a tool that everyone uses - the web browse…

> We really should have something better than email. Maybe, but e-mail is here, the whole world knows and uses it, and even if it is not perfect, I think it does the job pretty well (put aside the fact that managing a server is hard). > However, this is an additional barrier to entry. Agreed. I just feel like we keep pushing the barrier down, but at some point, professionals need to learn the basics. I don't feel lik…

> And if people can't learn their tools, I don't know if we can seriously expect them to write good software...

Last time I suggested [1] an approach to learning and taming the git CLI, somebody said that they want to design and write code, rather than learn too much CLI-fu. Big projects like Linux and Emacs can get away with requiring their contributors to learn the tools well enough. However, many developers may decide to avoid contributing rather than learn the tool, if the project is a small one.

Again, I agree with what you say - that's what I practice too.

[1] https://news.ycombinator.com/item?id=37014286

Re: Git-appraise – Distributed Code Review for Git

#119

Post by Corsome »

goku12 wrote: post #106
palata wrote: post #84

Earlier quoted context omitted.

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.

You do lose the information about where to apply the patch. This isn't a big problem for rebase-friendly crowd. But people who prefer the merge strategy strictly may not like it.

AFAICS there's `--base=auto` which captures explicit parent's sha in the patch text output (there's also a git config option to do that automatically).

Source: https://git-scm.com/docs/git-format-patch#_base_tree_informa...

Post reply on HN