Storage paths must be unique

[jolheiser]

I propose reverting #26318 in 1.20 and implementing this PR as-is, and also frontporting this enhancement for future versions.

#26318 would need to be reverted, otherwise the derivation would be "fixed" and this PR wouldn't have the intended effect.

This PR aims to help mitigate the bug being fixed by #26318 in a way that doesn't lead to silent data loss for existing users affected, while also being a valuable sanity check in the future.

/cc @go-gitea/technical-oversight-committee

Also /cc @earl-warren for the initial idea implemented here

---

For anyone directed here because of the log message

You'll want to follow the message and explicitly set your storage paths. Be aware that this will mean Gitea looks in new paths, so for any data that still lives in the old path, you will need to move it to the correct spot, such as repositories, LFS, attachments, etc.

Feel free to stop by any of our support avenues if you need further assistance.

[lunny]

I like this PR's idea but it will not conflict with #26318 whatever we will revert #26318

[jolheiser]

I like this PR's idea but it will not conflict with #26318 whatever we will revert #26318

#26318 would need to be reverted, otherwise the derivation would be "fixed" and this PR wouldn't have the intended effect.

[wxiaoguang]

I agree to make sure every path to be unique, and #26318 is not ideal.

While I do not see why #26318 needs to be replaced by a single fatal message, I do not know how the "fatal" message would help end users.

Imagine a real case (this example is from #26264)

A user set [repository].ROOT=/data/git/repositories, [storage].PATH=/data/git, then every thing is mixed in the /data/git, then they sees this "fatal message", what can they do?

The user's /data/git might be like this:

repositories/owner/repo.git  # for repositories
ab/cd/...   # for packages, lfs ...
12345/ab/...  # for repo archiver
754d63406bd4e450d338562da7a50741f49d6b27b9ef7064f99791929c760d89 # for avatars ...
# and for actions, etc ....

Then DeleteRepositoryArchives calls storage.Clean(storage.RepoArchives), it deletes everything in /data/git

What's the problem?

IMO the only missing thing is to teach the poor users how to separate these files into different folders, if the files could be separated, then #26318 is good enough.

ps: Every path should be unique (and they shouldn't overlap in most cases). The DeleteRepositoryArchives function's dangerous behavior should also be fixed.

[jolheiser]

The purpose of reverting the other PR is to avoid silent data loss.
This PR will cause a fatal, yes, which is noisy. Annoying, but better than silently losing data imo.

With the other PR in place, the derivation naturally fixes this noise, which is good in future (unreleased) versions, but less good in this (released) version because affected users may suddenly be missing data rather than a noisy message telling them something is wrong.

It's my opinion that a noisy failure to start is better than the potential for data to be lost in a patch upgrade. (Although neither is ideal)

[wxiaoguang]

Yup, see the real example #26380 (comment), if Gitea "fails" to start, then what should be done (how to "fix" the fatal)? That's the same question as #26318 . If the question can be answered, I think #26318 can be kept by a "detection"

[wxiaoguang]

ps: I approved #26318/#26271 because I guess it could be a rare case for users who set [storage].PATH manually. Either "breaking" or "fatal" wouldn't make too much difference (both need the unavoidable "fix" operations), so just let it go. While if there could be some more better solutions, eg: more detailed documents to help the users to fix the problem manually, more smart detection, more smart doctor fix helper, I would also approve.

[jolheiser]

The difference is the breaking version is silent and this one is not.

I would maybe be convinced to change this to error in this version, but future versions should still likely retain fatal.

I agree that mixed data isn't going to be simple to extricate, but at least affected admins should be made aware of it, not just silently start experiencing bugs.

[wxiaoguang]

The difference is the breaking version is silent and this one is not.

IMO the difference is that:

• Use the "breaking": the instance still starts, the repositories still exist, if any file is missing (avatar/lfs/packages), the user could re-upload them (or if there is a document, they could manually move them)
• Use the "fatal": the end users couldn't continue if they see this fatal message, no guideline yet and it's nearly impossible for an end user to separate the files into correct directories, the end user will be blocked and give up?

---

If it needs to keep 1.20 using legacy behavior, there could be a more strict change:

1. Check if the [storage].PATH is set, if no, no problem
2. If [storage].PATH is set, tell user that the serious problem and:
   • Let them set another option like I_KNOW_STORAGE_PATH_MIXTURE_DANGEROUS=NoSupportIn1.21
   • Disable the DeleteRepositoryArchives (it is the only Clean Storage caller)
   • Then they can bypass the fatal to continue, until 1.21 comes. (TBH, I think they should create a new clear instance in this case, otherwise it's impossible for them to upgrade)

Concern resolved

[lunny]

I think this PR could work well with #26318 but not without it. Maybe it should also be sent to main branch?

[wxiaoguang]

I think this PR could work well with #26318 but not without it. Maybe it should also be sent to main branch?

You misunderstood, this PR doesn't work with #26318 for existing users who set [storage].PATH manually.

[lunny]

[repository].ROOT

Yes. The path [repository].ROOT should also be considered and also all other PATH configurations.

[delvh]

So, what do we do with this PR?
Merge or no merge?

[lunny]

I would like to merge but not revert #26318 and if we can include other storages into the map(git storage path and etc.), that's better.

[delvh]

Yes, I've canceled the revert: #26381

[GiteaBot]

@jolheiser please fix the merge conflicts. 🍵

[techknowlogick]

Should this be closed now that the other PR was merged?

[lunny]

Should this be closed now that the other PR was merged?

I think this could be replaced by #26484