migrations: remove dead code in gitea uploader

[singuliere]

When migrating, g.issues is a map with all issues created during the
migration. If an issue is not found in g.issues when inserting a
comment or a review, it cannot exist in the database and trying to get
it via GetIssueByIndex() will always fail and return an error.

[wxiaoguang]

I think g.issues is used as a cache. If we want to support incremental migration, then we need these code.

Personally I prefer to leave it as it was

[singuliere]

It is dead code because there is no codepath that leads to it. There is no test that exist or could be written that would lead to these lines. If incremental migrations do not require modifying the migrateRepository functions, issues are handled first and the cache is populated in full. If incremental migrations use a logic different from migrateRepository the Gitea uploader will need to be refactored entirely. Or something else implemented from scratch. I can't imagine a single scenario where this dead code comes back to life. Can you?

[lunny]

This is totally wrong. CreateIssues and CreateComments maybe invoked

[zeripath]

It may be helpful to add a warning to the top of the functions involved stating that all of the issues need to be created before this is called occurs.

The code paths here are somewhat convoluted and complex (likely necessarily so) but we need to be clearer in the comments about what each function is expecting to have already happened.

I can easily imagine some new API coming out whereby people get the issue and all of its comments in one go - the refs would then probably break

[singuliere]

This is totally wrong. CreateIssues and CreateComments maybe invoked

I'm confused about what is wrong. Could you please explain? Is the proposed change wrong? Or am I wrong in thinking that the gitea uploader CreateComments function is only called from the migrateRepository function?

[singuliere]

It may be helpful to add a warning to the top of the functions involved stating that all of the issues need to be created before this is called occurs.

The code paths here are somewhat convoluted and complex (likely necessarily so) but we need to be clearer in the comments about what each function is expecting to have already happened.

I can easily imagine some new API coming out whereby people get the issue and all of its comments in one go - the refs would then probably break

The only thing I was able to come up with is the following:

//
// The migrateRepository function determines the context in which this method is invoked.
// It relies on other methods being called first, to populate caches, update the git
// repository, etc. If it was to be called independently, care must be taken to fulfill
// its expectations.
//
// CreateReviews create pull request reviews
func (g *GiteaLocalUploader) CreateReviews(reviews ...*base.Review) error {

This is very vague and not useful, I'm afraid. I can't figure out how to be more specific without being so specific that the comment will likely become obsolete as soon as the slightest change is introduced in the logic. For instance at the moment it relies on CreatePullRequests updating the git repository to create references to pull requests and, if there is a fork, fetching the commits from the fork into the repository.

Do you have a suggestion?

[Gusted]

Do you have a suggestion?

- // CreateReviews create pull request reviews
+ // CreateReviews create pull request reviews of currently migrated issues

The CreateIssues is already called before hand which already inserts the issues into g.issues, so something along the lines of currently imported issues or currently known issues, no need to go big, otherwise the code should be self-explanatory.

[singuliere]

Do you have a suggestion?

- // CreateReviews create pull request reviews
+ // CreateReviews create pull request reviews of currently migrated issues

The CreateIssues is already called before hand which already inserts the issues into g.issues, so something along the lines of currently imported issues or currently known issues, no need to go big, otherwise the code should be self-explanatory.

That makes perfect sense, thanks for the advice. I added a commit with your suggestion 👍

[singuliere]

The CI is stuck with:

Initialized empty Git repository in /drone/src/.git/

+ git fetch origin +refs/heads/main: