I found various mentions of a desire for code review tools while troweling through past forum posts.

As someone essentially forced to use github for its code review features, I've come to really enjoy using them, while simultaneously hating the git day-to-day (especially via the console, where I tend to live.)

Putting a small spin on the desire for built-in Fossil code review, in the form of the following question: For those of us that desperately want to use Fossil and pitch the ditching of git/github entirely, who actually has code review via software as part of their workflow, and what IS that workflow?

What tools, how integrated, etc?

Insights appreciated. From what I'm seeing the pickings are awfully slim, and it feels more and more like code review is really what is the github killer feature - which feels disappointing when the philosophies of a code repo are otherwise so misaligned.

Code review may mean different things to different folks. I'll assume you want
the ability to review a diff, click and make a comment right there. Maybe even
'assign' it to different folks for their approval.

That doesn't exist in Fossil, but these do:

1. forum
2. /chat
3. /wikiedit
4. patch
5. bundle

wikiedit may be a stretch, but people could use it to somewhat annotate things.

That doesn't exist in Fossil, but these do:

That would be a great feature set to have in fossil but it's far from trivial to implement and nobody's volunteered to do it. Decisions would need to be made on which review data is permanent and which is transient, and any relevant new artifact type(s) would need to be created¹, file views would need to be created for flagging and commenting on individual lines of code (we have some of that already, and review comments could be modelled as forum post artifacts), notifications would need to be generated (we have those already, too), and workflows would need to be designed which are generic enough for general-purpose use. Etc. etc. etc.

i'd love to see such a thing, but it would be a good-sized undertaking.

