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

141–150 of 173 posts

Re: Git-appraise – Distributed Code Review for Git

#141

Post by imoreno »

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…

Neat. What drawbacks have you seen?

Just from your comment, I can see: * People don't get notifications about new review requests across dozens of repos * No good way to just get a list of remaining comments (I guess you grep the diff for RVW?) * Hard to set up "require all comments to be resolved before merging"

These could be automated easily with scripts and git hooks though, then it would be a pretty good flow.

Re: Git-appraise – Distributed Code Review for Git

#142

Post by brandall10 »

mike_hearn wrote: post #114
brandall10 wrote: post #97

Earlier quoted context omitted.

"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 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 run roughshod over engineers they don't respect.

Re: Git-appraise – Distributed Code Review for Git

#143

Post by paulddraper »

OkayPhysicist wrote: post #137
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…

I've never gotten the point of squash-merge. The normal "merge into release branch, never fast-forward" workflow retains more useful information. Every release commit has 2 parents: the previous release commit, and the former head of the feature branch it got its changes from. If you want a chain of releases, you go down the path of commits labeled "RELEASE". If you want information on the changes, go down the other…

> I've never gotten the point of squash-merge.

1. Broken intermediate commits hamper git bisect.

2. Fixup commits are noisy in git log, git blame, etc.

Note: there's zero issue with NON-BROKEN, NON-FIXUP commits. Separating large changes is even preferable for the reasons you stated.

---

tl;dr Broken intermediate commits are bad; whole intermediate commits are good.

Re: Git-appraise – Distributed Code Review for Git

#144

Post by palata »

xvilka wrote: post #115
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…

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?

Re: Git-appraise – Distributed Code Review for Git

#145

Post by globular-toast »

OkayPhysicist wrote: post #138
globular-toast wrote: post #98

Earlier quoted context omitted.

I assumed the code review commits would be squashed before the merge. But you raise a valid point about the code review process being lost. One way to get around that is to keep the feature branches (or maybe just tags) forever and do all the squashing on the integration branch. This does require some kind of convention that records on which branch/tag the code review for a particular feature can be found. The advant…

Or just don't squash. There's no need. In fact, if you're doing a review process like this, it would be active vandalism to to squash, since there is a big difference between "code said x, commented on by reviewer, changed to y" vs "code says y".

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.

Re: Git-appraise – Distributed Code Review for Git

#146

Post by IshKebab »

mike_hearn wrote: post #72
IshKebab wrote: post #51

Earlier quoted context omitted.

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

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

Re: Git-appraise – Distributed Code Review for Git

#147

Post by OkayPhysicist »

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

Earlier quoted context omitted.

Or just don't squash. There's no need. In fact, if you're doing a review process like this, it would be active vandalism to to squash, since there is a big difference between "code said x, commented on by reviewer, changed to y" vs "code says y".

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.

Re: Git-appraise – Distributed Code Review for Git

#148

Post by alchemist1e9 »

zamalek wrote: post #107

How does this interact with rebases? If it's attached to git objects surely the notes will detach when the object hash changes?

No responses is worrying. I’m also in the heavy rebase workflow pattern and would be huge problem if rebase breaks it.

Re: Git-appraise – Distributed Code Review for Git

#149

Post by procrastitron »

alchemist1e9 wrote: post #148
zamalek wrote: post #107

How does this interact with rebases? If it's attached to git objects surely the notes will detach when the object hash changes?

No responses is worrying. I’m also in the heavy rebase workflow pattern and would be huge problem if rebase breaks it.

Rebases are supported; you have to do the rebases through git-appraise in order for it to know that the original and rewritten commits are related.

Re: Git-appraise – Distributed Code Review for Git

#150

Post by mike_hearn »

IshKebab wrote: post #146
mike_hearn wrote: post #72

Earlier quoted context omitted.

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

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 reviewed because it's the reviewer who merges them when they're satisfied.

Post reply on HN