Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor repo.isBare to repo.isEmpty #5629 #5714

Merged

Conversation

zeripath
Copy link
Contributor

Gitea's models/repo.go Repository.IsBare means that a repository is empty rather than that the repository is actually a bare repository in git terms. This is confusing and this PR changes Gitea so that all references to bare only refer to git bare.

Fix #5629

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

@codecov-io
Copy link

codecov-io commented Jan 13, 2019

Codecov Report

Merging #5714 into master will decrease coverage by 0.02%.
The diff coverage is 38.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5714      +/-   ##
==========================================
- Coverage   37.76%   37.73%   -0.03%     
==========================================
  Files         326      327       +1     
  Lines       47779    47803      +24     
==========================================
- Hits        18043    18038       -5     
- Misses      27146    27170      +24     
- Partials     2590     2595       +5
Impacted Files Coverage Δ
models/migrations/migrations.go 2.61% <ø> (ø) ⬆️
routers/repo/view.go 47.3% <0%> (+1.19%) ⬆️
routers/repo/pull.go 34.48% <0%> (ø) ⬆️
modules/context/api.go 50.81% <0%> (ø) ⬆️
models/migrations/v78.go 0% <0%> (ø)
routers/api/v1/repo/file.go 22.8% <0%> (ø) ⬆️
routers/repo/repo.go 20.82% <100%> (ø) ⬆️
models/action.go 60.32% <33.33%> (ø) ⬆️
modules/context/repo.go 59.78% <50%> (ø) ⬆️
models/repo.go 43.88% <57.14%> (ø) ⬆️
... and 6 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 9edc829...1a0f2f0. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 13, 2019
@techknowlogick techknowlogick added this to the 1.8.0 milestone Jan 13, 2019
@techknowlogick techknowlogick added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jan 13, 2019
@zeripath zeripath changed the title Migrate repo.isBare to repo.isEmpty #5629 Refactor repo.isBare to repo.isEmpty #5629 Jan 15, 2019
@zeripath zeripath force-pushed the issue-5629-refactor-isBare-to-isEmpty branch from 61a5ab6 to b255a04 Compare January 15, 2019 20:47
return err
}

_, err := sess.Query("UPDATE `repository` SET `is_empty` = `is_bare`")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just putting a note here in case we have problems in the future, I've tested this statement sucessfuly on SQLite, and MySQL, we probably won't run into problems with this raw SQL on Postgres, and hopefully we won't run into it on MSSQL (can't test that DB right now sadly).

If errors do arise from this statement, then we'll have to select rows in batches and loop while doing updates on each using XORM Update function.

@bkcsoft bkcsoft 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 Jan 15, 2019
IsEmpty bool `xorm:"INDEX"`
}

if err := x.Sync(new(Repository)); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just rename column instead of adding/removing?

ALTER TABLE "repository" RENAME COLUMN "is_bare" TO "is_empty";

for MSSQL you have to use:

EXEC sp_rename 'repository.is_bare', 'is_empty', 'COLUMN'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the line

strings.Contains(err.Error(), "no such column") 

still correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hard to tell, need to verify that. Also x.Sync is not needed any more otherwise it will add column and rename will fail

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath force-pushed the issue-5629-refactor-isBare-to-isEmpty branch from 7e379a0 to c4884ae Compare January 15, 2019 22:14
@bkcsoft bkcsoft 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 Jan 17, 2019
@techknowlogick techknowlogick merged commit 07802a2 into go-gitea:master Jan 18, 2019
@zeripath zeripath deleted the issue-5629-refactor-isBare-to-isEmpty branch January 23, 2019 20:22
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gitea's models/repo.go Repository.IsBare means that a repository is Empty
6 participants