Code review assistance

[lexxdark]

TLDR: I'm wondering on this if a code review feature should/could/is wise to be added to jj. Or if anybody knows of some extensions that can do that (either for jj or git for that matter)

On my side whenever I have to do a code review I did a bunch of scripts to help out with the task, which essentially do a diff with the main branch (well with the most recent common ancestor to be exact) and lunch my IDE on said file and a diff tool on each file. The process continues to the next file whenever I close the diff tool. Right now I'm a bit lazy to migrate these tools to

I couldn't find any tools to help me out with this especially for seeing the diff and switching into the IDE etc.

I'm wondering if it would not be wiser to have something backed in the SCM which would help you out with this also allowing you to mark some changes as reviewed.

A lot of ideas pop into my head on this like:

• having plugins for different tools (like gerrit github etc) to be able to also add the comments on the actual code review tool.
• being able to do a diff to whatever you reviewed until now and include the new diffs incoming from the upstream code
• being able to add comments on the diff directly
• this can come also as help when you develop, you can use this review functionality to add comments on what other things need to be done

Disclaimer: I really liked the sapling integration where it can fetch you detailed info about the PR status of a branch/commit, but I know that comes with some performance downsides and I know that is far from what I'm suggesting here.
Also this looks like a really complex implementation, but it should provide quite a few benefits in term of how one can develop and code review.

[PhilipMetzger]

We have both #485 and #2765 for a Web-UI and Code-Review integration, which we're probably going to base off Sapling's ISL.

[lexxdark]

May I say noice on both features they are both awesome and can't wait to see both of them implemented!

But what I was thinking more was like an integration to do the code review locally, which may or may not be covered in the WebUI issue above. It doesn't look covered from what I'm seeing there, but maybe I don't know enough.

What I was thinking is something along the lines:

• start a code review
• when you went through a file and agree that it's fine mark it as such and store that information in the repo as a commit (my thinking is just store the file in a commit in a parallel "branch" to what you are code reviewing with some marker)
• considering the storage is done via commits you can pause the review at any time and not lose progress
• if the PR changes you are able to diff against what you already reviewed and the nice thing is you could follow the PR even if rebased
• be able to lunch the diff tools configured on any of the files
• be able to lunch the ide on any file

Again, I'm wondering if it's a good ideea. What I don't like about it is that it's duplicating a bit on what the GitHub/Gerrit etc do and that it sounds complex. On the other hand if I don't have this I usually write a quick and dirty script to at least iterate through the files on a fast pace, because I don't like the flow of checking in the code review tool then copy pasting in the IDE/opening my favorite diff viewer to see the change, if I need to check something on it.

[lexxdark]

Note: I'm not sure if maybe the above was what you meant by "Code-Review integration" or it was just the ability as in sapling to check the status or maybe I don't know the feature well enough in sapling (I looked at sapling, but didn't do a full test drive to see everything that it can do)

[matklad]

I see that #485 is mostly about integrating with GitHub's review. GH review is not good for two reasons:

• it's just not good as an implementation of review workflow -- no native UX for stacked commits workflow, browser slows to crawl for larger reviews, etc
• it exists outside of the main data model -- the data is hidden behind REST API, so you can'd provide an alternative implementation, git clone doesn't get you the actual full state of repository, etc.

I'd personally would love to see someone to try to do a 10x improvement here, and move the entire metadata about the reviews (and issues, while we are at it) into the repository itself.

I think gerrit did half of that eventually: https://gerrit-review.googlesource.com/Documentation/note-db.html

But I think that's still gerrit's private implementation detail (stemming from the fact that initially it was a separate database).

I wonder if it's possible to create a sort-of open format for how stacked commits, reviews & issues are represented in git database, so that client-side tools and server-side tools could be developed indepenently, interoperating on the format?

One hard problem here is permission: on GitHub, when A sends a PR to B's repository, both A & B can "mutate" the review state. Additionally, even any C who merely has a github account can leave comments!

Though, even without solving the perms, built-in git-object-level code review would be tremendously helpful for intra-team reviews, leaving GitHub PRs only for the external collaborators.

[amiryal]

I’ve never used Fossil, but from what I read, I think it is the kind of self-contained all-in-one distributed SCM you describe.

[PhilipMetzger]

I see that #485 is mostly about integrating with GitHub's review.

I disagree, as it currently is used as an umbrella issue for Codereview integration with stacked diff support.

I'd personally would love to see someone to try to do a 10x improvement here, and move the entire metadata about the reviews (and issues, while we are at it) into the repository itself.

There is git-appraise which also partially does this.

think gerrit did half of that eventually: https://gerrit-review.googlesource.com/Documentation/note-db.html

But I think that's still gerrit's private implementation detail (stemming from the fact that initially it was a separate database).

I recall that using git-notes is generally slow and the same probably counts for NotesDB, which is why so many deployments of Gerrit use the SQL database.

I wonder if it's possible to create a sort-of open format for how stacked commits, reviews & issues are represented in git database, so that client-side tools and server-side tools could be developed indepenently, interoperating on the format?

This is probably possible, although I'd prefer if the storage details were kept agnostic of Git for easier integration with Jujutsu.

One hard problem here is permission: on GitHub, when A sends a PR to B's repository, both A & B can "mutate" the review state. Additionally, even any C who merely has a github account can leave comments!

ACLs are generally hard, so it'd probably be better of as a separate service but that is just my opinion on that.

Though, even without solving the perms, built-in [...] code review would be tremendously helpful for intra-team reviews, leaving GitHub PRs only for the external collaborators.

👍