fix: scope LFS diff detection to repository - #39059
Conversation
There was a problem hiding this comment.
I don't see how this PR "fixes" the problem.
The new logic is just stricter than the old one, the root logic is still unclear.
Could you clarify how the bug #39020 happens? Why you need to query the LFS meta by repo ID? No matter whether a LFS object can or can't be accessed by viewer, LFS pointer is LFS pointer.
|
Thanks for pointing that out. I understand your concern. My current change mainly makes the LFS lookup stricter, but I haven't clearly established that this is the actual root cause of #39020. I'll investigate the difference between the PR diff path and the repository file view to understand why the same LFS pointer is being displayed differently. I'll update the PR once I have a clearer understanding of the root cause. |
|
Thanks, I looked into this more. The issue is not about whether the viewer can access the LFS object. The problem is that the old diff logic only checked whether the OID existed anywhere in the LFS metadata table. Since the LFS metadata is associated with a repository, an OID belonging to another repository could make a pointer in the current repository's diff get treated as an LFS file. GetLFSMetaObjectByOid() already checks both the repository ID and OID, and the existing PR LFS handling also uses the repository ID. So this change passes the repository ID through the PR diff path and uses the repository-scoped lookup. I also added a regression test covering both cases: the OID belonging to another repository and the OID belonging to the current repository. |
Then, why the viewer is viewing "an OID belonging to another repository"? Why current repo doesn't own the LFS object? |
|
You're right. I looked into the PR diff flow more closely. For a fork PR, the diff is generated using the base repository's Git repo, while the head ref points to the PR's head commits. So the diff can contain an LFS pointer whose metadata belongs to the head repository rather than the base repository. The old lookup only checked whether the OID existed anywhere in lfs_meta_object, so that pointer could incorrectly be classified as an LFS file. The change makes the lookup repository-scoped by checking both repository_id and oid. I'll add/verify a regression test that specifically covers this fork PR scenario. |
This comment was marked as outdated.
This comment was marked as outdated.
|
I looked into the fork PR flow more closely and found that the diff is generated using the base repository's Git repo, while the PR's head commit comes from the head repository. Because of this, the LFS pointer in the diff can belong to the head repository. |
|
Which approach is right?
|
|
I think allowing both the base and head repositories makes more sense. The LFS lookup should stay scoped to the repositories involved in the PR, while avoiding a global OID lookup. This should cover both same-repository and fork PRs without allowing an unrelated repository to affect the detection. |
Fixes #39020.
Git LFS pointers were previously detected by OID without considering the repository they belonged to. This could cause an LFS file in a pull request to be displayed as plaintext when the same OID existed in another repository.
This change passes the repository ID through the pull request diff path and scopes the LFS metadata lookup to that repository.
Added a regression test covering:
Tests:
go test ./services/gitdiffgo test ./routers/web/repo ./routers/api/v1/repo ./services/gitdiff ./services/repository/filesgit diff --check