Perhaps - just spitballing here - it would be feasible to create such a thing as a completely separate tool using libfossil with a CGI front-end (which could be accessed via fossil's "/ext" path). Perhaps it could be modelled in such a way that no new artifact types would need to be created. e.g. by storing all review-related content as JSON files in the wiki or in some well-defined directory of the repo. Or maybe it needn't be stored in the repo at all. It's generally useful to see who signed off on a review, but it's seldom necessary to go look through the review's conversations.

(That's not me volunteering, though. :)

1. ^{^} In such a way that they don't conflict with any existing types. All artifact types have to be distinctly identifiable as that type solely by their contents, as there is no "this is an artifact of type X" info in artifacts.

My idea is "generalized fossil" (which is not implemented, though; I did write about it though). One of its features is that there are no artifact types; any one can have any cards...

Implementing a timeline like the one we know and love would be impossible with such artifacts. If any given artifact can have all cards from A to Z, there is no way to classify them, making it impossible to filter the timeline on specific artifact types. If the architecture had been that way from the start of a project, i could see it somehow working (much differently than the way we currently use it), but the current architecture defines the constraints we have to work with.

Another another another alternative is to do both (e.g. having Fossil and Generalized Fossil implemented in the same database, by storing the list of artifacts that Fossil does not know in a separate database table that an extension will use).

That physical separation would not strictly be necessary for compatibility or syncing purposes: fossil will sync whatever is stored in the blob table¹. Similarly, any blob which fossil cannot strictly parse as an artifact is simply treated like an opaque blob. (Edit: ah, of course, the extension would want a list of which blobs are things it knows how to deal with, otherwise it would have to try to brute-force parse the whole blob table to figure it out... and it would end up parsing all of fossil's artifacts at the same time.)

PS: i would love to have some longer discussions on these topics with you off-list. i know you've been hacking on these things for at least a couple of years now and have put a great deal of thought into them². Though i am very conceptually interested in seeing other variants of this architecture get developed, within fossil (and therefore libfossil), the architecture is already set in stone. Thus i'm not interested in seeing (lib)fossil's structure completely overhauled, but am very interested in seeing architectures derived from it to see what wonderful new things people can build out of it. Please feel free to get in touch.

PPS: George, thank you again for adding footnotes support!

1. ^{^} There are some minor caveats to that statement, but it's mostly true.
2. ^{^} AFAIK, we have only one anonymous user who's well-versed in fossil's artifact model!

what IS that workflow?

To not do code review. :)

It is a mathematically provable fact that the more delays you put into your process's OODA loop, the more you slow your organization down. The ideal is to keep the necessary delays as short as possible, and to make them asynchronous between any two individuals. Code review creates simultaneous delay for at least two developers, and if it's done as you see recommended, it can take as long to get through the code as it did to write it, what with all the necessary explanations, tutorials, justifications, arguments, etc.

I'm not against having someone look over my shoulder, but it should be in one of two forms:

1. Pair programming, where the other is working with you live, helping you through a problem.

2. After-the-fact review. The commit occurs regardless of what anyone else thinks about it, and then someone (e.g. the lead developer) looks through the commits to make sure they're sane and advancing the project toward the lead's goals. Corrections are made inline on that development branch with more commits, not by rejecting PRs and making the author go back and fix them, then resubmit.

Contrast the typical PR model, where someone spends a bunch of time working in private on a grand unified feature, submits it, and then has to sit on their hands until someone does code review on it. When a PR becomes a blocking point like this, you create maximum OODA loop delay. Fossil's closest alternative to this is a feature branch, where it may wait for someone to merge it, but not for it to be committed.

The GitHub code review process is meant for bazaar development, where you get random PRs from previously unknown contributors. The GH code review features are helpful in that context because drive-by PRs are often terrible. The submitter may either be:

• clueless, hence needing a lot of feedback before their PR is suitable for merging; or
• mismotivated, hence needing convincing to come into line with the project's goals; or
• wrong, hence needing correction.

Fossil is designed for cathedral-style development where developers are expected to be clueful, to have their motivations aligned with those of the project's leadership, and to be much more often right than wrong. In that type of situation, what code review there is is perfunctory, checks made to ensure that things are on the right track.

Ask yourself: why do you feel the need to pick over your fellow committers' work line-by-line, as necessitated by GitHub's commit commenting features? Shouldn't you stop meddling and get some of your own work done?

while troweling through past forum posts.

You are likely mishearing "trawling," sense 1b.

the more delays you put into your process's OODA loop, the more you slow your organization down

Some delays that can potentially ensure better quality might sometimes be worth an intentional time tradeoff, but I suspect everyone's mileage varies. I'm not a subscriber to the "move fast and break things" mindset, especially when some workloads don't have hard deadlines attached.

I definitely didn't intend to suggest that code review was an alternative to pair programming or after reviews. It's another tool in the toolbox that can be helpful for certain groups, and completely useless for others.

Corrections are made inline on that development branch with more commits, not by rejecting PRs and making the author go back and fix them, then resubmit. [...] Fossil's closest alternative to this is a feature branch, where it may wait for someone to merge it, but not for it to be committed

This would be a nice melding of workflow -- no PR rejections (I agree, that's antithetical to retaining history) -- but review, inline discussion, and code evolution in a feature branch before final merging into the mainline.

I was mostly curious what others were doing that didn't involve using Github, and if the answers include "just email and patchbombs" to "nothing at all", I think that all qualifies. Gives me more to think on.

You are likely mishearing "trawling," sense 1b.

Probably. I meant digging through past sediment. With a trowel. I'm not a gardener.

In any event I appreciate this (and the rest) of the responses.

I’d imagine the capability to more broadly annotate / refer to things like sections of code to be helpful

Fossil already supports the ability to create a wiki page associated with a specific check-in. (See check-in 3990518b for example.) What if we extend this capability to associate an entire forum thread with a check-in?

Or, what if we invented a new artifact time (or enhanced the wiki or forum artifact types) so that you could attach a brief (Markdown-formatted) note to a specific range of lines in a specific file in a specific check-in?

We just passed the 4th anniversary of my reply to commit proposal.

I couched it in terms of code review, but notice that the discussion isn't a blocker to ongoing development, as GitHub's PR commenting features tend to be. The developer keeps on working, but they get some feedback about a past commit that may change what they do next time they go heads-up.

In terms of appearance, I'm imagining that if you have WrForum capabilities on a repo, /timeline can show a "Reply" button that, when clicked, creates a reply to the commit's comment as the parent message.

No data is duplicated. Instead, this type of forum post is distinguished by having a commit as a parent, so the forum knows how to render this as a new thread.

Obviously this requires that the code repo host a forum, unlike the Fossil or SQLite projects.

Adding a forum post tied to the commit is nice. But, I often want to comment on lines in a file. The forum post start with the commit then each reply targets an artifact (file) with optional lines.

While looking at a file artifact, you can enable line numbers and click on a line number and shift-click to get a range that produces a popup to copy the URL for that range.

A similar mechanism for the checkin diff report would work similarly. The checkin diff report includes line numbers. Marking a number range could triggers a popup that prompts the user to add a comment to the original forum post.

One issue is that each comment really should have some state with it. At least a binary open/closed, addressed/unaddressed so that they can be styled differently and allow scanning of the forum thread to see if there is anything still open.

Ideally there should be some way in the checkin diff report to see if a line has a pending comment. But closing this loop is something for code review version 2.

There's lots to say on this topic, but my left hand is killing me today, so i'll refrain except to expand a bit on:

One issue is that each comment really should have some state with it. At least a binary open/closed, addressed/unaddressed ...

That's one of the areas where some serious thought would need to be given to how permanent code review state really needs to be. Is such state really worth keeping around forever or is it transient? (i dunno - that's why i ask.)

Giving a forum post a P-card referring to a checkin sounds like a brilliant idea. How to tie the code lines (perhaps multiple blocks) into one, though... perhaps simply a T-card (tag)?

Giving a forum post a P-card referring to a checkin sounds like a brilliant idea.

I don't think it is necessary to add a P card to the forum artifact. We could simply say that if the forum title is of the form "checkin/HASH" then that whole forum thread refers to checkin HASH. That's how wiki pages are linked to check-ins - via a magic name.

Using that approach, no file format changes are enhancements are required. It's just a little more code to make the appropriate linkages.

That's one of the areas where some serious thought would need to be given to how permanent code review state really needs to be. Is such state really worth keeping around forever or is it transient? (i dunno - that's why i ask.)

That's a good question. I have worked at places where it's transient and places where it's permanent data. So I can reply to your question with a resounding yes 8-).

Places that treated it as transient seemed to take a "get the code out the door" approach to development. Questions about why things were done in a particular way, or how decisions were reached weren't fruitful.

Places that were more likely to be able to respond to "why" and "how" questions often used code review data. They were able to identify recurrent issues, validate changes to processes, and drive training. They could build tools to prevent certain types of code review issues from ever reaching human reviewers.

So the longevity of the review data really depends on the people using it. Most of the places I have worked ignore the code review data once the commit is completed. However, I had a better time working at places that used review data to improve their process and training.

That being said, Fossil may want to choose to make it transient to avoid bloating the repo. But if that is done does it means that it won't sync? This would prevent people from doing offline code reviews while in a subway on the way home.

That being said, Fossil may want to choose to make it transient to avoid bloating the repo. But if that is done does it means that it won't sync? This would prevent people from doing offline code reviews while in a subway on the way home.

That's precisely the train of thought i was on. Transient would likely mean "doesn't sync," which would somewhat unfortunate but perhaps justifiable. The "correct" answer is unclear to me.

Just spitballing, in any case.

So the longevity of the review data really depends on the people using it.

Exactly. This isn't necessarily a tooling issue, but a people management issue,
IMO based on how you're explaining your experience.

Perhaps the 'discussion' of a code review could be made into a technote or wiki page,
which could then be synced, but that may be precisely what you're proposing above.

Store the review info in a separate but related repository, and sync as required?

Perhaps the 'discussion' of a code review could be made into a technote or wiki page,

The most severe disadvantage, compared to forum posts, is the lack of concurrent editing. Everyone is editing the same wiki/technote and the last edit wins.

Fair point.

I think the idea I had in mind was like a chat transcription log, where
it would log who said what on which lines. Though this could get messy
with many files in a code review, but it may provide a simple way to
have the code review log sync'd with the repo.

This is possible, but it means putting the "review" repo in the same login group most likely you want the same users in both repos.

Also you would need to have a reference to a code range in repo A easily accessible in repo b.

Referring to checkins in the fossil repo from the (separate) forum repo is doable, but is a bit of a pain. Displaying review items from a forum page back into the timeline on the source repo would be difficult if not very convoluted.

Don't forget that one does not review static code, but different revisions / uploads, possibly of many files, with many comments.

This means that ones goes back to a review many times, often over several days, and each time at least I want to concentrate on the new comments I have not read before / yet.

Similarly, files in a review can come and go (or be renamed), to address review comments (most devs in my experience are not good at naming things...), and tracking what's new/changed/deleted from one visit of the review, to the next, is essential at that level too.

This implies that a proper review tool must track this state per-reviewer.

I've done hundreds of code review per year, for the past 15-20 years, and I consider that feature essential, especially in large and/or long / protracted code reviews.

That said, if comments or threads can be attached to file-hashes with optional line-ranges, that's already great. But I guess I'm saying that not enough for a proper code review process / tool.

Giving a forum post a P-card referring to a checkin sounds like a brilliant idea.

The P-card (and any other card) should not be used for a different meaning than what it is. The P-card means the previous version of a check-in or wiki or forum message or whatever else it might be, and it is not normally sensible that a check-in should be the previous version of a forum post, I think.

My other ideas below are other ideas relating to it. (Fossil does not currently allow what is mentioned below although a future version might. However, it could be applicable to Generalized Fossil.)

Adding a P-card referring to a check-in would make it a new version of the check-in (so it should also have F-cards). This might be useful for branches of proposed changes, but for "ordinary commentary" you might want to use the I-card instead.

For state such as open/closed, addressed/unaddressed, I suppose that you could either treat them as tickets and using J and K, or as tags and using T.

About how to tie the code lines, I don't know. Maybe T-card will help; I don't know.

I think the latter has some advantages - the comments/notes should probably correspond to a tuple like (check-in, file, line-range, author).

Since this looks like the next (and last?) big Fossil feature, I think we should take our time to find the best specification before heading to implementation. "Best" can be evaluated based on various criteria - richness, efficiency, maintainablilty/extensibility etc.

If we manage to get this right, I daresay this will be a killer feature for a lot of users that grew up with CI/CD. After that, there will be nothing missing except for "rebase" :)

