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 git module, make Gitea use internal git config #19732

Merged
merged 32 commits into from
Jun 10, 2022
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
dac2594
Refactor git module, make Gitea use internal git config, add safe.dir…
wxiaoguang May 17, 2022
45a9375
introduce git.InitSimple and git.InitWithConfigSync, make serv cmd us…
wxiaoguang May 17, 2022
22d62bc
use HOME instead of GIT_CONFIG_GLOBAL, because git always needs a cor…
wxiaoguang May 17, 2022
b7f928c
fix cmd env in cmd/serv.go
wxiaoguang May 17, 2022
0841cc6
fine tune error message
wxiaoguang May 17, 2022
6274dc9
Fix a incorrect test case
wxiaoguang May 17, 2022
fae4484
Merge branch 'main' into refactor-git-fix-safedirectory
wxiaoguang May 17, 2022
b06a4ef
fix configAddNonExist
wxiaoguang May 17, 2022
fba95dc
fix configAddNonExist logic, add `--fixed-value` flag, add tests
wxiaoguang May 17, 2022
241104d
add configSetNonExist function in case it's needed.
wxiaoguang May 17, 2022
3b1e6d1
use configSetNonExist for `user.name` and `user.email`
wxiaoguang May 17, 2022
4e2679f
add some comments
wxiaoguang May 17, 2022
9866add
Update cmd/serv.go
wxiaoguang May 18, 2022
5d6cd68
Update cmd/serv.go
wxiaoguang May 18, 2022
7d96b39
Update modules/git/git.go
wxiaoguang May 18, 2022
c09f9c9
Update modules/setting/setting.go
wxiaoguang May 18, 2022
57d9b1b
Update modules/git/repo_attribute.go
wxiaoguang May 18, 2022
ae9d786
fix spaces in messages
wxiaoguang May 18, 2022
71eb7f4
use `configSet("core.protectNTFS", ...)` instead of `globalCommandArgs`
wxiaoguang May 18, 2022
1da696d
remove GIT_CONFIG_NOSYSTEM, continue to use system's git config
wxiaoguang May 19, 2022
58e909c
Merge branch 'main' into refactor-git-fix-safedirectory
wxiaoguang May 19, 2022
9eeabb1
Merge branch 'main' into refactor-git-fix-safedirectory
wxiaoguang May 21, 2022
733445c
Update cmd/serv.go
wxiaoguang Jun 2, 2022
1472714
Merge branch 'main' into refactor-git-fix-safedirectory
wxiaoguang Jun 4, 2022
30c49f3
fix merge
wxiaoguang Jun 4, 2022
c5f1e94
remove code for safe.directory
wxiaoguang Jun 4, 2022
b301197
separate git.CommonEnvs to CommonGitCmdEnvs and CommonCmdServEnvs
wxiaoguang Jun 5, 2022
985620e
Merge branch 'main' into refactor-git-fix-safedirectory
wxiaoguang Jun 8, 2022
25f1906
Merge branch 'main' into refactor-git-fix-safedirectory
lunny Jun 9, 2022
26fa087
avoid Golang's data race error
wxiaoguang Jun 9, 2022
7fb7c6e
Merge branch 'main' into refactor-git-fix-safedirectory
wxiaoguang Jun 9, 2022
dedca11
Merge branch 'main' into refactor-git-fix-safedirectory
lunny Jun 10, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 35 additions & 23 deletions cmd/serv.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package cmd

