Refactor codebase for Phase 3
This commit is contained in:
@@ -0,0 +1,646 @@
|
||||
# Phase 3 Plan: Split the God File into an Organized Codebase
|
||||
|
||||
## Goal
|
||||
|
||||
Refactor the current monolithic `cmd/gitocean/main.go` into a maintainable, testable, multi-package Go codebase without changing user-facing behavior.
|
||||
|
||||
Phase 3 is primarily a structural refactor. The CLI commands, API routes, Git HTTP behavior, MySQL schema, storage layout, and auth rules should continue to work exactly as they do after Phase 2.
|
||||
|
||||
Phase 3 should also finish documenting and preserving the CLI UX direction from Phase 2: human-readable CLI output by default, with JSON available only as an explicit opt-in where useful. This does **not** mean removing JSON from the HTTP API; API requests and responses should remain JSON.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- Do not add a Web UI.
|
||||
- Do not add SSH Git transport.
|
||||
- Do not replace MySQL.
|
||||
- Do not redesign API routes unless a compatibility shim is kept.
|
||||
- Do not introduce large frameworks.
|
||||
- Do not implement new product features until the refactor is stable.
|
||||
- Do not remove JSON from the HTTP API. Only CLI output should become human-readable by default.
|
||||
|
||||
## Current problem
|
||||
|
||||
`cmd/gitocean/main.go` currently contains everything:
|
||||
|
||||
- Type definitions
|
||||
- CLI command routing and command implementations
|
||||
- HTTP server bootstrapping
|
||||
- API route dispatch
|
||||
- Auth/token logic
|
||||
- User/repository/PR/collaborator handlers
|
||||
- Git HTTP backend integration
|
||||
- MySQL migrations and DB helpers
|
||||
- Config loading/saving
|
||||
- Git helper functions
|
||||
- Backup/restore logic
|
||||
- Output formatting
|
||||
- Generic HTTP JSON helpers
|
||||
|
||||
This makes the code hard to test, hard to extend, and risky to modify.
|
||||
|
||||
## Refactor principles
|
||||
|
||||
1. **No behavior changes first**
|
||||
- Move code into packages with minimal edits.
|
||||
- Keep route paths, JSON payloads, CLI command names, flags, and output stable unless explicitly noted.
|
||||
|
||||
2. **Small, reviewable steps**
|
||||
- Move one domain at a time.
|
||||
- Run `gofmt`, `go test ./...`, and `go vet ./...` after each major move.
|
||||
|
||||
3. **Human-readable CLI by default**
|
||||
- During CLI extraction, audit commands that still print raw JSON.
|
||||
- Convert default CLI output to concise human-readable text/tables.
|
||||
- Keep `--json` as an explicit escape hatch for scripting where appropriate.
|
||||
- Do not change API JSON payloads.
|
||||
|
||||
4. **Thin `main` package**
|
||||
- `cmd/gitocean/main.go` should only call into an application package.
|
||||
|
||||
5. **Reusable packages**
|
||||
- Shared utilities should live in `internal/` packages, not in `cmd/`.
|
||||
|
||||
6. **Domain boundaries over technical dumping grounds**
|
||||
- Prefer packages like `repos`, `pulls`, `auth`, and `gitserver` over one huge `utils` package.
|
||||
|
||||
7. **Testable dependencies**
|
||||
- Handlers and services should accept dependencies through structs.
|
||||
- Avoid package-level global state except constants and regex validators.
|
||||
|
||||
## Target directory layout
|
||||
|
||||
```text
|
||||
cmd/
|
||||
gitocean/
|
||||
main.go # Tiny entrypoint only
|
||||
|
||||
internal/
|
||||
app/
|
||||
app.go # Top-level CLI dispatch / application orchestration
|
||||
usage.go # Help text
|
||||
|
||||
config/
|
||||
client.go # CLI config: ~/.config/gitocean/config.json
|
||||
server.go # Server config: storage/config.json
|
||||
env.go # Environment defaults
|
||||
|
||||
db/
|
||||
mysql.go # MySQL open/create database logic
|
||||
migrate.go # Migrations
|
||||
|
||||
model/
|
||||
user.go
|
||||
repository.go
|
||||
pull_request.go
|
||||
collaborator.go
|
||||
token.go
|
||||
ref.go
|
||||
|
||||
httpapi/
|
||||
server.go # Server struct and ServeHTTP
|
||||
router.go # API route dispatch
|
||||
json.go # decodeJSON/writeJSON/writeError
|
||||
|
||||
auth/
|
||||
handlers.go # register/login/logout/me handlers
|
||||
service.go # token generation, hashing, bearer/basic auth
|
||||
password.go # password hashing helpers if needed
|
||||
|
||||
tokens/
|
||||
handlers.go # token list/revoke/prune handlers
|
||||
|
||||
admin/
|
||||
handlers.go # admin users/repos/storage/token routes
|
||||
|
||||
repos/
|
||||
handlers.go # repo create/get/update/delete/search/fork
|
||||
service.go # repo access checks and repo loading
|
||||
collaborators.go # collaborator handlers and role logic
|
||||
validate.go # repo/user name validation helpers if not shared
|
||||
|
||||
pulls/
|
||||
handlers.go # PR create/list/view/close/merge/diff/comments
|
||||
service.go # PR loading, scanning, merge/diff helpers
|
||||
|
||||
gitserver/
|
||||
http.go # Smart HTTP route parsing and permissions
|
||||
backend.go # git http-backend CGI bridge
|
||||
|
||||
gitutil/
|
||||
git.go # runGit, gitInitBare, gitBranchExists, gitRefs
|
||||
|
||||
cli/
|
||||
root.go # CLI command dispatch
|
||||
auth.go # register/login/logout/whoami
|
||||
repo.go # repo command dispatch
|
||||
repo_create.go
|
||||
repo_publish.go
|
||||
repo_view.go
|
||||
repo_collaborators.go
|
||||
pr.go
|
||||
token.go
|
||||
admin.go
|
||||
backup.go
|
||||
output.go # CLI formatting helpers
|
||||
parse.go # splitOwnerRepo, splitRepoBranch, flag parsing
|
||||
prompt.go
|
||||
api_client.go # CLI HTTP client helper
|
||||
|
||||
backup/
|
||||
backup.go # createBackup/restoreBackup/mysqlCLIArgs/copyDir
|
||||
|
||||
validate/
|
||||
names.go # username/repo/branch validation and reserved names
|
||||
```
|
||||
|
||||
The exact final package names can change during implementation, but the direction should stay the same: small domain packages with clear responsibilities.
|
||||
|
||||
## Dependency direction
|
||||
|
||||
Recommended dependency flow:
|
||||
|
||||
```text
|
||||
cmd/gitocean
|
||||
-> internal/app
|
||||
-> internal/cli
|
||||
-> internal/httpapi
|
||||
-> internal/config
|
||||
-> internal/db
|
||||
|
||||
httpapi
|
||||
-> auth, tokens, admin, repos, pulls, gitserver
|
||||
-> model
|
||||
|
||||
repos/pulls/auth/etc.
|
||||
-> model
|
||||
-> gitutil where needed
|
||||
-> validate where needed
|
||||
|
||||
cli
|
||||
-> config
|
||||
-> model
|
||||
-> backup where needed
|
||||
```
|
||||
|
||||
Avoid circular dependencies by keeping shared types in `internal/model` and shared helpers in targeted utility packages.
|
||||
|
||||
## Proposed milestones
|
||||
|
||||
### Milestone 1: Prepare shared model and validation packages
|
||||
|
||||
Move pure data/types and validators first.
|
||||
|
||||
Create:
|
||||
|
||||
- `internal/model`
|
||||
- `internal/validate`
|
||||
|
||||
Move:
|
||||
|
||||
- `User`
|
||||
- `Repository`
|
||||
- `PullRequest`
|
||||
- `ServerConfig`
|
||||
- `RefInfo`
|
||||
- `Collaborator`
|
||||
- `PRComment`
|
||||
- `TokenInfo`
|
||||
- username/repo/branch regex validation
|
||||
- `isReservedName`
|
||||
|
||||
Acceptance:
|
||||
|
||||
- `cmd/gitocean/main.go` still builds.
|
||||
- All references use `model.User`, `model.Repository`, etc.
|
||||
- `go test ./...` passes.
|
||||
|
||||
### Milestone 2: Extract config and generic helpers
|
||||
|
||||
Create:
|
||||
|
||||
- `internal/config`
|
||||
- `internal/httpapi/json.go`
|
||||
- `internal/gitutil`
|
||||
|
||||
Move:
|
||||
|
||||
- CLI config path/load/save
|
||||
- server config path/load/save
|
||||
- env server URL helper
|
||||
- JSON request/response helpers where appropriate
|
||||
- `gitInitBare`
|
||||
- `gitBranchExists`
|
||||
- `gitRefs`
|
||||
- `runGit`
|
||||
|
||||
Acceptance:
|
||||
|
||||
- No behavior changes.
|
||||
- CLI login still saves config.
|
||||
- Server still reads config and env overrides.
|
||||
- Git helper functions are reusable outside `main`.
|
||||
|
||||
### Milestone 3: Extract DB connection and migrations
|
||||
|
||||
Create:
|
||||
|
||||
- `internal/db`
|
||||
|
||||
Move:
|
||||
|
||||
- `openMySQLAndCreateDatabaseIfMissing`
|
||||
- `quoteMySQLIdentifier`
|
||||
- `migrate`
|
||||
- `isDuplicateColumnError`
|
||||
|
||||
Acceptance:
|
||||
|
||||
- Server starts from empty DB.
|
||||
- Missing database auto-create behavior still works.
|
||||
- Migrations are isolated and testable.
|
||||
|
||||
### Milestone 4: Extract HTTP API server shell
|
||||
|
||||
Create:
|
||||
|
||||
- `internal/httpapi/server.go`
|
||||
- `internal/httpapi/router.go`
|
||||
|
||||
Move:
|
||||
|
||||
- `Server` struct
|
||||
- `ServeHTTP`
|
||||
- top-level API route dispatch
|
||||
- subroute dispatch helpers
|
||||
|
||||
Keep domain handler method bodies temporarily in `httpapi` if needed, then move them by domain in later milestones.
|
||||
|
||||
Acceptance:
|
||||
|
||||
- `runServer` constructs `httpapi.Server`.
|
||||
- API route paths remain unchanged.
|
||||
- Git HTTP routes still reach the backend.
|
||||
|
||||
### Milestone 5: Extract auth and token domains
|
||||
|
||||
Create:
|
||||
|
||||
- `internal/auth`
|
||||
- `internal/tokens`
|
||||
|
||||
Move:
|
||||
|
||||
- register/login/logout/me handlers
|
||||
- token creation/hash helpers
|
||||
- bearer/basic auth helpers
|
||||
- token list/revoke/prune handlers
|
||||
- direct admin-user creation helper used by `init`
|
||||
|
||||
Acceptance:
|
||||
|
||||
- Registration still makes the first user admin.
|
||||
- Login still returns 7-day tokens.
|
||||
- CLI login/whoami/logout still work.
|
||||
- Git basic auth still works with username/token.
|
||||
|
||||
### Milestone 6: Extract repository domain
|
||||
|
||||
Create:
|
||||
|
||||
- `internal/repos`
|
||||
|
||||
Move:
|
||||
|
||||
- repo create/get/update/delete/search/fork handlers
|
||||
- repository load/scan helpers
|
||||
- repo access helpers:
|
||||
- `requireReadableRepo`
|
||||
- `canReadRepo`
|
||||
- `canWriteRepo`
|
||||
- `collaboratorRole`
|
||||
- collaborator list/add/remove handlers
|
||||
|
||||
Acceptance:
|
||||
|
||||
- Public/private visibility rules still work.
|
||||
- Private collaborators can read when allowed.
|
||||
- Write collaborators can push when allowed.
|
||||
- Archived repositories still reject pushes.
|
||||
- Repo CLI commands still work.
|
||||
|
||||
### Milestone 7: Extract pull request domain
|
||||
|
||||
Create:
|
||||
|
||||
- `internal/pulls`
|
||||
|
||||
Move:
|
||||
|
||||
- PR create/list/view/close/merge handlers
|
||||
- PR comments handlers
|
||||
- PR diff handler
|
||||
- `prSelectSQL`
|
||||
- `loadPR`
|
||||
- `scanOnePR`
|
||||
- `scanPRs`
|
||||
- `mergePR`
|
||||
- `prDiff`
|
||||
|
||||
Acceptance:
|
||||
|
||||
- Same-repo PRs still work.
|
||||
- Cross-repo PRs still work.
|
||||
- PR merge still uses Git commands correctly.
|
||||
- PR diff/comments endpoints still work.
|
||||
|
||||
### Milestone 8: Extract Git Smart HTTP server
|
||||
|
||||
Create:
|
||||
|
||||
- `internal/gitserver`
|
||||
|
||||
Move:
|
||||
|
||||
- `handleGitHTTP`
|
||||
- `parseGitPath`
|
||||
- `gitService`
|
||||
- `runGitHTTPBackend`
|
||||
- `writeCGIResponse`
|
||||
|
||||
Design note:
|
||||
|
||||
`gitserver` should depend on a small permission interface instead of importing the entire repo handler package if possible:
|
||||
|
||||
```go
|
||||
type RepoAccess interface {
|
||||
LoadRepo(owner, name string) (model.Repository, error)
|
||||
CanReadRepo(repo model.Repository, user model.User, authed bool) bool
|
||||
CanWriteRepo(repo model.Repository, user model.User) bool
|
||||
UserFromBasic(r *http.Request) (model.User, bool)
|
||||
}
|
||||
```
|
||||
|
||||
Acceptance:
|
||||
|
||||
- Public clone/fetch still works without auth.
|
||||
- Private clone/fetch works for owner/collaborators only.
|
||||
- Push requires auth.
|
||||
- Archived repos reject pushes.
|
||||
- Git HTTP backend response handling remains correct.
|
||||
|
||||
### Milestone 9: Extract CLI package
|
||||
|
||||
Create:
|
||||
|
||||
- `internal/cli`
|
||||
|
||||
Move:
|
||||
|
||||
- command dispatch
|
||||
- `usage`
|
||||
- `cliInit`
|
||||
- auth CLI commands
|
||||
- repo CLI commands
|
||||
- PR CLI commands
|
||||
- token/admin/backup CLI commands
|
||||
- CLI API request helper
|
||||
- output formatting helpers
|
||||
- prompt helpers
|
||||
- parsing helpers
|
||||
- Git credential approval helper
|
||||
|
||||
Acceptance:
|
||||
|
||||
- `cmd/gitocean/main.go` is reduced to roughly:
|
||||
|
||||
```go
|
||||
package main
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"os"
|
||||
|
||||
"gitocean/internal/app"
|
||||
)
|
||||
|
||||
func main() {
|
||||
if err := app.Run(os.Args[1:]); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- Every existing CLI command still works.
|
||||
- Help text is unchanged except for intentional wording cleanup.
|
||||
|
||||
### Milestone 10: Extract backup package
|
||||
|
||||
Create:
|
||||
|
||||
- `internal/backup`
|
||||
|
||||
Move:
|
||||
|
||||
- `createBackup`
|
||||
- `restoreBackup`
|
||||
- `mysqlCLIArgs`
|
||||
- `copyDir`
|
||||
|
||||
Acceptance:
|
||||
|
||||
- `gitocean backup create FILE` still works.
|
||||
- `gitocean backup restore FILE` still works.
|
||||
- Backup logic is testable independently.
|
||||
|
||||
### Milestone 11: Add package-level tests
|
||||
|
||||
Add focused unit tests where possible.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- `internal/validate`
|
||||
- username validation
|
||||
- repo name validation
|
||||
- branch validation
|
||||
- reserved names
|
||||
|
||||
- `internal/cli`
|
||||
- `splitOwnerRepo`
|
||||
- `splitRepoBranch`
|
||||
- `parseRepoCreateArgs`
|
||||
- `parseRepoPublishArgs`
|
||||
- boolean flag removal
|
||||
|
||||
- `internal/config`
|
||||
- client config load/save round trip
|
||||
- server config load/save round trip
|
||||
- env override behavior
|
||||
|
||||
- `internal/gitserver`
|
||||
- `parseGitPath`
|
||||
- `gitService`
|
||||
- CGI response parsing
|
||||
|
||||
- `internal/db`
|
||||
- MySQL identifier quoting
|
||||
|
||||
Acceptance:
|
||||
|
||||
- `go test ./...` includes meaningful tests.
|
||||
- Tests avoid requiring a live MySQL instance unless explicitly integration-tagged.
|
||||
|
||||
### Milestone 12: CLI output audit
|
||||
|
||||
After the CLI package has been extracted, audit every CLI command for output style.
|
||||
|
||||
Commands should default to human-readable output:
|
||||
|
||||
- Short success messages for create/update/delete actions.
|
||||
- Tables for lists.
|
||||
- Labeled fields for detail views.
|
||||
- Helpful next-step commands where useful.
|
||||
- Clear auth/session error messages.
|
||||
|
||||
JSON should be retained only as an explicit opt-in for scripting, for example:
|
||||
|
||||
```bash
|
||||
gitocean repo view OWNER/REPO --json
|
||||
gitocean repo search QUERY --json
|
||||
gitocean pr view OWNER/REPO NUMBER --json
|
||||
gitocean token list --json
|
||||
```
|
||||
|
||||
Acceptance:
|
||||
|
||||
- No normal CLI command prints raw JSON by default.
|
||||
- Commands that are useful in scripts have a documented `--json` option.
|
||||
- HTTP API responses remain JSON and are not changed by this milestone.
|
||||
|
||||
### Milestone 13: Documentation cleanup
|
||||
|
||||
Update docs/plans after the refactor:
|
||||
|
||||
- Update `plans/PLAN.md` if architecture descriptions changed.
|
||||
- Update `plans/PHASE_2_PLAN.md` if current layout references are stale.
|
||||
- Add a short `README.md` if missing with:
|
||||
- quickstart
|
||||
- server setup
|
||||
- CLI commands
|
||||
- storage layout
|
||||
- development workflow
|
||||
|
||||
Acceptance:
|
||||
|
||||
- New contributors can find where CLI, API, DB, Git HTTP, and models live.
|
||||
|
||||
## Suggested implementation order
|
||||
|
||||
Recommended commit sequence:
|
||||
|
||||
1. `Add Phase 3 refactor plan`
|
||||
2. `Move shared models and validators`
|
||||
3. `Extract config and Git utilities`
|
||||
4. `Extract database setup and migrations`
|
||||
5. `Extract HTTP API server shell`
|
||||
6. `Extract auth and token handlers`
|
||||
7. `Extract repository handlers`
|
||||
8. `Extract pull request handlers`
|
||||
9. `Extract Git HTTP backend`
|
||||
10. `Extract CLI commands`
|
||||
11. `Extract backup utilities`
|
||||
12. `Audit CLI output and JSON escape hatches`
|
||||
13. `Add package-level tests and documentation`
|
||||
|
||||
## Compatibility checklist
|
||||
|
||||
After each milestone, run:
|
||||
|
||||
```bash
|
||||
gofmt -w .
|
||||
go test ./...
|
||||
go vet ./...
|
||||
```
|
||||
|
||||
Before declaring Phase 3 complete, manually verify:
|
||||
|
||||
```bash
|
||||
gitocean init
|
||||
gitocean server --config storage/config.json
|
||||
gitocean register
|
||||
gitocean login
|
||||
gitocean whoami
|
||||
gitocean repo create demo --public
|
||||
gitocean repo view USER/demo
|
||||
gitocean repo branches USER/demo
|
||||
gitocean repo tags USER/demo
|
||||
gitocean repo search demo --all
|
||||
gitocean clone USER/demo
|
||||
gitocean repo publish demo2 --public
|
||||
gitocean repo fork USER/demo
|
||||
gitocean pr create --from USER/demo-fork:main --to USER/demo:main --title "Test PR"
|
||||
gitocean pr list USER/demo
|
||||
gitocean pr view USER/demo 1
|
||||
gitocean pr diff USER/demo 1
|
||||
gitocean pr comment USER/demo 1 "Looks good"
|
||||
gitocean pr comments USER/demo 1
|
||||
gitocean token list
|
||||
gitocean admin users list
|
||||
gitocean backup create backup.tar.gz
|
||||
```
|
||||
|
||||
Also verify Git HTTP manually:
|
||||
|
||||
```bash
|
||||
git clone http://localhost:8080/USER/demo.git
|
||||
cd demo
|
||||
echo test >> README.md
|
||||
git add README.md
|
||||
git commit -m "test push"
|
||||
git push origin main
|
||||
```
|
||||
|
||||
## Risks and mitigations
|
||||
|
||||
### Risk: Circular package dependencies
|
||||
|
||||
Mitigation:
|
||||
|
||||
- Put shared structs in `internal/model`.
|
||||
- Put small interfaces between domains where needed.
|
||||
- Keep HTTP response helpers separate from domain services.
|
||||
|
||||
### Risk: Refactor changes behavior accidentally
|
||||
|
||||
Mitigation:
|
||||
|
||||
- Move code first, improve code second.
|
||||
- Keep commits small.
|
||||
- Add tests for parsing/routing helpers early.
|
||||
|
||||
### Risk: Handlers still know too much about SQL
|
||||
|
||||
Mitigation:
|
||||
|
||||
- Phase 3 can keep SQL in domain packages.
|
||||
- A later phase can introduce repository/store interfaces if needed.
|
||||
|
||||
### Risk: `utils` package becomes another god package
|
||||
|
||||
Mitigation:
|
||||
|
||||
- Only use utility packages for genuinely cross-domain helpers.
|
||||
- Prefer domain packages.
|
||||
|
||||
## Phase 3 completion criteria
|
||||
|
||||
Phase 3 is complete when:
|
||||
|
||||
- `cmd/gitocean/main.go` is a thin entrypoint.
|
||||
- No package contains unrelated CLI, API, DB, Git, and model code together.
|
||||
- Domain packages have clear responsibilities.
|
||||
- Existing CLI commands and API routes still work.
|
||||
- CLI output is human-readable by default, with `--json` opt-in where useful.
|
||||
- `go test ./...` passes.
|
||||
- `go vet ./...` passes.
|
||||
- Meaningful unit tests exist for parsing, validation, config, and Git HTTP helpers.
|
||||
Reference in New Issue
Block a user