[go: up one dir, main page]
Skip to content

GitHub Import: Switch from LegacyDiffNote to DiffNote usage

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

Problem

Currently, GitHub Importer uses two note types DiffNote and LegacyDiffNote. Only comments with suggestions (and if the commit is still present on the GitHub side) are imported with DiffNotes, any other note is created via LegacyDiffNote.

The new DiffNotes was introduced a pretty long time ago and as it is stated here:

All new diff notes are of the type DiffNote, but any diff notes created before the introduction of the new implementation still use LegacyDiffNote.

which is not 100% true since GitHub Importer still uses it. LegacyDiffNote have some limitations in terms of diffs visibility and resolvability, so it can cause some issues like #381503 (comment 1393663758)+. This issue will be handled separately and fixed on UI.

Proposal

Get rid of LegacyDiffNote usage for GitHub imports since looks like GitHub Importer is the only one that still uses it.

Technical details

The main issue is with importing Comments that are referencing commits that no longer exist (were overridden by force-pushes). To show diff on UI for DiffNote two commits (head and base) are compared. But since the commit no longer exist on Github (but GitHub api returns it's sha for comment to specify at what point the comment was left to which code is related) there is nothing to compare to get the diff and to create NoteDiffFile. There are a couple of places where logic depends on commits comparison. And as it is stated here

And last, if this is about GitHub missing the commits... well, nothing much we can do in that case

There is a couple of scenarios of how to proceed with it:

  1. Additionally fetch missing commits (it will work for garbage-collected commits that are still present in GitHub), so the issue will still take a place.
  2. Store diff hunk in st_diff (as it is done for LegacyDiffNote) and use it for diff_file creation when the commit is not present.
  3. Do not import comments if the commit does not exist (but it will be a breaking change since it was imported with LegacyDiffNote before).

So option 2 was chosen. For me, it looks like the existing logic for handling DiffNote creation was designed for the creation of new notes on GitLab and is not well-suitable for importing. The changes are not related to GitHub Import only so can affect existing logic for DiffNote creation.

There is WIP MR with details and problems faced during implementation.

Edited by 🤖 GitLab Bot 🤖