Next, I'm now going to clean things up a bit and prepare to make Flirt open-source. It won't be ready for users, but for people who can relate to my ideas for code review and want to shape the development of Flirt from early on. I'm aiming to get that done within 1-2 months.
Very exciting! Looking forward to contributing. :)
Being able to type comments inside an actual diff file is starting to feel like a valid and important use case to me.
Yeah... My current thinking is that I want a "rich diff" text format, that shows the diff between base and HEAD, for all files, and includes all comment threads inline. Like, + for added lines, - for deleted lines, c for review comments, maybe r for resolved.
This text format doubles both as a canonical source of truth for the state of the review, as well as the most bare-bones UI. The real UI can then be built on top, either enriching the plain text with LSP (syntax highlighting! Magit style folding! Navigation (I think it's reasonable to assume that you have HEAD and diff opened at the same time)! Code action to add a comment!), or building something fully custom to, eg, show PR as a static web page with rendered markdown.
Though, its a bit unclear to me, what should happen here if the PR branch is rebased, so that both base and HEAD change ... Well, one constraint is that no comment should never get lost, even if it doesnt' attach clearly to the new diff, but beyond that, its fog of war. It probably doesn't make to materialize the rich diff for every PR change, as that's going to get too much duplication. So probably the actual store of data is linked list -- a review contains:
link to HEAD commit
link to base commit
A set of comments on the diff (diff is assumed to be deterministic (which is another design area --- how do you incorporate custom diff algorithms?))
A link to the previous review
So, to get the rich diff, the tool looks at the latest review, computes the diff, then walks the link list and attaches all the comments. Comments from the last batch attach clearly, comment from the previous batches might become orphaned, they are attached heuristically, and, presumably, there's some sort of functionality to go to the review version that introduced the comment?
But then the question, how do you store it? You definitely want to capture, when the PR is merged, the final state of the review, the final base, HEAD, and cumulative set of comments. However, you probably actively don't want to capture the entire history of the review. Imagine
Alice: Why did you commit 4MiB cat picture to your repo, please delete the file?
Bob: Sure, done!
Alice: Sight, you have to force push it, otherwise the blob is still in history?
Bob: 😅, done.
Would be a shame if the file is still pinned by the history!
So perhaps it is the merge commit that materializes the "review diff" in commit message?
All questions are rhetorical, I am basically just thinking aloud here!
This text format doubles both as a canonical source of truth for the state of the review, as well as the most bare-bones UI.
I don't think Flirt could use such a text format as the source of truth. For any backend, we have to consider the information we get from it as the source of truth. A "rich diff" format could be the source of truth for the native backend, but I don't see much benefit to that. (compared to the simple "define Rust type, dump as json" Flirt uses now)
Though, its a bit unclear to me, what should happen here if the PR branch is rebased, so that both base and HEAD change ... Well, one constraint is that no comment should never get lost, even if it doesnt' attach clearly to the new diff, but beyond that, its fog of war.
I did have this idea that when the code author does the rebase - and they use flirt themselves - flirt could ask them to provide the information where old threads should be located in the new, rebased version of the code. But that would only really be useful if the information is synced with the backend, which most backends don't support directly. The native backend could support that more easily, but I don't expect most users to use that for actual work.
In the absence of that "perfect" solution, I'm planning to put these orphaned comment threads in the same place where I put "general" comments. Probably just some file. The original diff (that doesn't apply anymore) can be shown at the start of the thread, so there's at least some context for what that thread was about.
You definitely want to capture, when the PR is merged, the final state of the review, the final base, HEAD, and cumulative set of comments. However, you probably actively don't want to capture the entire history of the review.
I would distinguish between bloat on the remote and local clones. If Flirt stores pointers to all commits that were ever part of a Spirit (which it currently does, for the native backend), then yes, force-pushed-over cat pictures could never be deleted on the remote. local clones would not include it though, if they don't have the Spirit ref. Git doesn't fetch custom refs by default. So, Flirt can manage "local bloat" by removing old Spirit refs locally. If you want to save space on the remote as well, you have to be OK with losing data permanently, so the UI around that should be done carefully.
Very exciting! Looking forward to contributing. :)
Yeah... My current thinking is that I want a "rich diff" text format, that shows the diff between base and HEAD, for all files, and includes all comment threads inline. Like,
+for added lines,-for deleted lines,cfor review comments, mayberfor resolved.This text format doubles both as a canonical source of truth for the state of the review, as well as the most bare-bones UI. The real UI can then be built on top, either enriching the plain text with LSP (syntax highlighting! Magit style folding! Navigation (I think it's reasonable to assume that you have HEAD and diff opened at the same time)! Code action to add a comment!), or building something fully custom to, eg, show PR as a static web page with rendered markdown.
Though, its a bit unclear to me, what should happen here if the PR branch is rebased, so that both base and HEAD change ... Well, one constraint is that no comment should never get lost, even if it doesnt' attach clearly to the new diff, but beyond that, its fog of war. It probably doesn't make to materialize the rich diff for every PR change, as that's going to get too much duplication. So probably the actual store of data is linked list -- a review contains:
So, to get the rich diff, the tool looks at the latest review, computes the diff, then walks the link list and attaches all the comments. Comments from the last batch attach clearly, comment from the previous batches might become orphaned, they are attached heuristically, and, presumably, there's some sort of functionality to go to the review version that introduced the comment?
But then the question, how do you store it? You definitely want to capture, when the PR is merged, the final state of the review, the final base, HEAD, and cumulative set of comments. However, you probably actively don't want to capture the entire history of the review. Imagine
Alice: Why did you commit 4MiB cat picture to your repo, please delete the file?
Bob: Sure, done!
Alice: Sight, you have to force push it, otherwise the blob is still in history?
Bob: 😅, done.
Would be a shame if the file is still pinned by the history!
So perhaps it is the merge commit that materializes the "review diff" in commit message?
All questions are rhetorical, I am basically just thinking aloud here!
Or just shoving the text file into your LLM of choice, if that's your thing....
Or, like, https://heiwiper.com/posts/magit-status-tour/
I don't think Flirt could use such a text format as the source of truth. For any backend, we have to consider the information we get from it as the source of truth. A "rich diff" format could be the source of truth for the native backend, but I don't see much benefit to that. (compared to the simple "define Rust type, dump as json" Flirt uses now)
I did have this idea that when the code author does the rebase - and they use flirt themselves - flirt could ask them to provide the information where old threads should be located in the new, rebased version of the code. But that would only really be useful if the information is synced with the backend, which most backends don't support directly. The native backend could support that more easily, but I don't expect most users to use that for actual work.
In the absence of that "perfect" solution, I'm planning to put these orphaned comment threads in the same place where I put "general" comments. Probably just some file. The original diff (that doesn't apply anymore) can be shown at the start of the thread, so there's at least some context for what that thread was about.
I would distinguish between bloat on the remote and local clones. If Flirt stores pointers to all commits that were ever part of a Spirit (which it currently does, for the native backend), then yes, force-pushed-over cat pictures could never be deleted on the remote. local clones would not include it though, if they don't have the Spirit ref. Git doesn't fetch custom refs by default. So, Flirt can manage "local bloat" by removing old Spirit refs locally. If you want to save space on the remote as well, you have to be OK with losing data permanently, so the UI around that should be done carefully.