(Maybe we should add a command "rebase" that just prints out something alone the lines "By concept, Fossil does not support 'rebase'. For motivation and alternatives, read [webpage]". :-) )

I'm nobody to judge, of course, but my experience suggests that linking a whole forum thread to a specific check-in or branch should be the way to go.

People could survive doing a little of copy-pasting in the end, if this means keeping the fossil code simple.

https://github.com/google/git-appraise provides distributed code review for git. Might be some ideas there??

After-the-fact review. The commit occurs regardless of what anyone else thinks about it, and then someone (e.g. the lead developer) looks through the commits to make sure they're sane and advancing the project toward the lead's goals. Corrections are made inline on that development branch with more commits, not by rejecting PRs and making the author go back and fix them, then resubmit.

Contrast the typical PR model, where someone spends a bunch of time working in private on a grand unified feature, submits it, and then has to sit on their hands until someone does code review on it. When a PR becomes a blocking point like this, you create maximum OODA loop delay. Fossil's closest alternative to this is a feature branch, where it may wait for someone to merge it, but not for it to be committed.

Can you clarify what you see as the difference between this and GH PR workflow? I'm not really seeing it. On the first point:

Corrections are made inline on that development branch with more commits, not by rejecting PRs and making the author go back and fix them, then resubmit.

