Remove repo unit index #2621

Morlinest · 2017-09-28T11:41:12Z

~~Fix Index values in RepoUnit.~~#2601 has to be merged first.~~

~~Edit: #2601 or #2603 can fix problems with accessing repo pages with this changes.~~

Removes unused RepoUnit.Index

codecov-io · 2017-09-28T11:56:51Z

Codecov Report

Merging #2621 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2621      +/-   ##
==========================================
+ Coverage   27.11%   27.11%   +<.01%     
==========================================
  Files          86       86              
  Lines       17064    17062       -2     
==========================================
  Hits         4627     4627              
+ Misses      11759    11757       -2     
  Partials      678      678

Impacted Files	Coverage Δ
models/repo_unit.go	`8% <ø> (ø)`	⬆️
models/repo.go	`13.14% <0%> (+0.01%)`	⬆️

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 46cc45f...72d14ed. Read the comment docs.

ethantkoenig · 2017-09-29T03:39:27Z

@Morlinest In the future, could you please not create PRs than depend on other, unmerged PRs? If you don't create the PR until the things it depends on are finished, that would make things a lot simpler for reviewers.

Morlinest · 2017-09-29T07:35:39Z

@ethantkoenig It doesn't "need" anything, I want to merge another PR only because there is another bug that will pop up after merge. I know it only because I've tested it and also know, there is already fix for it. I think #2603 will also fix it. I am writing comments about merges as bug prevention...

Edit: I think it's totally legal and common to reference other issues/pullrequests.

ethantkoenig · 2017-09-29T18:11:43Z

@Morlinest So this PR fixes a bug that does not currently exist, but will exist after #2601 is merged?

daviian · 2017-09-29T18:18:38Z

models/migrations/v45.go

+		UnitTypeReleases:        3,
+		UnitTypeWiki:            4,
+		UnitTypeExternalWiki:    4,
+	}


Why is this different than creating the RepoUnits!?
UnitTypeExternalWiki is here 4 but in when saving settings it is the Idx of models.Units, thus 6?
Definitely not correct, needs to be changed.

@daviian I think this is correct and doesn't need any changes. RepoUnit contains UnitType, see line below.
Please see "unit.go" file. It's copied from there. The only thing I did was to replace map[UnitType]Unit for map[UnitType]int to reduce number of lines added (around 60 lines) and copied only Unit.Idx values. All Index values in RepoUnit table with UnitTypeExternalWiki should be updated to 4 as it is in units map. Where do you see 6?

daviian · 2017-09-29T18:20:28Z

@ethantkoenig It is a bug, that has no visible impact currently, but the Index of RepoUnits is indeed set to a wrong value.

Morlinest · 2017-09-29T21:58:39Z

@ethantkoenig Please look at files changed. Do you see 2 different ways of setting Index value (first time as index from range -> 0 to x, second time as int value of UnitType -> 1 to x)? Read comments in #2603. None of the current ways is correct. So this PR fixes 3 bugs at once in code + set correct values in DB.

ethantkoenig · 2017-09-30T03:10:32Z

This may be a stupid question, but where exactly is the RepoUnit.Index field used? It is never read in the code (only populated in struct declarations), and it doesn't seem to be used anywhere in the HTML templates.

Also, if the index should be uniquely determined by the RepoUnit.Type field (as it is in this PR), do we even need to store in the database at all?

Morlinest · 2017-09-30T13:02:54Z

@ethantkoenig That's not a stupid question. As I wrote before in #2603

We can improve it later. And then we can just remove Index from DB as it will be useless.

it can be removed. But first we need to check code and rewrite Home handler at least. I was focused on making quick fix of bug instead. But maybe I can rewrite #2601 to fix UnitTypeCode check and also to use Unit.Idx for rest. This should be just fast fix, but now I have time to work on it.

Morlinest · 2017-09-30T20:01:48Z

@ethantkoenig Changed this PR to completly remove RepoUnit.Index instead of fixing it as #2601 is rewriten and it's not needed for anything.

@daviian With this two changes (if aprooved and merged) will not be #2603 needed anymore. Right?

lafriks · 2017-09-30T21:53:02Z

models/migrations/v45.go

