Skip failed LFS objects and continue when migrating LFS #31623
Conversation
wolfogre
added
type/bug
topic/lfs
backport/v1.22
GiteaBot
added
the
lgtm/need 2
wolfogre
changed the title
GiteaBot
added
lgtm/blocked
|
... so if repo_module.StoreMissingLfsObjectsInRepository is reused at other code parts we do handle this ... and only skip by case in the migrator |
A dedicated error to indicate "Not found" can help only when we can choose to "ignore and continue" or "stop and fail". So I don't think it makes sense to block this PR. We should merge this first and focus on |
|
I still think it should be fixed in this pull |
@6543 I understand you think this PR doesn't fix it completely (I agree with that so it's "Partially fix"), but it at least it doesn't makes things worse. And the introduced TODO comments indicate what we should do next. By the way, you said "I would create an dedicated error ...", I'm not sure whether you mean you will do this or you suggest me to do this? I thought it was "(If I have time) I would..." so I waited for two weeks. But I just noticed that it could be "(If I were you) I would...", so we need to clarify it. 😄 Never mind, It's just a friendly discussion about English studying between two non-native English speakers. If you suggest me to do this, I would say I already tried, but it need more work since we cannot check the error just from the message, it need follow the spec to differentiate error types, but Gitea misses it in LFS. So I chose to add the TODO and finish it in another PR. |
|
I added the errors here: #31697 |
|
ok pushing to your branch was not intendet ... well @wolfogre either keep this pull and close mine or reverte my changes on your pull if you are not ok with it etc ... |
|
Close it since #31702 finished the work. |
…ion (#31702) Fix #31137. Replace #31623 #31697. When migrating LFS objects, if there's any object that failed (like some objects are losted, which is not really critical), Gitea will stop migrating LFS immediately but treat the migration as successful. This PR checks the error according to the [LFS api doc](https://github.com/git-lfs/git-lfs/blob/main/docs/api/batch.md#successful-responses). > LFS object error codes should match HTTP status codes where possible: > > - 404 - The object does not exist on the server. > - 409 - The specified hash algorithm disagrees with the server's acceptable options. > - 410 - The object was removed by the owner. > - 422 - Validation error. If the error is `404`, it's safe to ignore it and continue migration. Otherwise, stop the migration and mark it as failed to ensure data integrity of LFS objects. And maybe we should also ignore others errors (maybe `410`? I'm not sure what's the difference between "does not exist" and "removed by the owner".), we can add it later when some users report that they have failed to migrate LFS because of an error which should be ignored.
…ion (go-gitea#31702) Fix go-gitea#31137. Replace go-gitea#31623 go-gitea#31697. When migrating LFS objects, if there's any object that failed (like some objects are losted, which is not really critical), Gitea will stop migrating LFS immediately but treat the migration as successful. This PR checks the error according to the [LFS api doc](https://github.com/git-lfs/git-lfs/blob/main/docs/api/batch.md#successful-responses). > LFS object error codes should match HTTP status codes where possible: > > - 404 - The object does not exist on the server. > - 409 - The specified hash algorithm disagrees with the server's acceptable options. > - 410 - The object was removed by the owner. > - 422 - Validation error. If the error is `404`, it's safe to ignore it and continue migration. Otherwise, stop the migration and mark it as failed to ensure data integrity of LFS objects. And maybe we should also ignore others errors (maybe `410`? I'm not sure what's the difference between "does not exist" and "removed by the owner".), we can add it later when some users report that they have failed to migrate LFS because of an error which should be ignored.
…ion (#31702) (#31745) Backport #31702 by @wolfogre Fix #31137. Replace #31623 #31697. When migrating LFS objects, if there's any object that failed (like some objects are losted, which is not really critical), Gitea will stop migrating LFS immediately but treat the migration as successful. This PR checks the error according to the [LFS api doc](https://github.com/git-lfs/git-lfs/blob/main/docs/api/batch.md#successful-responses). > LFS object error codes should match HTTP status codes where possible: > > - 404 - The object does not exist on the server. > - 409 - The specified hash algorithm disagrees with the server's acceptable options. > - 410 - The object was removed by the owner. > - 422 - Validation error. If the error is `404`, it's safe to ignore it and continue migration. Otherwise, stop the migration and mark it as failed to ensure data integrity of LFS objects. And maybe we should also ignore others errors (maybe `410`? I'm not sure what's the difference between "does not exist" and "removed by the owner".), we can add it later when some users report that they have failed to migrate LFS because of an error which should be ignored. Co-authored-by: Jason Song <[email protected]>
Partially fix #31137.
When migrating LFS objects, if there's any object that failed (like some objects are losted, which is not really critical), Gitea will stop migrating LFS immediately but treat the migration as successful.
This PR makes it ignore failed objects and continue migrating. It may not be the best way to handle it, but a better way.
And to ensure data integrity of LFS objects, we may need to add an option to fail the migration if there's any error in the feature.