I'm interpreting this to mean that the developer has not merged the development branch to trunk until the lead developer approves it. Am I misunderstanding that? Or are you saying that commit to dev branch, immediately merge to trunk, and if lead has a problem with it then you fix it on dev branch and immediately merge again?

Fossil's closest alternative to this is a feature branch, where it may wait for someone to merge it, but not for it to be committed.

This is exactly how GH PRs work.

This is exactly how GH PRs work.

While it is true that GH PRs can be based on branches, the whole idea of a PR is to pull what was submitted, not that plus whatever else occurs on that branch while the developer is waiting for it to be merged.

Setup sequence:

1. Hack, hack, hack.
2. Submit PR.
3. Rather that sit idly for approval, be productive. Either pick a second feature off the queue that depends on the first, or extend the initial feature.

Now what?

Does the developer create a branch of the branch to isolate the second change set from the first?

Or, does he go back to the branch's base and merge the first change set down regardless, as the basis of a new branch for the second change set?

Regardless, the second branch won't merge cleanly with the first PR unless it's accepted as-is.

Under my model, there is no "PR" step. The developer keeps working on that branch until someone decides it's safe to merge, after which point the developer works on the target branch instead. Corrections leading up to that merge occur on the development branch. Instead of sending a nastygram via GH code review, the reviewer can simply commit to the development branch.

