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

51–60 of 173 posts

Re: Git-appraise – Distributed Code Review for Git

#51

Post by IshKebab »

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…

That sounds like a great idea! Difficult to see any significant downsides. OTOH checking out a branch and doing a commit involves slightly more friction that just clicking some links. I probably wouldn't bother if I had GitHub or Gitlab set up. But if I was starting a new company and didn't have them set up yet this sounds like genius.

I guess you can do a similar thing with issues just by having a folder of markdown files for each issue and appending comments to them.

If some conventions could be agreed you could have GUI support.

Re: Git-appraise – Distributed Code Review for Git

#52

Post by thatguy27 »

rurban wrote: post #18
gigatexal wrote: post #2

Needs a vscode plugin/integration

There is an extra webui nearby, though. Similar to git-bug

I tried https://github.com/google/git-appraise-web, with a review made only locally, not pushed to the remote, but it doesn't seem to work. Did I miss something? I went up to accepting the request, so the command line is working, but the web GUI - at each step - only shows a closed request [0], and clicking on it gives an empty page.

[0] https://ibb.co/HTg8Jfc

Re: Git-appraise – Distributed Code Review for Git

#53

Post by jauco »

F-W-M wrote: post #49
FPGAhacker wrote: post #48

Earlier quoted context omitted.

What did you do with all the comments at the end of the review?

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 approach was chosen over another approach.

If you have code reviews from developers with differing skill levels and the response to the question is just an explanations how the language or framework works then this might become a bit cumbersome (because you have to leave the comment to reply and then remove the whole discussion before merging)

Re: Git-appraise – Distributed Code Review for Git

#54

Post by vander_elst »

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 think this has some drawbacks because it seems to require a lot of discipline from all the parties, to keep the source code clean once you are ready to merge and even if you have some presubmit checks if a human makes a mistake and has some typo in the comment, that might end up in the main branch. Ideally all the new changes are small but that's not always possible and the larger the change the higher the probability to leave comments around. I agree it's not the end of the world but such things do add up over time and if you are in a big shop things will definitely get messy, it's just a matter of time.

Re: Git-appraise – Distributed Code Review for Git

#55

Post by seeekr »

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 alone, I do that too sometimes! But I've also found that most people have a hard time doing or following along with something like that. Possibly because in the end it does require a whole lot of skill in using git, because you will usually want to rewrite history in some ways, and large numbers of developers are very uncomfortable with that.

Re: Git-appraise – Distributed Code Review for Git

#56

Post by Corsome »

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…

Wow, that's really cool. If these commits were marked one could quickly drop them during rebate at the end of the review before merging. Or just remove them when the fix is applied.

Thanks for sharing!

Re: Git-appraise – Distributed Code Review for Git

#57

Post by sevazhidkov »

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’ve recently listened to a talk from Jane Street about their review system: they use this method exactly with a helper tool that keeps track of what you’ve already reviewed in the PR, minus any changes that were committed after review. PR author also “reviews” the branch, so the code review comments and changes would show up on self-re-review (because they were not reviewed).

https://youtu.be/MUqvXHEjmus Transcript: https://www.janestreet.com/tech-talks/janestreet-code-review...

Re: Git-appraise – Distributed Code Review for Git

#58

Post by SonOfLilit »

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 also used to do this in a couple of small startups. I loved having the review history just stored in git, and how commits that fix a problem also delete the comment about it, so when reviewing fixes I make sure every newly deleted comment is taken care of, then git grep "CR:" and if it comes back empty we're good to go.

However, I got a feeling it wouldn't scale well to large teams. Specifically because you have to manually keep very strict about not mixing reviewed and unreviewed code, and if you do it's hard to disentangle them and decide what's in scope for a review.

Re: Git-appraise – Distributed Code Review for Git

#59

Post by Cthulhu_ »

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 github/lab, gerrit, etc), unless you also wrote tools that can figure out which commits and comments belong to a certain feature.

Re: Git-appraise – Distributed Code Review for Git

#60

Post by Corsome »

AceJohnny2 wrote: post #29

Very tangential: Gerrit also stores some of its configs in a git repo. I was setting up a new instance, but couldn't get Admin permissions because the way my auth front-end didn't play well with the docker image's assumptions. Gerrit already does a lot of its work via non-standard references. For example, you don't push to a branch, `refs/heads/foo`, you push to a separate `refs/for/foo` namespace that creates the re…

> Gerrit also stores some of its configs in a git repo.

So does gitolite where the config (repos, user's SSH keys etc) is stored completely in a git repo. Perfect for migration, backup, audits.

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

Post reply on HN