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

161–170 of 173 posts

Re: Git-appraise – Distributed Code Review for Git

#161

Post by mike_hearn »

IshKebab wrote: post #153
mike_hearn wrote: post #150

Earlier quoted context omitted.

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.

git push. We use Gitolite which is a git server setup that can be made to enforce access control rules based on branch names.

https://gitolite.com/gitolite/index.html

Re: Git-appraise – Distributed Code Review for Git

#162

Post by d4rkp4ttern »

jauco wrote: post #159
d4rkp4ttern wrote: post #110

Earlier quoted context omitted.

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”

I don’t use vscode, but does this show the diffs in the gutter within the IDE ? Or does it simply display the diffs in a separate pane?

Re: Git-appraise – Distributed Code Review for Git

#163

Post by withinboredom »

palata wrote: post #103
withinboredom wrote: post #95

Earlier quoted context omitted.

Deleting three lines to keep both comments doesn’t seem too bad. You might even just delete your comment entirely if it’s for the same thing.

I completely agree, if you don't make many reviews and if it does not happen often. But still it puts the burden on the reviewer rather than on the patch author. And the reviewer's time is never less precious. If you need a review from Linus Torvalds, I think it makes sense to make it as seamless as possible for him. And that is what the e-mail workflow does, I would say.

It depends on what you value in a review. For me, a review is for me to get a glimpse at new parts of the code and learn what is coming. It is important that I understand the WHY more than the WHAT. I also put code reviews as the highest priority item in my todo list. I will always do a code review next, if there is one to do. If you send me a request, you'll have a review from me before the end of my day.

Why? Because unblocking my teammates is the most important thing I can do, and I expect the same from my teammates. I never want the reason our team is late to be "I had more important things to do" because (cliché incoming), "the is no 'I' in 'team'".

A reviewers time isn't more valuable than mine, and my time is no more valuable than anyone else (assuming accountants don't exist). If changing 3 lines is really an issue, then create a merge-tool for this workflow and add it to your git configuration file.

Re: Git-appraise – Distributed Code Review for Git

#164

Post by palata »

withinboredom wrote: post #163
palata wrote: post #103

Earlier quoted context omitted.

I completely agree, if you don't make many reviews and if it does not happen often. But still it puts the burden on the reviewer rather than on the patch author. And the reviewer's time is never less precious. If you need a review from Linus Torvalds, I think it makes sense to make it as seamless as possible for him. And that is what the e-mail workflow does, I would say.

It depends on what you value in a review. For me, a review is for me to get a glimpse at new parts of the code and learn what is coming. It is important that I understand the WHY more than the WHAT. I also put code reviews as the highest priority item in my todo list. I will always do a code review next, if there is one to do. If you send me a request, you'll have a review from me before the end of my day. Why? Becau…

Sure, but what if your lead has a review list so big that they almost never get to empty it totally? I would consider that the time of such a lead is definitely more valuable than mine. Again, it's easy for me to accept that Linus Torvalds' time is more valuable than mine.

> If changing 3 lines is really an issue, then create a merge-tool for this workflow and add it to your git configuration file.

Or use the e-mail workflow, where the lead can just comment on the patch right away, without having to even pull the branch (and worse, push comment and potentially fix conflicts), right?

Re: Git-appraise – Distributed Code Review for Git

#165

Post by withinboredom »

palata wrote: post #164
withinboredom wrote: post #163

Earlier quoted context omitted.

It depends on what you value in a review. For me, a review is for me to get a glimpse at new parts of the code and learn what is coming. It is important that I understand the WHY more than the WHAT. I also put code reviews as the highest priority item in my todo list. I will always do a code review next, if there is one to do. If you send me a request, you'll have a review from me before the end of my day. Why? Becau…

Sure, but what if your lead has a review list so big that they almost never get to empty it totally? I would consider that the time of such a lead is definitely more valuable than mine. Again, it's easy for me to accept that Linus Torvalds' time is more valuable than mine. > If changing 3 lines is really an issue, then create a merge-tool for this workflow and add it to your git configuration file. Or use the e-mail…

I understood that the presented workflow meant that it had already been reviewed, it just needed to be merged.

But on a team, I’d say there is no Most Valuable Player.

Re: Git-appraise – Distributed Code Review for Git

#166

Post by indymike »

Corsome wrote: post #121
indymike wrote: post #117

Earlier quoted context omitted.

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

> A meaningful comment stay as long as it is meaningful. Sometimes even longer :)

# TODO remove this

is my all time favorite.

Post reply on HN