And yes, I'm aware that this requires a single central repo. Thus the characterization of bazaar vs cathedral in all our "Fossil v Git" comparisons.

isn't GitHub also one of the biggest "central repositories" in the world?

Not in the sense meant here. A PR is a way to request that changes be pulled from one repo into another, invented for the case where the initial developer doesn’t have commit access on another’s repo.

PRs don’t require centralization or even public repositories, merely that the developer commit to a repo that the puller has clone rights on. Classic case, outside Linux kernel feature maintainers.

Fossil is also a DVCS, but the closest we have to this model are the bundle and patch commands.

the whole idea of a PR is to pull what was submitted, not that plus whatever else occurs on that branch while the developer is waiting for it to be merged.

You can push new commits to a branch after creating a PR, nothing wrong with that. GH has the idea of "draft PRs" which is a way to communicate that a PR is still in progress. But even if I've decided that the PR is ready for review, I can push new commits to it before it's been reviewed - and if someone has previously looked at all/some of the files in the PR, GH will indicate which of those files has changed since their last review.

Does the developer create a branch of the branch to isolate the second change set from the first?

That's pretty common if you want to isolate changes that build on pending changes.

Regardless, the second branch won't merge cleanly with the first PR unless it's accepted as-is.

Hrm... I don't know. Say you've got b1 branched from trunk, and b2 branched from b1. If there are new changes to b1, then you can merge them into b2 if you need them. Or keep working on b2 independently until it's time to merge if you don't need them. Merge b1 into trunk, merge b2 into trunk.

Under my model, there is no "PR" step. The developer keeps working on that branch until someone decides it's safe to merge, after which point the developer works on the target branch instead. Corrections leading up to that merge occur on the development branch.

There's no "PR" step... but there is sometimes a "post to the forum about a WIP branch" step, or a "see that someone has made a new branch in the timeline" step, or (I think, I don't have access) a "message in chat that someone made a branch" step. The underlying principle is the same: someone makes a branch, communicates that it's in progress, other team members have the chance to provide some feedback, and the team decides whether and when to merge.

Instead of sending a nastygram via GH code review, the reviewer can simply commit to the development branch.

This seems to me like it may be the crux of what you perceive as the difference... and even then I'd say that's more a matter of culture / chosen process, than any kind of technical difference. In shared repos, say in a commercial setting, there's no obstacle to committing directly to the branch. In fact this is something that I've encouraged new members on my team to do. They come from places where you see a PR, comment on it, and expect the other person to do the work. GH has several ways of letting the commenter do the work though - they can create "suggestions" which are code changes inline with comments that can be applied with a button press; they can commit directly to the branch; they can even make a new branch and create a new PR-2 into PR-1 with proposed changes to be discussed independently.

So then there's the question of open source projects without a shared repo... well if somebody submits a PR, and I as a maintainer need it to have changes - I can create a branch in my repo based off theirs, make the changes it needs, and commit it.

A GitHub PR is nothing more than a diff between commits, with the ability to comment. If you object to common workflows around PRs (e.g. "nastygrams") then I can certainly empathize. I don't think that has anything to do with PRs though. A fossil committer could look at someone's branch, post to the forum "you idiot, you better not commit those changes" and it would be fundamentally the same thing.

I'm not seeing as great of a difference between the Fossil code sharing and review process, and the git process, as you seem to. I would say Fossil definitely has way better visibility into a single repo's activities than git (or even github) provides. But some of the things you've said... well, trusted committers get to commit directly to a repo whether it's fossil or git, and untrusted ones don't. I have a fossil fork, nobody from the Fossil team gets to see my changes unless / until I post them to the forum for feedback. If you stored Fossil's code in a repo on github, all of the trusted members could commit directly to it.

