restore-repo: issue comments are no longer imported

[drsybren]

Description

Since 71ca306 issue comments are no longer imported with gitea restore-repo.

See func (r *RepositoryRestorer) GetComments(...) in services/migrations/restore.go , it still uses commentable.GetForeignIndex() (link) but as far as I can see that foreign index isn't loaded any more since that commit.

This patches things up for me locally, but I don't know if this is the right thing to do:

diff --git a/services/migrations/restore.go b/services/migrations/restore.go
index fd337b22c..6ee878a64 100644
--- a/services/migrations/restore.go
+++ b/services/migrations/restore.go
@@ -198,7 +198,7 @@ func (r *RepositoryRestorer) GetIssues(page, perPage int) ([]*base.Issue, bool,
 // GetComments returns comments according issueNumber
 func (r *RepositoryRestorer) GetComments(commentable base.Commentable) ([]*base.Comment, bool, error) {
        comments := make([]*base.Comment, 0, 10)
-       p := filepath.Join(r.commentDir(), fmt.Sprintf("%d.yml", commentable.GetForeignIndex()))
+       p := filepath.Join(r.commentDir(), fmt.Sprintf("%d.yml", commentable.GetLocalIndex()))
        _, err := os.Stat(p)
        if err != nil {
                if os.IsNotExist(err) {

To reproduce the issue, unzip demo-repo-22581.zip, then run gitea restore-repo -r /path/to/demo-repo-22581 --owner_name youruser --repo_name demo-repo-22581

When visiting the repo in Gitea's web interface, you'll notice that there is one imported issue, but no comments, even though there is a corresponding file in demo-repo-22581/comment/104010.yml.

There's nothing about this in the logs.

Gitea Version

29b78bc

How are you running Gitea?

Self-built from the main branch.

Database

PostgreSQL

[zeripath]

Hmm... My original suggestion was to simply add a primary key to that table and IIRC correctly my code to do that was even removed from that PR and therefore I should not have been left on the commit as an author.

IIRC it was later decided that the foreign reference wasn't used anywhere and it was simpler to drop the table completely. I don't know this code very well so I don't understand why and wherefore this was added.

@wxiaoguang IIRC you said to remove this table?

[zeripath]

Now looking at #18446. ForeignReference was added as a way to keep a match between a migrated repo issue id and pull id and its original repo. The idea presumably being that it would help in future to auto add incremental migrations and federation etc.

Of course the problem is whether the ForeignReference is actually helpful for that - possibly not as it may need to have the actual url for the entity that is referencing, but I'm not certain.

---

Now I don't fully understand why the code in #21721 would break things. @drsybren could you please give clear instructions on how to reproduce your issue and report any logs?

[wxiaoguang]

Maybe the PR #21721 missed something (hmm, it's caused by an old bug in history). If @wolfogre doesn't have time, I can take it.

---

ps: I think the patch in the first comment could be right. We can take it, but need to clarify all details (see the comment below)

[wxiaoguang]

About why:

Now I don't fully understand why the code in 21721 would break things.

Actually, I think it's caused by some PRs before 18446. If I understand correctly, there was an IssueContext, which has ForeignID() and LocalID(), they all returned the same result at that time. However, that design also seems to be flawed, and using ForeignID for issue yaml files seems to be incorrect: The exported files are named by the original repo's issue ID -- which is likely to be LocalID.

The root problem is: the ForeignID and LocalID were already ambiguity. For example, if site-a exports a dataset, then site-b imports it:

• site-a exports files by using its LocalID
• from site-a's view, LocalID is site-a's ID while ForeignID is site-b's ID
• but from site-b's view, LocalID is site-b's ID while ForeignID is site-a's ID

Maybe I misunderstood something , but indeed the design really seemed misleading for me.

---

In history, that's all correct: fmt.Sprintf("%d.yml", opts.IssueNumber))

https://github.com/realaravinth/gitea/blame/2d1935acc7b2a6ecc797346625b80caa7e0b5787/modules/migrations/restore.go

But #16356 introduced IssueContext and started using ForeignID. At that time ForeignID was LocalID, so the design bug didn't cause problems. But now, the bug finally causes problems.

[drsybren]

To reproduce the issue, unzip demo-repo-22581.zip, then run gitea restore-repo -r /path/to/demo-repo-22581 --owner_name youruser --repo_name demo-repo-22581

When visiting the repo in Gitea's web interface, you'll notice that there is one imported issue, but no comments, even though there is a corresponding file in demo-repo-22581/comment/104010.yml.

There's nothing about this in the logs.

[wxiaoguang]

Fix restore repo bug, clarify the problem of ForeignIndex #22776