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

101–110 of 173 posts

Re: Git-appraise – Distributed Code Review for Git

#101

Post by nurbl »

jauco wrote: post #50
joachimma wrote: post #47

Earlier quoted context omitted.

If you leave the comments, doesn't the code get cluttered? If you remove the comments, don't you lose information? I could see this being useful if there was a script, that moved the comments out into either the commit message or a separate file.

Oh, yeah I didn't explain that part. In GH you'd click "resolve" to mark a discussion as finished. Here you just remove the comments to do the same (they're still in the git history, so you don't loose them forever) This doesn't need to be an extra step. If the comment is something like: "potential null pointer here" you can just fix the bug and remove the comment in one go, the reviewer will see what you changed fro…

When we did this, I think we had some git hooks to make sure that no comments in "review form" were left when merging. So you'd have to go through all the comments and at least remove them.

Re: Git-appraise – Distributed Code Review for Git

#102

Post by goku12 »

palata wrote: post #89
goku12 wrote: post #43

Earlier quoted context omitted.

> What's wrong with patch email chains? The fact that it's email. There are two categories of problems with it: 1. It's been extended unnecessarily and implemented poorly. The biggest one in this category is HTML email. It's really hard to write mails using common clients without messing up mailing lists. Another problem is how badly email threading is displayed in these clients. Email UI is still abysmal. 2. Many fe…

> Another problem is how badly email threading is displayed in these clients. Email UI is still abysmal. Fair point. However, given that the current alternative is "use another service entirely (e.g. GitHub)", I think it would be fair to assume that devs could choose a good e-mail client and learn how to format such e-mails correctly. It works for Linux, for instance. I started using Aerc, and I love it: https://aerc…

> 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 browser.

> we should have something better than e-mail in 2023

We really should have something better than email. I'm saying this as someone who operates a personal mail server and a bunch of desktop services for it. It's really hard to get the setup correct.

In that context, it's worth looking at forgefed (https://forgefed.org/). It's a protocol for federating forges like Gitea and Gitlab. It's built on top of ActivityPub - which behaves a bit like email (it has inboxes and outboxes for every user). From the spec, it seems like pull requests happen by sending patches to the destination forge.

> Nobody takes the time to try the e-mail workflow (even though it's really two git commands)

Email workflow seems simple. But there are two things that make it complicated:

1. The patches don't specify the commits they apply to. It's simply assumed that they apply to the head of the main branch. The commits have to be carefully rebased on the main branch before sending the patches. It could otherwise lead to conflicts and a lot of wasted time.

2. Each commit/patch is send as a single email. Developers usually make frequent commits when they develop. Such patches can be confusing and hellish to review. A sane patchset requires the developers to edit the commit history, usually using interactive rebases. Each commit should contain a single feature and shouldn't break the build.

I consider both the above to be good development practices and follow them even on my personal projects. However, this is an additional barrier to entry. In fact, this may be a bigger problem for many than setting up git for email.

Re: Git-appraise – Distributed Code Review for Git

#103

Post by palata »

withinboredom wrote: post #95
palata wrote: post #83

Earlier quoted context omitted.

Which I can imagine could be annoying in some situations :-). But no workflow is perfect. I think I like the idea that in an e-mail, I can really comment on some patch, and then the author of the patch can decide what to do with it. Maybe the author modified the patch in the meantime, but they will know if my comment is still valid or not. Without me having to handle the conflicts. I'm thinking mostly about leads who…

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.

Re: Git-appraise – Distributed Code Review for Git

#104

Post by goku12 »

sirsuki wrote: post #96
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 have been giving this one a lot of thought. I jumped to sourcehut for my personal projects and was amazed how viscerally pleasing a [PATCH] email thread could be. I am convinced there is a big psychological difference between discussions via Pull Requests and discussions via email/mailing lists. I can’t pin point it but it is almost palpable. A question I have with the process is: can email based workflow still wor…

