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

Simplify how git repositories are opened #28937

Merged
merged 22 commits into from
Jan 27, 2024

Conversation

lunny
Copy link
Member

@lunny lunny commented Jan 26, 2024

Purpose

This is a refactor toward building an abstraction over managing git repositories.
Afterwards, it does not matter anymore if they are stored on the local disk or somewhere remote.

What this PR changes

We used git.OpenRepository everywhere previously.
Now, we should split them into two distinct functions:

Firstly, there are temporary repositories which do not change:

git.OpenRepository(ctx, diskPath)

Gitea managed repositories having a record in the database in the repository table are moved into the new package gitrepo:

gitrepo.OpenRepository(ctx, repo_model.Repo)

Why is repo_model.Repository the second parameter instead of file path?
Because then we can easily adapt our repository storage strategy.
The repositories can be stored locally, however, they could just as well be stored on a remote server.

Further changes in other PRs

…epositories, use the method on repository module package. For others, use git.OpenRepository as before
@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jan 26, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 26, 2024
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 26, 2024
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin labels Jan 26, 2024
@lunny lunny changed the title Move open git repository method split from git package. For managed repositories, use the method on repository module package. For others, use git.OpenRepository as before Split open git repository methods. For managed repositories, use the method on repository module package. Jan 26, 2024
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 26, 2024
@lunny lunny requested a review from delvh January 26, 2024 05:41
@lunny lunny requested a review from wxiaoguang January 26, 2024 05:49
@wolfogre
Copy link
Member

I think it's a good idea to do this; however, I'm afraid it's not enough.

The package git also exposes other functions that accept paths, like:

  • GetBranchesByPath(ctx context.Context, path string, skip, limit int),
  • GetBranchCommitID(ctx context.Context, path, branch string)
  • RepositoryFromContextOrOpen(ctx context.Context, path string)

I believe all of them should be removed to prevent misuse and keep only git.OpenRepository(ctx, diskPath) for gitrepo.

@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 26, 2024
@lunny
Copy link
Member Author

lunny commented Jan 26, 2024

I think it's a good idea to do this; however, I'm afraid it's not enough.

The package git also exposes other functions that accept paths, like:

* `GetBranchesByPath(ctx context.Context, path string, skip, limit int)`,

* `GetBranchCommitID(ctx context.Context, path, branch string)`

* `RepositoryFromContextOrOpen(ctx context.Context, path string)`

I believe all of them should be removed to prevent misuse and keep only git.OpenRepository(ctx, diskPath) for gitrepo.

Done.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 27, 2024
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

I've no idea about whether the design is good enough, some points look really strange to me.

But at least, I do not think it is right to introduce a dependency between modules/gitrepo and models/repo. Isn't there already a guideline saying to avoid making modules calls models? To make the code maintainable, it could use some interfaces, eg: interface { GitRepoStoragePath() string }

modules/gitrepo/branch.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 27, 2024
@lunny
Copy link
Member Author

lunny commented Jan 27, 2024

@wxiaoguang @delvh Introduce an interface respository and removed the dependency on respotiroy package.

modules/gitrepo/gitrepo.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Jan 27, 2024
@lunny
Copy link
Member Author

lunny commented Jan 27, 2024

I've no idea about whether the design is good enough, some points look really strange to me.

But at least, I do not think it is right to introduce a dependency between modules/gitrepo and models/repo. Isn't there already a guideline saying to avoid making modules calls models? To make the code maintainable, it could use some interfaces, eg: interface { GitRepoStoragePath() string }

There is no design currently. It's just refactor to move all managed git repositories operations into the single package to make further implementation easier.

@GiteaBot GiteaBot 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 27, 2024
@6543 6543 merged commit 5f82ead into go-gitea:main Jan 27, 2024
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Jan 27, 2024
@lunny lunny deleted the lunny/move_openrepository branch January 28, 2024 01:45
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 29, 2024
* giteaofficial/main:
  [skip ci] Updated licenses and gitignores
  Fix bug for generated repository object format (go-gitea#28969)
  Fixing small space missing in sample config file (go-gitea#28967)
  Fix inconsistent naming of OAuth 2.0 `ENABLE` setting (go-gitea#28951)
  Add screenshot for "Profile Readmes" to docs (go-gitea#28964)
  Simplify how git repositories are opened (go-gitea#28937)
  Preserve BOM in web editor (go-gitea#28935)
  Make loading animation less aggressive (go-gitea#28955)
  Fix SSPI user creation (go-gitea#28948)
  Strip `/` from relative links (go-gitea#28932)
  Fix non-alphabetic sorting of repo topics (go-gitea#28938)
  Don't remove all mirror repository's releases when mirroring (go-gitea#28817)
  Use new RPM constants (go-gitea#28931)
  Check for sha256 support to use --object-format flag (go-gitea#28928)
  fix: update enable_prune even if mirror_interval is not provided (go-gitea#28905)
  Implement `MigrateRepository` for the actions notifier (go-gitea#28920)
  Respect branch info for relative links (go-gitea#28909)
henrygoodman pushed a commit to henrygoodman/gitea that referenced this pull request Jan 31, 2024
## Purpose
This is a refactor toward building an abstraction over managing git
repositories.
Afterwards, it does not matter anymore if they are stored on the local
disk or somewhere remote.

## What this PR changes
We used `git.OpenRepository` everywhere previously.
Now, we should split them into two distinct functions:

Firstly, there are temporary repositories which do not change:

```go
git.OpenRepository(ctx, diskPath)
```

Gitea managed repositories having a record in the database in the
`repository` table are moved into the new package `gitrepo`:

```go
gitrepo.OpenRepository(ctx, repo_model.Repo)
```

Why is `repo_model.Repository` the second parameter instead of file
path?
Because then we can easily adapt our repository storage strategy.
The repositories can be stored locally, however, they could just as well
be stored on a remote server.

## Further changes in other PRs
- A Git Command wrapper on package `gitrepo` could be created. i.e.
`NewCommand(ctx, repo_model.Repository, commands...)`. `git.RunOpts{Dir:
repo.RepoPath()}`, the directory should be empty before invoking this
method and it can be filled in the function only. go-gitea#28940
- Remove the `RepoPath()`/`WikiPath()` functions to reduce the
possibility of mistakes.

---------

Co-authored-by: delvh <dev.lh@web.de>
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
## Purpose
This is a refactor toward building an abstraction over managing git
repositories.
Afterwards, it does not matter anymore if they are stored on the local
disk or somewhere remote.

## What this PR changes
We used `git.OpenRepository` everywhere previously.
Now, we should split them into two distinct functions:

Firstly, there are temporary repositories which do not change:

```go
git.OpenRepository(ctx, diskPath)
```

Gitea managed repositories having a record in the database in the
`repository` table are moved into the new package `gitrepo`:

```go
gitrepo.OpenRepository(ctx, repo_model.Repo)
```

Why is `repo_model.Repository` the second parameter instead of file
path?
Because then we can easily adapt our repository storage strategy.
The repositories can be stored locally, however, they could just as well
be stored on a remote server.

## Further changes in other PRs
- A Git Command wrapper on package `gitrepo` could be created. i.e.
`NewCommand(ctx, repo_model.Repository, commands...)`. `git.RunOpts{Dir:
repo.RepoPath()}`, the directory should be empty before invoking this
method and it can be filled in the function only. go-gitea#28940
- Remove the `RepoPath()`/`WikiPath()` functions to reduce the
possibility of mistakes.

---------

Co-authored-by: delvh <dev.lh@web.de>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 5, 2024
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. modifies/api This PR adds API routes or modifies them modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. 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.

6 participants