Let package git depend on setting but not opposite

[lunny]

Package Setting should be dependent by package git but not opposite.

Extract from #15234 .

[zeripath]

I would argue we should be doing the opposite.

[lunny]

I would argue we should be doing the opposite.

Since most modules depend on setting, so that most modules will depend on package git if that's what you want.

[6543]

Unit tests fail related

[zeripath]

I think we should also rip out those dependencies too.

[lunny]

But then even gitea serv and other hooks subcommand will depend on all setting's dependent packages?

[6543]

I think setting package as dependeny is the way to go. You need one place to maintain settings ... doing this for each package on its own is stupid

[zeripath]

I think setting package as dependeny is the way to go. You need one place to maintain settings ... doing this for each package on its own is stupid

Wanna see stupid? Let's take a look at an example of what this causes:

https://github.com/go-gitea/gitea/blob/master/modules/setting/queue.go

So if I want to add a new Queue type I have change the setting module and the queues module and have this weird catch all configuration.

If the configuration for the Queue module was entirely within modules/queue then all I have to do is change modules/queue. The configurability components could simply be as much as init module that registers with the settings or configuration.

That leaves concerns like cross cutting configuration like setting.AppSubUrl and setting.AppURL - well - that this immediately causes problems tells us that there is a problem with our architecture. Temporarily these could go in to modules/setting/server etc. but we should consider the appropriateness of functions that like user.HTMLURL() - that's a routing function but it is in models.

There is a question of how you deal with reloadable configuration - well that requires a Manager-like model and a common interface within setting could be easily used.

This way we stop being so dependent go-ini - and tests become a lot easier.

[6543]

ok what you propose is a new architecture - and if we migrate to this, I totally agree on you opinion

[lunny]

@zeripath I don't think this PR against your new idea. And I think your model is very ideal but Golang don't support package reference each other. Some settings will only be used by models but not the packages under modules or services.

[codecov-commenter]

Codecov Report

Merging #15241 (5bf50a6) into main (e673e42) will increase coverage by 0.00%.
The diff coverage is 65.11%.

@@           Coverage Diff           @@
##             main   #15241   +/-   ##
=======================================
  Coverage   45.46%   45.47%           
=======================================
  Files         708      709    +1     
  Lines       83418    83418           
=======================================
+ Hits        37930    37932    +2     
  Misses      39416    39416           
+ Partials     6072     6070    -2     

Impacted Files	Coverage Δ
modules/git/repo_commit.go	40.86% <0.00%> (ø)
modules/setting/git.go	33.33% <ø> (-38.10%)	⬇️
modules/setting/lfs.go	56.52% <ø> (+3.58%)	⬆️
routers/api/v1/repo/commits.go	56.47% <0.00%> (ø)
routers/web/repo/wiki.go	41.42% <0.00%> (ø)
modules/git/lfs.go	53.84% <53.84%> (ø)
routers/web/repo/commit.go	28.94% <66.66%> (ø)
modules/git/git.go	44.85% <86.66%> (+4.64%)	⬆️
modules/git/command.go	91.66% <100.00%> (ø)
routers/init.go	59.42% <100.00%> (+0.59%)	⬆️
... and 4 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 e673e42...5bf50a6. Read the comment docs.

[zeripath]

I remain unconvinced of the wisdom of moving more things in to setting and in fact intend to make a refactor that will require mostly reverting this.

We should be using a Configure interface e.g.

setting.Configure(setting.Configurable) error

so that the explicit dependency on setting everywhere can eventually be dropped.

[6543]

@zeripath simething for v1.16 ...

[zeripath]

yes