> A question I have with the process is: can email based workflow still work with for a continuous integration/delivery pipeline?

Doesn't sourcehut already support that? (https://man.sr.ht/builds.sr.ht/)

Re: Git-appraise – Distributed Code Review for Git

#105

Post by palata »

sirsuki wrote: post #96
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 have been giving this one a lot of thought. I jumped to sourcehut for my personal projects and was amazed how viscerally pleasing a [PATCH] email thread could be. I am convinced there is a big psychological difference between discussions via Pull Requests and discussions via email/mailing lists. I can’t pin point it but it is almost palpable. A question I have with the process is: can email based workflow still wor…

> I jumped to sourcehut for my personal projects

Same :-). I also love how people don't have to create an account to send a patch. They just need to send me an e-mail. That is not a problem on GitHub (because everybody is forced to have an account there), but it is if you want to host your repo somewhere else.

> can email based workflow still work with for a continuous integration/delivery pipeline?

SourceHut has a CI feature integrated with the mailing-lists: https://man.sr.ht/builds.sr.ht/

Re: Git-appraise – Distributed Code Review for Git

#106

Post by goku12 »

palata wrote: post #84
worldsayshi wrote: post #82

Earlier quoted context omitted.

Don't you lose a bit of important information when you create a patch? Like which commit it started from.

You don't send a diff, you send an e-mail formatted by git. So you get all the commits you put in it, it's really just like seeing the commits in a GitHub PR. So the reviewer receives the commits, with their descriptions, authors, everything. Just like pulling a branch, really.

You do lose the information about where to apply the patch. This isn't a big problem for rebase-friendly crowd. But people who prefer the merge strategy strictly may not like it.

Re: Git-appraise – Distributed Code Review for Git

#108

Post by chriswarbo »

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…

> Edit: I wondered about using a separate Blockchain to maintain consensus on the "central" representation of the repo that we get from a "git server".

There's nothing special about a "git server": you can clone/push/pull/etc. from any checkout you like (apart from shallow clones, which may not have the requested commits). You can do this using local file paths, or over SSH, or over HTTP, etc. For example, here's an old blog post describing my git setup (pushing commits between folders on my laptop, and syncing those to multiple mirrors (a self-hosted static file server and GitHub) when online): http://www.chriswarbo.net/blog/2015-06-06-more_git.html

You might declare that some particular clone is the central repo for some project, but nobody has to care what you say: everyone's free to push/pull from each other, or send patches over email, regardless of what you've written on some blockchain or website. That's an advantage of git!

Edit: If you want a verifiable mechanism to know what X considers to be the canonical version of Y, you can use "whichever commit of Y that's signed by X has the highest timestamp". That may work better with more anonymous p2p transports like https://github.com/dhappy/git-remote-ipfs https://scuttlebot.io/apis/community/git-ssb.html

Re: Git-appraise – Distributed Code Review for Git

#109

Post by cdogl »

chriswarbo wrote: post #87
epolanski wrote: post #79

Earlier quoted context omitted.

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.

> 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. Another thing that's easy, but often overlooked: putting relevant URLs in comments. The best comments explain why some approach was taken; linking to a discussion on JIRA, or the Stack Overflow answer we c̶o̶p̶y̶-̶p̶a̶s̶t̶e̶d̶ ̶f̶r̶o̶m̶ used as inspiration, is (a) quicker than w…

I prefer clean code that doesn’t have comments except as doc strings for various parsers on declarations and in areas that are particularly surprising.

If I want to know why something was done, I expect to be able to use git log or git blame on the file or line and get find a commit message, issue tracker link or pull request reference that I can pursue.

Re: Git-appraise – Distributed Code Review for Git

#110

Post by d4rkp4ttern »

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 workflow/command to achieve the above? (Granted this is not so bad, and was suggested by GPT4, but I wondered if there might be a more straightforward way)

Post reply on HN