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

71–80 of 173 posts

Re: Git-appraise – Distributed Code Review for Git

#71

Post by palata »

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…

I'd love to know why I have been downvoted on that...

Re: Git-appraise – Distributed Code Review for Git

#72

Post by mike_hearn »

IshKebab wrote: post #51
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…

It's not that hard if you use an IDE. I review work this way all the time. It involves:

1. Click the fetch icon. New work to review shows up.

2. Right click to compare the branches, take a look through the diffs.

3. If I see something I want to check, click "Checkout" in the branch selector and start making the changes, commit, push.

4. Otherwise when done, use git merge and delete the origin branch (I have some shell functions for this).

One thing I've found hard to settle on is what message to use in the merge commits. I don't insist all changes have tickets because it'd just be redundant work for the case of small cleanups, fixing bugs you spotted whilst doing other stuff. But then the default merge commit message isn't terribly helpful, and writing one that is can end up redundant.

Another problem is that the railway lines can get quite hairy if people aren't disciplined about constantly rebasing. You can of course rebase for them, but then you lose the ability to view only the merge commits to master.

Re: Git-appraise – Distributed Code Review for Git

#73

Post by mike_hearn »

raffraffraff wrote: post #34

Would it be possible to build a gitlab-like ui that is completely decentralized? Instead of depending on (or paying $$$ for) some web UI that wraps standard git functionality, could we run something completely locally that keeps repos/branches in sync and provides the basic merge/comment/review functionally that most people use? In other words, let me host the most basic git server in the world and put the complexity…

What's wrong with a $5 VPS and using your IDE as the git UI?

Re: Git-appraise – Distributed Code Review for Git

#74

Post by yurishimo »

infogulch wrote: post #36
Arbortheus wrote: post #28

Earlier quoted context omitted.

I assume it would also have the advantage of not tying the code reviews and PRs to a proprietary git provider. Say I move from GitHub to GitLab, I assume I lose all the PR descriptions and comments people have made over the years and only keep the commit messages. It would be nice to have a provider-agnostic representation of this data.

I seem to recall the gitea project working on something like git-tracked metadata (PRs, issues, etc), at least the idea has been floating around for a while. One problem with this idea that I haven't seen a good solution for is serializing issue numbers across distributed forks. Maybe we would give up nice sequential numbers and settle for hashed issue refs.

We could also hash the issue with a time sensitive UUID. Then they could be ordered but unique to each person assuming they don't create an issue in the same microsecond.

Re: Git-appraise – Distributed Code Review for Git

#75

Post by withinboredom »

palata wrote: post #67
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 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.

Re: Git-appraise – Distributed Code Review for Git

#76

Post by mike_hearn »

Cthulhu_ wrote: post #59
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…

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 forum (email, slack, or for recording, the issue tracker). Then once the design disagreement is resolved, go back to using CR comments.

Re: Git-appraise – Distributed Code Review for Git

#77

Post by epolanski »

Cthulhu_ wrote: post #59
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…

I think the point previous user is making or I would add is: it is essentially up to you to decide what to keep and what not.

Simple example: say a comment points out some issue with the changes (say, lack of unit test on feature a), author could say that emulating scroll events in unit tests is complex and the mocks provide no real value to actual in-browser behavior.

This comment is something you may want to remove for the final merge or add a TODO: in the code, or leave the comments as a discussion for future but meanwhile push so prod people can test, etc.

You could move these comments to your saas git provider tools (e.g. github issues or comments or wiki for the final merge).

In any case the thing I like about this approach is that essentially *forces* users to actually checkout the damn thing and go through its code. I think this is something I would really want to test out with my team. I have a junior coming soon to my team (it's his first project and employment ever, he's 18), and I will test this approach with him and see how it works before I propose it to some of my peers and seniors.

Re: Git-appraise – Distributed Code Review for Git

#78

Post by wordpad25 »

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

I'd love to know why I have been downvoted on that...

the answer is in one of the first lines in your linked article "Email isn’t as sexy as GitHub"

Re: Git-appraise – Distributed Code Review for Git

#79

Post by epolanski »

mike_hearn wrote: post #76
Cthulhu_ wrote: post #59

Earlier quoted context omitted.

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…

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.

Re: Git-appraise – Distributed Code Review for Git

#80

Post by jauco »

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…

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