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

151–160 of 173 posts

Re: Git-appraise – Distributed Code Review for Git

#151

Post by mike_hearn »

rvdginste wrote: post #131
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…

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

There's no overhead because the forks are done serverside and hosted on the same machine. Git knows how to use hard links to rapidly do local clones, so disk space isn't wasted.

Yes, it makes ownership clear and prevents code from being merged without being reviewed. GitHub offers CODEOWNERS files but often ownership doesn't map neatly to source code layout (and nor should it).

Most devs have not been comfortable enough with git to do this when they first joined but they learned quickly enough, and I helped them learn. The operations needed aren't that complicated. For example you don't need to do rebases in this workflow. It's just branching and pushing.

I should write up a proper blog post on the workflow. The code review author creates the branch by pushing into the reviewer's repository. The reviewer deletes the branch once they merge it. The author knows the review was finished because the code either gets merged, or it gets another commit on top that adds requests for changes. The branch in the reviewer's repository is where collaboration happens once the review process starts.

Re: Git-appraise – Distributed Code Review for Git

#152

Post by mike_hearn »

brandall10 wrote: post #142
mike_hearn wrote: post #114

Earlier quoted context omitted.

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

You can do a "show don't tell" with a fix suggestion in a Github PR, which one accept as-is or can use for inspiration. I feel like in the majority of cases doing something for someone is a missed opportunity - esp. again in the situation where there are many. A lot of those aren't going to stick for most people if they're simply quickly reviewing it. It also seems like an opportunity for certain personality types to…

Yes GitHub is slowly reinventing IDEs in their code review tools. If you do it in the IDE with real commits you can use refactoring aids, search+replace etc.

If someone keeps making the same errors and isn't paying attention to the fixes you can just talk about it with them in a meeting, same as with any other recurring problem you might have with a teammate.

The model we implement is hierarchical, so there's no running roughshod over other people by design. If the reviewer isn't happy they won't merge, and you have to keep going until they are. But, it's also on the reviewer to get the work in because they're being held accountable for delivery too, and they can make changes with the same tools as the authors. So it's a collaboration.

Re: Git-appraise – Distributed Code Review for Git

#153

Post by IshKebab »

mike_hearn wrote: post #150
IshKebab wrote: post #146

Earlier quoted context omitted.

How do you request a review and ensure that branches are reviewed before merging?

To request a review you push the changes into a special branch namespace in the reviewer's repository. IntelliJ uses / as a way to organize branches into folders, so it's easy to see what review requests come from who. You can also inform them in other ways (CCing on a ticket, email, chat, etc). We use a kernel model in which there is no shared repository, so it's impossible for branches to be merged without being re…

How do you push changes to someone's repo? I think the kernel guys use email right? Sounds a bit painful.

Re: Git-appraise – Distributed Code Review for Git

#155

Post by viraptor »

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…

> but at some point, professionals need to learn the basics

Think about a junior joining a company. Through the first weeks they get: to be useful you need to learn about dependency management, git, frameworks we use, data stores we use, processes for review, our ci approach, our release approach, ...

Adding yet another process your work depends on, is fiddly to setup and doesn't have good docs online really isn't great. "We can add this on top, it's just basics" can really add up with enough systems.

Re: Git-appraise – Distributed Code Review for Git

#156

Post by globular-toast »

OkayPhysicist wrote: post #147
globular-toast wrote: post #145

Earlier quoted context omitted.

If you don't squash then you're essentially dedicating the history to code reviews because it won't be much use for anything else. If you leave broken commits on the main branch then it becomes really hard to bisect to find regressions. And that's the only reason for keeping history really.

git bisect --first-parent Solves that issue entirely. Let branches hold their history, let the release/main branch hold the history of stable builds.

If branches were rebased and then merged with `--no-ff` that could work.

Re: Git-appraise – Distributed Code Review for Git

#157

Post by jasonpeacock »

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'm going one-up you in craziness, and suggest Chelsea Troy's idea[1] that you don't bother commenting - after checking out the PR locally you just make the changes and then update the PR.

Sure, there's still discussion when needed, but otherwise take it as a collaborative effort where you're asynchronously pair-programming.

[1] https://chelseatroy.com/2019/12/18/reviewing-pull-requests/

Re: Git-appraise – Distributed Code Review for Git

#158

Post by xvilka »

palata wrote: post #144
xvilka wrote: post #115

Earlier quoted context omitted.

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.

I am not sure I follow this. If you can login to GitHub from your device, why couldn't you log into your e-mail?

The email-based workflow often requires quite specific email setup, many mobile-friendly email apps would break the formatting, etc, like Gmail.

Re: Git-appraise – Distributed Code Review for Git

#159

Post by jauco »

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…

In vs code you check out the pr branch and then do cmd-shift-p “show differences with” and then select “main”

Re: Git-appraise – Distributed Code Review for Git

#160

Post by palata »

viraptor wrote: post #155
palata wrote: post #111

Earlier quoted context omitted.

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

> but at some point, professionals need to learn the basics Think about a junior joining a company. Through the first weeks they get: to be useful you need to learn about dependency management, git, frameworks we use, data stores we use, processes for review, our ci approach, our release approach, ... Adding yet another process your work depends on, is fiddly to setup and doesn't have good docs online really isn't gr…

> "We can add this on top, it's just basics" can really add up with enough systems.

That's the thing. In my view, you say "we should keep adding wrappers on top of wrappers and libraries on top of libraries, such that juniors can quickly get productive, but nobody actually understands what they are doing anymore and the smallest executable takes 50MB".

I say "we should reduce the number of dependencies such that one can understand the system better, even if it means that juniors need to spend time learning the basics".

Of course there is a balance to find (not everybody needs to master assembly), but the modern way, IMHO, goes too much towards productivity, and not enough towards mastering ones tools.

Post reply on HN