Disallow duplicate storage paths

[lunny]

Replace #26380

[delvh]

Wait, we haven't merged this PR yet?

[GiteaBot]

I was unable to create a backport for 1.21. @lunny, please send one manually. 🍵

go run ./contrib/backport 26484
...  // fix git conflicts if any
go run ./contrib/backport --continue

[lunny]

Since it's not a bug, I will not send backport to 1.21.

[wxiaoguang]

1. I think it is breaking, it would stop some poor users from starting the instance.
2. What an end user could do if they see such fatal message? They are unable to use their instance anymore.

[wxiaoguang]

I would strongly suggest to revert this PR, if you really would like to check, you could check it in the "self check" page.

If you'd like to show "fatal" message, I think you should only do it if the directory is empty (say, an empty instance)

[lunny]

Or maybe we can use a warning instead of fatal here?

[delvh]

@wxiaoguang the problem I can see with that is that this misconfiguration can have dire consequences:
It can lead to end user data being irreversibly corrupted without knowing why.
I don't think it is right that Gitea lets users shoot themselves in the foot.
If it is only a warning for something this critical, it might be missed for a long time leading to more corrupted data.

In this case, I'd say the negative outcomes of allowing this bug to "silently" occur far outweigh the negative outcomes of giving new instance admins a fatal error to notice they need to fix it when updating.
As such, I'm in favor of keeping the fatal over a warning.
If the bug had a reversible effect, I'd agree with you.

[wxiaoguang]

Suppose I am an unlucky user: 1. I misconfigured my storage 2. I have data in the storage 3. I updated to a new Gitea 4. Then gitea shows the fatal message and won't start 5. My team are waiting for the git server to work Now, what's your suggestion about what I should do next? delvh ***@***.***>

…

@wxiaoguang <https://github.com/wxiaoguang> the problem I can see with that is that this misconfiguration can have dire consequences: It can lead to end user data being irreversibly corrupted without knowing why. I don't think it is right that Gitea lets users shoot themselves in the foot. If it is only a warning for something this critical, it might be missed for a long time leading to more corrupted data. In this case, I'd say the negative outcomes of allowing this bug to "silently" occur far outweigh the negative outcomes of giving new instance admins a fatal error to notice they need to fix it when updating. As such, I'm in favor of keeping the fatal over a warning. If the bug had a reversible effect, I'd agree with you

[delvh]

1. The number of affected users should be low as Gitea does not generate duplicate storage paths by default, and most often, people do not reuse the same directories for file storage
2. Then, define unlucky user here:
   If you're updating Gitea, you're not just a normal user but an admin.
   As such, you typically choose a time to update when there will not be much traffic and have defined an update time when the server will be unavailable.
3. Assuming you run into a problem while updating, if you are a good admin, you made a backup beforehand. If you are unable to update in time, roll back to the previous version and try to allocate more time to the update
4. Next, if you encounter this problem, you'll need to move all affected files to the new location, or you accept losing this data. Sometimes, this is an easy process that simply involves one mv *.<suffix> <new folder>, and sometimes you need to move these things manually. When in doubt about where a file belongs, then so is Gitea, so Gitea is already broken then.

I know, it is not the nicest option, but as I said, allowing broken Giteas only leads to more problems down the line instead of once when updating.

[delvh]

Additionally, as I just noticed, there is an easy workaround that does not lose any data:
if path $OLD is used at least twice, you can cp -r $OLD/* $NEW for every $NEW storage path.
While your disk usage might increase somewhat and some things might be accessible suddenly to outsiders that should not be accessible to them, you won't lose any data.

[wxiaoguang]

The root issue is # #26264 : incorrect config removes user files.

---

About the proposals for "helping unlucky users to fix the problem":

this is an easy process that simply involves one mv . , and sometimes you need to move these things manually.
Additionally, as I just noticed, there is an easy workaround that does not lose any data:
if path $OLD is used at least twice, you can cp -r $OLD/ $NEW for every $NEW storage path.

IIRC that's the same conclusion as the old discussion in discord and #26380 (comment) :

• It is impossible for end users to "mv" the related files to fix the "fatal".
• For the "cp" approach, it might work, at least there should be some documents to help the poor users .... but there is no document .....

Beside the documentation problem, there are more problems in this PR (why I suggested to revert, or improve):

• Incomplete, the real root problem is not "make sure path unique", but "avoid path overlap". Think about a case:
  • storage.a.path = "/data"
  • storage.b.path = "/data/sub"
  • It passes this PR's check, but it still causes problems (that's also my opinion in Storage paths must be unique #26380: "they shouldn't overlap in most cases")
• Improper fatal message. The log says app_work_path / "storage." + name, but app_work_path is not a config option. It would cause misuse or misunderstanding.
• Incorrect, because in some cases (eg: docker), the AppWorkPath is CustomPath, but this PR makes it "fatal"
  • Fix WORK_DIR for docker (root) image #25738
  • That's a quite old problem, not introduced by me ....
• No test ....

[wxiaoguang]

By using a docker image, you will see

gitea  | 2024/02/10 12:25:05 cmd/web.go:114:showWebStartupMessage() [I] * WorkPath: /data/gitea
gitea  | 2024/02/10 12:25:05 cmd/web.go:115:showWebStartupMessage() [I] * CustomPath: /data/gitea

So, WorkPath is CustomPath in docker.

But why the nightly doesn't fail? Because the code fatalDuplicatedPath("app_work_path", AppWorkPath) is also incorrect: it only reads the default AppWorkPath, but not the real AppWorkPath. So, incorrect code + incorrect code = the docker image still runs ....

[delvh]

Yep, you've got a point there.
If our docker image misbehaves like that, I can hardly disagree with you that the current fix is sufficient.

[wxiaoguang]

@lafriks @delvh what do you think about

• Revert "Disallow duplicate storage paths (#26484)" #29171

?

[github-actions]

Automatically locked because of our CONTRIBUTING guidelines