Clarify the suffices and prefixes of setting.AppSubURL and setting.AppURL

[zeripath]

In more than a couple of places we're slightly confused about whether AppURL and AppSubURL have preceding or trailing slashes.

This PR adds comments to the definition of the setting.App*, removes an old unused setting variable, uses the clarity obtained to simplify several uses of AppURL and AppSubURL and fixes a potential issue with a missing QuoteMeta in regexp.

Signed-off-by: Andrew Thornton art27@cantab.net

[zeripath]

setting.AppSubURLDepth has been removed as it is not used.

[codecov-commenter]

Codecov Report

Merging #12999 into master will decrease coverage by 0.00%.
The diff coverage is 35.00%.

@@            Coverage Diff             @@
##           master   #12999      +/-   ##
==========================================
- Coverage   42.59%   42.59%   -0.01%     
==========================================
  Files         671      671              
  Lines       73625    73619       -6     
==========================================
- Hits        31363    31357       -6     
+ Misses      37183    37181       -2     
- Partials     5079     5081       +2     

Impacted Files	Coverage Δ
routers/admin/admin.go	11.45% <0.00%> (ø)
routers/api/v1/org/member.go	19.54% <0.00%> (+0.22%)	⬆️
routers/user/notification.go	40.17% <0.00%> (ø)
models/notification.go	65.76% <100.00%> (ø)
models/repo.go	55.15% <100.00%> (ø)
models/user.go	53.58% <100.00%> (ø)
modules/markup/html.go	78.61% <100.00%> (+0.26%)	⬆️
modules/setting/setting.go	47.50% <100.00%> (-0.12%)	⬇️
routers/repo/search.go	69.23% <100.00%> (-1.14%)	⬇️
modules/util/timer.go	42.85% <0.00%> (-42.86%)	⬇️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6da033...b2c29e2. Read the comment docs.

[lafriks]

Imho both appurl and appsuburl should end with / otherwise it will still be confusing and inconsistent

[zeripath]

The reason that AppSubURL doesn't have a terminal / is because it allows it to be empty when there is no suburl.

That's quite nice actually and leads to very compact relative URLs and easy handling of AppSubURL with relative addresses. (Just prefix them with AppSubURL and it will always be right.)

AppURL will have a terminal slash for other similar reasons in that some proxies will likely only proxy the slash terminal address and I suspect that means that it's easier to and simpler to just write AppURL instead of AppURL+"/"

This PRs intention was not change behaviour but rather to clarify what is actually happenin, but if we were to change anything I'd argue we should not have the terminal / on the AppURL.

That would make creating the "relpath" and making the absolute URL the same: clean the path to root it on the AppSubURL then either prefix with suburl or the URL as appropriate.

[lafriks]

I would prefer that non of them should have / at the end imho, this way AppURL + AppSubURL + /path is always valid

[zeripath]

I would prefer that non of them should have / at the end imho, this way AppURL + AppSubURL + /path is always valid

AppURL has to contain the AppSubURL so I think you mean AppURL + "/path".

[lafriks]

I would prefer that non of them should have / at the end imho, this way AppURL + AppSubURL + /path is always valid

AppURL has to contain the AppSubURL so I think you mean AppURL + "/path".

yes, I meant (AppURL || AppSubURL) + /path. So that you don't have to think if you need to add / before rest of url or not

[zeripath]

As a (partially relevant) aside:

Do you know if there is ever a reason for Gitea to be producing non-FQDN urls that point outside of its own root? My suspicion is that there is not and that there should not be.

[lafriks]

I don't know what did you mean by that but there should not be such urls

[codecov-io]

Codecov Report

Merging #12999 into master will increase coverage by 0.28%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master   #12999      +/-   ##
==========================================
+ Coverage   41.96%   42.24%   +0.28%     
==========================================
  Files         683      683              
  Lines       75279    75375      +96     
==========================================
+ Hits        31589    31843     +254     
+ Misses      38524    38306     -218     
- Partials     5166     5226      +60     

Impacted Files	Coverage Δ
routers/admin/admin.go	11.41% <0.00%> (ø)
routers/api/v1/org/member.go	19.54% <0.00%> (+0.22%)	⬆️
routers/repo/pull.go	33.29% <0.00%> (ø)
routers/user/notification.go	40.17% <0.00%> (ø)
models/notification.go	65.76% <100.00%> (ø)
models/repo.go	56.24% <100.00%> (ø)
models/user_avatar.go	31.57% <100.00%> (ø)
modules/markup/html.go	78.61% <100.00%> (+0.26%)	⬆️
modules/setting/setting.go	45.88% <100.00%> (-0.12%)	⬇️
routers/repo/search.go	69.23% <100.00%> (-1.14%)	⬇️
... and 20 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b50448b...a87d7e4. Read the comment docs.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

[6543]

@uli-heller would you like to test now ?

[uli-heller]

@6543 : I'll do so but I'm not sure it it will work out today. Still busy @work...

[6543]

🚀