This is an update on the development of Flirt. Previous posts:

It's been a while since the last post. I was focused on my thesis, which I handed in on August 1st 🥳

I managed to get comment threads basically working and implemented backends for GitHub and mailing lists. These features are limited and buggy in several ways. But I think this can be overcome with some effort.

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.

Continue reading for some technical details about the GitHub and mailing list backends!

Local storage

The native backend stored everything locally by necessity, that's how Git works. You fetch Spirits from the remote, do stuff with them, and push them again. For now, I ended up using the same custom ref mechanism for the other backends as well. It would be annoying if every time you want to do something, Flirt would hit the GitHub API or download an email thread. The custom refs stay local for those backends and are converted to API calls or emails when the user requests to sync with the backend.

Past submissions of GitHub PRs

The biggest issue with GitHub is that you don't get information about past force-pushes to a PR. That means, Flirt can't show you the interdiff between all PR submissions in principle. In practice, it's not so bad. You probably only care about submissions you previously observed. The standard use case is: interdiff between last-reviewed and current submission. That works mostly fine, because the last-reviewed submission can be remembered locally.

Some caveats:

  • Switching between workstations can break that. If workstation has never observed your last-reviewed submission, it can't easily get it from the GitHub API.

  • The API tells you which exact commit hash any review comment was made on. That could be used to reconstruct past submissions. But then you'd get a weird edge case where, if there's a comment on a "middle" commit, but no comment on the commit at the tip of the PR branch, the submission would only be partially reconstructed. Implementing that would be difficult without introducing more weirdness.

Comments on the left side of the diff

GitHub's review UI shows you the diff of a PR or commit to comment on. Flirt on the other hand adds comments on a specific state of the code base and assumes you look at the diff via regular VCS tooling. The problem with that is, Flirt can't add comments to the left side of the diff. That raises the question: Where to put them? Adding them to the right side of the diff in a "similar" location is no good, because the context might be completely different.

The best short-to-medium term solution I can come up with is adding the context of the deleted lines as a "header" to the comment block containing the thread. That way, the context is always where the thread is. The thread could either be put in a "similar" location to where the lines were deleted, or, if that doesn't work, in a separate special file that's created only as part of the review workflow.

But, I don't think that's quite ideal either. Thinking about this problem has started to convince me that only looking at one state of the code and representing the previous state as the Git HEAD is not enough. Being able to type comments inside an actual diff file is starting to feel like a valid and important use case to me. That might be a second "mode" supported by Flirt, or maybe there is even a hybrid workflow that combines the best of both worlds. Either way, it'll probably take some time for this to be implemented in Flirt.

public-inbox, the open-source mailing list archive

A mailing list is kind of an abstract concept. Supporting it as such is not possible. In practice, the open-source projects I know use a project called public-inbox to host their mailing list archives. Specifically, that includes the Linux kernel and Git.

For now, I have opted to only support projects using public-inbox. Please let me know if there are other important projects using something else! I would be interested to at least explore how much effort it would take to support them.

Missing information in git format-patch output

The best "standard" a review tool built on top of mailing lists is the output of git format-patch. Sadly, it leaves a few things to be desired for Flirt's use case.

Base commit

git format-patch can add the base commit information automatically, but doesn't do so by default. Users have to request it with the --base flag. If the base commit information is missing, what should Flirt do? Apply the patches to HEAD, or to master? Either way, it's going to be wrong and lead to conflicts in some cases. We could let the reviewer specify the base manually, but that's pretty bad UX in my opinion. Ideally, the reviewer would just ask the patch series author to resubmit with --base.

Jujutsu change-id header

Jujutsu stores its change-id in a custom commit header. git format-patch doesn't preserve them. There was a proposal a while back on the Git mailing list to change that. Unfortunately, I don't think that effort was continued.

Without the change-id header, the most important feature of Flirt doesn't work reliably. Flirt can implement heuristics like matching subject line, author and so on. git range-diff basically tries to solve this problem simply based on the similarity of the patches. Whatever Flirt ends up doing, it will have to be a heuristic given the current constraints.

Flirt has a relatively strict model of Spirits and their submissions. That doesn't perfectly map onto mailing lists. git format-patch can add a version tag to the subject line. Well-behaved patch series submissions will have subject lines like:

  • [PATCH v1 0/N] add some feature
  • [PATCH v2 0/N] add some feature
  • [PATCH v3 0/N] add some feature

If the subject line is specific enough to distinguish it from all other patch series, it can be used to determine all submissions of a Spirit. But, the version tag can be forgotten and the prose might change.

Depending on the project, there is a more reliable way to detect related versions. The Git projects recommends that the first email of every patch series is a reply to the first email of the previous submission. That way, all submissions form one big email thread. Here's a little visualization:

[PATCH v1 0/3] add feature
├── [PATCH v1 1/3] implement feature
├── [PATCH v1 2/3] test feature
├── [PATCH v1 3/3] document feature
│
└── [PATCH v2 0/3] add feature
    ├── [PATCH v2 1/3] implement feature
    ├── [PATCH v2 2/3] test feature
    ├── [PATCH v2 3/3] document feature
    │
    └── [PATCH v3 0/3] add feature
        ├── [PATCH v3 1/3] implement feature
        ├── [PATCH v3 2/3] test feature
        └── [PATCH v3 3/3] document feature

This is great for Flirt, and it's the one format I chose to support for now. Fallback heuristics can be implemented in the future.

Unfortunately, this practice isn't as widespread as one might like. The Linux kernel documentation specifically recommends against it:

However, for a multi-patch series, it is generally best to avoid using In-Reply-To: to link to older versions of the series. This way multiple versions of the patch don’t become an unmanageable forest of references in email clients.

When I read that, I couldn't help thinking to myself: "Shouldn't it be an indication that the technology you're using is unfit for the task, if you intentionally choose to omit clearly relevant, structured information, in order to work around its limitations?"

Reading and writing comments

Here's what a typical "comment thread" looks like on a mailing list:

>> +fn foo() {
>> +    println!("foo")
>> +}
>
> I think this function could be implemented more efficiently.

Which concrete optimizations do you suggest?

Parsing this into a structured format is, as you can imagine, quite messy. It's not rocket science, there are just a lot of edge cases.

  • Threads don't have to be continued in emails that are direct replies. It's legal to quote text from an earlier email.

  • Commenters can quote text that doesn't originate in the thread. That's not a reply to an existing thread, it's just a quote.

  • Commenters often abbreviate and edit the context they are replying to. Ideally, such comments would still be detected as part of the existing thread. Maybe it makes sense to split quotes on the common [...] abbreviation delimiter and search for those substrings in the existing threads?

For now, I've got it working for very simple cases only.

Sending comments email is a little more easy. Flirt just has to obey the conventions at a minimum. For now, the direct predecessor of a new comment is quoted as the context.

If that one is really long, users might want to abbreviate it before sending the email, but that's not possible for the moment.

Actually, Flirt doesn't send real emails at all. It just constructs them and pipes them into public-inbox-mda in order to import it into a local testing instance of public-inbox. After some more testing, actually sending those mails shouldn't be too hard. (right?)

Conclusion

That's all the technical ramblings I have to report for now. Next time you'll hear from me, Flirt should be open-source. Also, I will be attending JJ Con this year. Feel free to say hi 🙂