We work in branches, we review the code by whatever mechanisms we have available, we merge the branches. Fossil or git.

One big thing about Github PRs is that the PR lives in the repository of the contributor, not the project. The UI seamlessly shows you the difference between the contributor's branch and the default branch of the project's repository (the project owner gets to choose).

As a project owner, I like this because I don't have to give randoms commit access to my repository, and don't end up with a million half-finished feature branches. As one of those randoms, I like this because I don't have to ask permission before I start working. Frequently I just want to play with stuff, and if it turns out to be useful I'll tidy things up and send a PR.

To do things the Github way on Fossil, you'd need a tool which can see both the contributor's and the project's repository. That's clearly not ideal for Fossil. However, doing it the Fossil way means putting everything into the project repository. Using actual Fossil branches for this is undesirable to me for the reasons described above.

The obvious compromise is to have special branches which are PRs. Call them 'drafts':

• the project admin can allow anyone to push them. (More likely, you'd probably want to have a self-service signup on the project website, for ease of user tracking, rather than allowing pure anonymous pushes.)

• they don't get synced by default --- you have to ask for them.

• they expire and get garbage collected after an interval.

This allows a centralised Fossil server to see both the project and the draft, which means a traditional PR-style code review is possible. The only issue is that expiring data out of a database is very much not in the Fossil ethos, but that's kind of a required feature to prevent abuse. I have vague memories that sqlite supports offloading some data into another database. Could this be used to implement something like thin forks, where a repository gets cloned without actually copying the data? If so, that'd make a reasonable implementation: the server would create one of these for each draft, so that nothing needs to get committed to the project repository until the draft is merged.

The PR lives in the project repo and the branch stays wherever it is until someone with access merges the PR into the project.

This does keep stray branches out of the main project and (at least the way our projects are configured) GH even deletes the branch when the PR is merged (I'm sure you can turn that off if you want to).

Another possible way to do this in Fossil is like the above model except to use a separate "PR repository" for incoming branches. The contributor pushes their branch to the PR repository and the project gets visibility into that. If you want to clean up, you can sync any pending branches in the scratch repo into another scratch repo and delete the old one. Rejected branches disappear and merged branches will already be in the project repo. This might violate the "remember everything" principal but it's a way to manage spam anyway.

Sorry, yeah, I was conflating the PR (which is visible from the project's github page) and the branch (which is in the contributor's repo). The PR itself isn't stored in any VCS. This also means that any code review comments, which are part of the PR, don't end up in the repository either.

(Where I work we use Perforce. Every commit is a PR, which gets reviewed before being approved to be merged into the main branch. You effectively get to levels of commit: the PR-level stuff, and the versioning within the PR itself as the author works on it. This gives you a combination of the feature-based commit model which some git repos like, where each commit represents a distinct feature, plus the advantages of being able to look inside and see how it got that way, as well as the entire code review conversation. It's very heavyweight but I like it a great deal.)

Add my few cents here with the features I really miss (some of them already mentioned):

1. References to line blocks and multiple line blocks of a certain checkin of a file (aready suggested few times).
2. Display these references in diff as suggested in /info/c2fe626dd2f5bd83.
3. Mention of a user for example with github-like "at"-syntax (like @zeliboba7) and a possibility to send an email alert on the mention.

I belive, the points 1 and 3 can be partially implemented as an extension to Markdown, which will allow to referencing the lines and users in forum/wiki/tickets. It looks like similar points are mentioned in src:/wiki?name=To+Do+List, but not sure if those are related to code review.

(not sure about policy on posting to the old threads, please advice if not it is not appropriate)

WY EDIT: URL fixes.

We already have #1. (Proof) What we don’t have is an easy way to do that directly from a diff display section on the /info page. The prior link is several clicks/taps/swipes away from the most recent commit info page where it was changed.

It would indeed be nice to select lines or line ranges from a diff section directly.

(PS: You would do well to treat the forum’s Preview button as an aid prior to posting, not as a pointless speed bump. My impression of your use of interwiki references was that you made a plausible guess as to the desired URL without checking that they referred the things you wanted referenced. I believe I second-guessed you correctly, but… 🤷‍♂️)