-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Fix to use only needed columns from tables to get repository git paths #3870
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,6 +163,7 @@ func NewRepoContext() { | |
type Repository struct { | ||
ID int64 `xorm:"pk autoincr"` | ||
OwnerID int64 `xorm:"UNIQUE(s)"` | ||
OwnerName string `xorm:"-"` | ||
Owner *User `xorm:"-"` | ||
LowerName string `xorm:"UNIQUE(s) INDEX NOT NULL"` | ||
Name string `xorm:"INDEX NOT NULL"` | ||
|
@@ -225,9 +226,17 @@ func (repo *Repository) MustOwner() *User { | |
return repo.mustOwner(x) | ||
} | ||
|
||
// MustOwnerName always returns valid owner name to avoid | ||
// conceptually impossible error handling. | ||
// It returns "error" and logs error details when error | ||
// occurs. | ||
func (repo *Repository) MustOwnerName() string { | ||
return repo.mustOwnerName(x) | ||
} | ||
|
||
// FullName returns the repository full name | ||
func (repo *Repository) FullName() string { | ||
return repo.MustOwner().Name + "/" + repo.Name | ||
return repo.MustOwnerName() + "/" + repo.Name | ||
} | ||
|
||
// HTMLURL returns the repository HTML URL | ||
|
@@ -479,6 +488,41 @@ func (repo *Repository) mustOwner(e Engine) *User { | |
return repo.Owner | ||
} | ||
|
||
func (repo *Repository) getOwnerName(e Engine) error { | ||
if len(repo.OwnerName) > 0 { | ||
return nil | ||
} | ||
|
||
if repo.Owner != nil { | ||
repo.OwnerName = repo.Owner.Name | ||
return nil | ||
} | ||
|
||
u := new(User) | ||
has, err := e.ID(repo.OwnerID).Cols("name").Get(u) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lunny do you want me to submit PR to change this to master first? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean something like:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lafriks the currently code is OK. I only want to say there is another way to do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, thanks, I will keep in mind that |
||
if err != nil { | ||
return err | ||
} else if !has { | ||
return ErrUserNotExist{repo.OwnerID, "", 0} | ||
} | ||
repo.OwnerName = u.Name | ||
return nil | ||
} | ||
|
||
// GetOwnerName returns the repository owner name | ||
func (repo *Repository) GetOwnerName() error { | ||
return repo.getOwnerName(x) | ||
} | ||
|
||
func (repo *Repository) mustOwnerName(e Engine) string { | ||
if err := repo.getOwnerName(e); err != nil { | ||
log.Error(4, "Error loading repository owner name: %v", err) | ||
return "error" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It already returns owner name |
||
} | ||
|
||
return repo.OwnerName | ||
} | ||
|
||
// ComposeMetas composes a map of metas for rendering external issue tracker URL. | ||
func (repo *Repository) ComposeMetas() map[string]string { | ||
unit, err := repo.GetUnit(UnitTypeExternalTracker) | ||
|
@@ -590,7 +634,7 @@ func (repo *Repository) GetBaseRepo() (err error) { | |
} | ||
|
||
func (repo *Repository) repoPath(e Engine) string { | ||
return RepoPath(repo.mustOwner(e).Name, repo.Name) | ||
return RepoPath(repo.mustOwnerName(e), repo.Name) | ||
} | ||
|
||
// RepoPath returns the repository path | ||
|
@@ -2141,7 +2185,7 @@ func ReinitMissingRepositories() error { | |
// SyncRepositoryHooks rewrites all repositories' pre-receive, update and post-receive hooks | ||
// to make sure the binary and custom conf path are up-to-date. | ||
func SyncRepositoryHooks() error { | ||
return x.Where("id > 0").Iterate(new(Repository), | ||
return x.Cols("owner_id", "name").Where("id > 0").Iterate(new(Repository), | ||
func(idx int, bean interface{}) error { | ||
if err := createDelegateHooks(bean.(*Repository).RepoPath()); err != nil { | ||
return fmt.Errorf("SyncRepositoryHook: %v", err) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than a slight increase in memory usage, I don't see any point in only loading the name. The database still has to find and fetch the entire row 🤔 I would change this to just
mustOwner()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's needed because it is used in migrations and can be that struct has more fields than database has so there will be error in select that database does not contain such column