import (
"context"
"fmt"
"net/http"
"net/url"
Expand Down Expand Up @@ -64,6 +65,21 @@ func setup(logPath string, debug bool) {
if debug {
setting.RunMode = "dev"
}

// Check if setting.RepoRootPath exists. It could be the case that it doesn't exist, this can happen when
// `[repository]` `ROOT` is a relative path and $GITEA_WORK_DIR isn't passed to the SSH connection.
if _, err := os.Stat(setting.RepoRootPath); err != nil {
if os.IsNotExist(err) {
_ = fail("Incorrect configuration, no repository directory.", "Directory `[repository].ROOT` was not found, please check if $GITEA_WORK_DIR is passed to the SSH connection or make `[repository].ROOT` an absolute value.")
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
} else {
_ = fail("Incorrect configuration, repository directory is inaccessible", "Directory `[repository].ROOT` is inaccessible. err: %v", err)
}
return
}

if err := git.InitSimple(context.Background()); err != nil {
_ = fail("Failed to init git", "Failed to init git, err: %v", err)
}
}

var (
Expand All @@ -79,12 +95,12 @@ var (
func fail(userMessage, logMessage string, args ...interface{}) error {
// There appears to be a chance to cause a zombie process and failure to read the Exit status
// if nothing is outputted on stdout.
fmt.Fprintln(os.Stdout, "")
fmt.Fprintln(os.Stderr, "Gitea:", userMessage)
_, _ = fmt.Fprintln(os.Stdout, "")
_, _ = fmt.Fprintln(os.Stderr, "Gitea:", userMessage)

if len(logMessage) > 0 {
if !setting.IsProd {
fmt.Fprintf(os.Stderr, logMessage+"\n", args...)
_, _ = fmt.Fprintf(os.Stderr, logMessage+"\n", args...)
}
}
ctx, cancel := installSignals()
Expand Down Expand Up @@ -236,17 +252,6 @@ func runServ(c *cli.Context) error {
}
return fail("Internal Server Error", "%s", err.Error())
}
os.Setenv(repo_module.EnvRepoIsWiki, strconv.FormatBool(results.IsWiki))
os.Setenv(repo_module.EnvRepoName, results.RepoName)
os.Setenv(repo_module.EnvRepoUsername, results.OwnerName)
os.Setenv(repo_module.EnvPusherName, results.UserName)
os.Setenv(repo_module.EnvPusherEmail, results.UserEmail)
os.Setenv(repo_module.EnvPusherID, strconv.FormatInt(results.UserID, 10))
os.Setenv(repo_module.EnvRepoID, strconv.FormatInt(results.RepoID, 10))
os.Setenv(repo_module.EnvPRID, fmt.Sprintf("%d", 0))
os.Setenv(repo_module.EnvDeployKeyID, fmt.Sprintf("%d", results.DeployKeyID))
os.Setenv(repo_module.EnvKeyID, fmt.Sprintf("%d", results.KeyID))
os.Setenv(repo_module.EnvAppURL, setting.AppURL)

// LFS token authentication
if verb == lfsAuthenticateVerb {
Expand Down Expand Up @@ -297,19 +302,26 @@ func runServ(c *cli.Context) error {
gitcmd = exec.CommandContext(ctx, verb, repoPath)
}

// Check if setting.RepoRootPath exists. It could be the case that it doesn't exist, this can happen when
// `[repository]` `ROOT` is a relative path and $GITEA_WORK_DIR isn't passed to the SSH connection.
if _, err := os.Stat(setting.RepoRootPath); err != nil {
if os.IsNotExist(err) {
return fail("Incorrect configuration.",
"Directory `[repository]` `ROOT` was not found, please check if $GITEA_WORK_DIR is passed to the SSH connection or make `[repository]` `ROOT` an absolute value.")
}
}

gitcmd.Dir = setting.RepoRootPath
gitcmd.Stdout = os.Stdout
gitcmd.Stdin = os.Stdin
gitcmd.Stderr = os.Stderr
gitcmd.Env = append(gitcmd.Env, os.Environ()...) // FIXME: the legacy code passes the whole env. in the future, should only pass the env which are really needed
lunny marked this conversation as resolved.
Show resolved Hide resolved
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
gitcmd.Env = append(gitcmd.Env,
repo_module.EnvRepoIsWiki+"="+strconv.FormatBool(results.IsWiki),
repo_module.EnvRepoName+"="+results.RepoName,
repo_module.EnvRepoUsername+"="+results.OwnerName,
repo_module.EnvPusherName+"="+results.UserName,
repo_module.EnvPusherEmail+"="+results.UserEmail,
repo_module.EnvPusherID+"="+strconv.FormatInt(results.UserID, 10),
repo_module.EnvRepoID+"="+strconv.FormatInt(results.RepoID, 10),
repo_module.EnvPRID+"="+fmt.Sprintf("%d", 0),
repo_module.EnvDeployKeyID+"="+fmt.Sprintf("%d", results.DeployKeyID),
repo_module.EnvKeyID+"="+fmt.Sprintf("%d", results.KeyID),
repo_module.EnvAppURL+"="+setting.AppURL,
)
gitcmd.Env = append(gitcmd.Env, git.CommonEnvs()...)
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved

if err = gitcmd.Run(); err != nil {
return fail("Internal error", "Failed to execute git command: %v", err)
}
Expand Down
3 changes: 2 additions & 1 deletion integrations/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,8 @@ func prepareTestEnv(t testing.TB, skip ...int) func() {
deferFn := PrintCurrentTest(t, ourSkip)
assert.NoError(t, unittest.LoadFixtures())
assert.NoError(t, util.RemoveAll(setting.RepoRootPath))

assert.NoError(t, unittest.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath))
assert.NoError(t, git.InitWithConfigSync(context.Background()))
ownerDirs, err := os.ReadDir(setting.RepoRootPath)
if err != nil {
assert.NoError(t, err, "unable to read the new repo root: %v\n", err)
Expand Down Expand Up @@ -563,6 +563,7 @@ func resetFixtures(t *testing.T) {
assert.NoError(t, unittest.LoadFixtures())
assert.NoError(t, util.RemoveAll(setting.RepoRootPath))
assert.NoError(t, unittest.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath))
assert.NoError(t, git.InitWithConfigSync(context.Background()))
ownerDirs, err := os.ReadDir(setting.RepoRootPath)
if err != nil {
assert.NoError(t, err, "unable to read the new repo root: %v\n", err)
Expand Down
1 change: 1 addition & 0 deletions integrations/migration-test/migration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ func initMigrationTest(t *testing.T) func() {
assert.True(t, len(setting.RepoRootPath) != 0)
assert.NoError(t, util.RemoveAll(setting.RepoRootPath))
assert.NoError(t, unittest.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath))
assert.NoError(t, git.InitWithConfigSync(context.Background()))
ownerDirs, err := os.ReadDir(setting.RepoRootPath)
if err != nil {
assert.NoError(t, err, "unable to read the new repo root: %v\n", err)
Expand Down
5 changes: 2 additions & 3 deletions models/migrations/migrations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,8 @@ func prepareTestEnv(t *testing.T, skip int, syncModels ...interface{}) (*xorm.En
ourSkip += skip
deferFn := PrintCurrentTest(t, ourSkip)
assert.NoError(t, os.RemoveAll(setting.RepoRootPath))

assert.NoError(t, unittest.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"),
setting.RepoRootPath))
assert.NoError(t, unittest.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath))
assert.NoError(t, git.InitWithConfigSync(context.Background()))
ownerDirs, err := os.ReadDir(setting.RepoRootPath)
if err != nil {
assert.NoError(t, err, "unable to read the new repo root: %v\n", err)
Expand Down
5 changes: 5 additions & 0 deletions models/unittest/testdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/modules/base"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/storage"
"code.gitea.io/gitea/modules/util"
Expand Down Expand Up @@ -116,6 +117,9 @@ func MainTest(m *testing.M, testOpts *TestOptions) {
if err = CopyDir(filepath.Join(testOpts.GiteaRootPath, "integrations", "gitea-repositories-meta"), setting.RepoRootPath); err != nil {
fatalTestError("util.CopyDir: %v\n", err)
}
if err = git.InitWithConfigSync(context.Background()); err != nil {
fatalTestError("git.Init: %v\n", err)
}

ownerDirs, err := os.ReadDir(setting.RepoRootPath)
if err != nil {
Expand Down Expand Up @@ -198,6 +202,7 @@ func PrepareTestEnv(t testing.TB) {
assert.NoError(t, util.RemoveAll(setting.RepoRootPath))
metaPath := filepath.Join(giteaRoot, "integrations", "gitea-repositories-meta")
assert.NoError(t, CopyDir(metaPath, setting.RepoRootPath))
assert.NoError(t, git.InitWithConfigSync(context.Background()))

ownerDirs, err := os.ReadDir(setting.RepoRootPath)
assert.NoError(t, err)
Expand Down
38 changes: 30 additions & 8 deletions modules/git/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@ package git
import (
"bytes"
"context"
"errors"
"fmt"
"io"
"os"
"os/exec"
"path/filepath"
"strings"
"time"
"unsafe"
Expand Down Expand Up @@ -104,6 +106,16 @@ type RunOpts struct {
PipelineFunc func(context.Context, context.CancelFunc) error
}

func CommonEnvs() []string {
return []string{
fmt.Sprintf("LC_ALL=%s", DefaultLocale),
"GIT_TERMINAL_PROMPT=0", // avoid prompting for credentials interactively, supported since git v2.3
"GIT_NO_REPLACE_OBJECTS=1", // ignore replace references (https://git-scm.com/docs/git-replace)
"GIT_CONFIG_NOSYSTEM=1", // https://git-scm.com/docs/git-config, and GIT_CONFIG_GLOBAL, they require git >= 2.32
"HOME=" + HomeDir, // make Gitea use internal git config only, to prevent conflicts with user's git config
lunny marked this conversation as resolved.
Show resolved Hide resolved
}
}

// Run runs the command with the RunOpts
func (c *Command) Run(opts *RunOpts) error {
if opts == nil {
Expand Down Expand Up @@ -148,15 +160,15 @@ func (c *Command) Run(opts *RunOpts) error {
cmd.Env = opts.Env
}

cmd.Env = append(
cmd.Env,
fmt.Sprintf("LC_ALL=%s", DefaultLocale),
// avoid prompting for credentials interactively, supported since git v2.3
"GIT_TERMINAL_PROMPT=0",
// ignore replace references (https://git-scm.com/docs/git-replace)
"GIT_NO_REPLACE_OBJECTS=1",
)
if HomeDir == "" {
// TODO: now, some unit test code call the git module directly without initialization, which is incorrect.
// at the moment, we just use a temp HomeDir to prevent from conflicting with user's git config
// in future, the git module should be initialized first before use.
HomeDir = filepath.Join(os.TempDir(), "/gitea-temp-home")
lunny marked this conversation as resolved.
Show resolved Hide resolved
log.Warn("Git's HomeDir is empty, the git module is not initialized correctly, using a temp HomeDir (%s) temporarily", HomeDir)
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
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 fix those tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR has changed a lot, I do not want to mix too many things together.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I have a feeling that not fixing those tests would come back to bite us later on.

Copy link
Contributor Author

@wxiaoguang wxiaoguang Jun 2, 2022

Choose a reason for hiding this comment

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

I am sure it's not that serious. It only affects the unit tests of modules/markup.

To fix them, it requires extra work and will make this PR more complex.

Without fixing them, everything is still fine.


ps: even if some tests bite us later, there is a clear warning and the tests can be fixed easily.

If these tests must be fixed, I still prefer to fix them in another PR, instead of mixing together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a separate issue to make sure the task won't be forgotten.

}

cmd.Env = append(cmd.Env, CommonEnvs()...)
cmd.Dir = opts.Dir
cmd.Stdout = opts.Stdout
cmd.Stderr = opts.Stderr
Expand All @@ -183,7 +195,9 @@ func (c *Command) Run(opts *RunOpts) error {

type RunStdError interface {
error
Unwrap() error
Stderr() string
IsExitCode(code int) bool
}

type runStdError struct {
Expand All @@ -208,6 +222,14 @@ func (r *runStdError) Stderr() string {
return r.stderr
}

func (r *runStdError) IsExitCode(code int) bool {
var exitError *exec.ExitError
if errors.As(r.err, &exitError) {
return exitError.ExitCode() == code
}
return false
}

func bytesToString(b []byte) string {
return *(*string)(unsafe.Pointer(&b)) // that's what Golang's strings.Builder.String() does (go/src/strings/builder.go)
}
Expand Down
5 changes: 0 additions & 5 deletions modules/git/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -398,11 +398,6 @@ func (c *Commit) GetSubModule(entryname string) (*SubModule, error) {

// GetBranchName gets the closest branch name (as returned by 'git name-rev --name-only')
func (c *Commit) GetBranchName() (string, error) {
err := LoadGitVersion()
if err != nil {
return "", fmt.Errorf("Git version missing: %v", err)
}

args := []string{
"name-rev",
}
Expand Down
Loading