+)
+
+func removeIndexColumnFromRepoUnitTable(x *xorm.Engine) (err error) {
+	if _, err := x.Exec("ALTER TABLE repo_unit DROP COLUMN index"); err != nil {


Columns can not be dropped on sqlite. Please add if to switch like in this migration:

gitea/models/migrations/v40.go

Line 43 in 1739e84

switch {

ethantkoenig · 2017-09-30T22:06:45Z

LGTM

lafriks · 2017-10-01T06:14:16Z

LGTM

lunny · 2017-10-01T14:15:20Z

conflicted

lafriks · 2017-10-01T15:21:26Z

@lunny fixed

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 28, 2017

Morlinest mentioned this pull request Sep 28, 2017

add sorting to database query of repo_units #2603

Closed

Morlinest force-pushed the fix/use-correct-repo-unit-index branch from 7fcd211 to c927767 Compare September 28, 2017 13:39

daviian suggested changes Sep 29, 2017

View reviewed changes

daviian approved these changes Sep 29, 2017

View reviewed changes

ethantkoenig mentioned this pull request Sep 30, 2017

Code/repo search #2582

Merged

Morlinest mentioned this pull request Sep 30, 2017

Fix implementation of repo Home func #2601

Merged

Morlinest force-pushed the fix/use-correct-repo-unit-index branch from c927767 to e64156a Compare September 30, 2017 19:47

Morlinest changed the title ~~Use Unit.Idx as repo unit index~~ Remove repo unit index Sep 30, 2017

Remove repo unit index

eff2b8f

Morlinest force-pushed the fix/use-correct-repo-unit-index branch from e64156a to eff2b8f Compare September 30, 2017 20:01

lafriks requested changes Sep 30, 2017

View reviewed changes

Fix sqlite

7b01b26

ethantkoenig approved these changes Sep 30, 2017

View reviewed changes

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 30, 2017

lafriks approved these changes Oct 1, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 1, 2017

Merge branch 'master' into fix/use-correct-repo-unit-index

88e3e9f

lafriks added 2 commits October 1, 2017 18:19

Merge branch 'master' into fix/use-correct-repo-unit-index

9706749

Fix dupplicate type after merge

988fc75

lafriks added 2 commits October 1, 2017 19:55

Merge branch 'master' into fix/use-correct-repo-unit-index

50dcee0

Merge branch 'master' into fix/use-correct-repo-unit-index

72d14ed

lafriks merged commit a04718a into go-gitea:master Oct 2, 2017

Morlinest deleted the fix/use-correct-repo-unit-index branch October 2, 2017 20:36

lafriks added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Oct 4, 2017

lafriks added this to the 1.3.0 milestone Oct 4, 2017

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove repo unit index #2621

Remove repo unit index #2621

Morlinest commented Sep 28, 2017 •

edited

Loading

codecov-io commented Sep 28, 2017 •

edited

Loading

ethantkoenig commented Sep 29, 2017 •

edited

Loading

Morlinest commented Sep 29, 2017 •

edited

Loading

ethantkoenig commented Sep 29, 2017 •

edited

Loading

daviian Sep 29, 2017

Morlinest Sep 29, 2017 •

edited

Loading

daviian commented Sep 29, 2017 •

edited

Loading

Morlinest commented Sep 29, 2017 •

edited

Loading

ethantkoenig commented Sep 30, 2017

Morlinest commented Sep 30, 2017

Morlinest commented Sep 30, 2017

lafriks Sep 30, 2017

Morlinest Sep 30, 2017

ethantkoenig commented Sep 30, 2017

lafriks commented Oct 1, 2017

lunny commented Oct 1, 2017

lafriks commented Oct 1, 2017

Remove repo unit index #2621

Remove repo unit index #2621

Conversation

Morlinest commented Sep 28, 2017 • edited Loading

codecov-io commented Sep 28, 2017 • edited Loading

Codecov Report

ethantkoenig commented Sep 29, 2017 • edited Loading

Morlinest commented Sep 29, 2017 • edited Loading

ethantkoenig commented Sep 29, 2017 • edited Loading

daviian Sep 29, 2017

Choose a reason for hiding this comment

Morlinest Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

daviian commented Sep 29, 2017 • edited Loading

Morlinest commented Sep 29, 2017 • edited Loading

ethantkoenig commented Sep 30, 2017

Morlinest commented Sep 30, 2017

Morlinest commented Sep 30, 2017

lafriks Sep 30, 2017

Choose a reason for hiding this comment

Morlinest Sep 30, 2017

Choose a reason for hiding this comment

ethantkoenig commented Sep 30, 2017

lafriks commented Oct 1, 2017

lunny commented Oct 1, 2017

lafriks commented Oct 1, 2017

Morlinest commented Sep 28, 2017 •

edited

Loading

codecov-io commented Sep 28, 2017 •

edited

Loading

ethantkoenig commented Sep 29, 2017 •

edited

Loading

Morlinest commented Sep 29, 2017 •

edited

Loading

ethantkoenig commented Sep 29, 2017 •

edited

Loading

Morlinest Sep 29, 2017 •

edited

Loading

daviian commented Sep 29, 2017 •

edited

Loading

Morlinest commented Sep 29, 2017 •

edited

Loading