From 73981089584aa109ca296407d13ccd30173992ef Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 11 Dec 2022 21:24:31 +0800 Subject: [PATCH 1/6] Use git command instead of exec.Cmd in blame --- modules/git/blame.go | 74 +++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 45 deletions(-) diff --git a/modules/git/blame.go b/modules/git/blame.go index fea75b481827..39e6d849dffe 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -9,10 +9,7 @@ import ( "fmt" "io" "os" - "os/exec" "regexp" - - "code.gitea.io/gitea/modules/process" ) // BlamePart represents block of blame - continuous lines with one sha @@ -23,12 +20,11 @@ type BlamePart struct { // BlameReader returns part of file blame one by one type BlameReader struct { - cmd *exec.Cmd - output io.ReadCloser - reader *bufio.Reader - lastSha *string - cancel context.CancelFunc // Cancels the context that this reader runs in - finished process.FinishedFunc // Tells the process manager we're finished and it can remove the associated process from the process table + cmd *Command + output io.WriteCloser + reader io.ReadCloser + done chan error + lastSha *string } var shaLineRegex = regexp.MustCompile("^([a-z0-9]{40})") @@ -37,7 +33,7 @@ var shaLineRegex = regexp.MustCompile("^([a-z0-9]{40})") func (r *BlameReader) NextPart() (*BlamePart, error) { var blamePart *BlamePart - reader := r.reader + reader := bufio.NewReader(r.reader) if r.lastSha != nil { blamePart = &BlamePart{*r.lastSha, make([]string, 0)} @@ -99,51 +95,39 @@ func (r *BlameReader) NextPart() (*BlamePart, error) { // Close BlameReader - don't run NextPart after invoking that func (r *BlameReader) Close() error { - defer r.finished() // Only remove the process from the process table when the underlying command is closed - r.cancel() // However, first cancel our own context early - + err := <-r.done + _ = r.reader.Close() _ = r.output.Close() - - if err := r.cmd.Wait(); err != nil { - return fmt.Errorf("Wait: %w", err) - } - - return nil + return err } // CreateBlameReader creates reader for given repository, commit and file func CreateBlameReader(ctx context.Context, repoPath, commitID, file string) (*BlameReader, error) { - return createBlameReader(ctx, repoPath, GitExecutable, "blame", commitID, "--porcelain", "--", file) -} - -func createBlameReader(ctx context.Context, dir string, command ...string) (*BlameReader, error) { - // Here we use the provided context - this should be tied to the request performing the blame so that it does not hang around. - ctx, cancel, finished := process.GetManager().AddContext(ctx, fmt.Sprintf("GetBlame [repo_path: %s]", dir)) - - cmd := exec.CommandContext(ctx, command[0], command[1:]...) - cmd.Dir = dir - cmd.Stderr = os.Stderr - process.SetSysProcAttribute(cmd) - - stdout, err := cmd.StdoutPipe() + cmd := NewCommandContextNoGlobals(ctx, "blame", CmdArg(commitID), "--porcelain") + cmd.AddDashesAndList(file) + cmd.SetDescription(fmt.Sprintf("GetBlame [repo_path: %s]", repoPath)) + reader, stdout, err := os.Pipe() if err != nil { - defer finished() - return nil, fmt.Errorf("StdoutPipe: %w", err) + return nil, err } - if err = cmd.Start(); err != nil { - defer finished() - _ = stdout.Close() - return nil, fmt.Errorf("Start: %w", err) - } + done := make(chan error, 1) - reader := bufio.NewReader(stdout) + go func(cmd *Command, dir string, stdout io.WriteCloser, done chan error) { + if err := cmd.Run(&RunOpts{ + Dir: dir, + Stdout: stdout, + Stderr: os.Stderr, + }); err == nil { + stdout.Close() + } + done <- err + }(cmd, repoPath, stdout, done) return &BlameReader{ - cmd: cmd, - output: stdout, - reader: reader, - cancel: cancel, - finished: finished, + cmd: cmd, + output: stdout, + reader: reader, + done: done, }, nil } From 7e4a66dea56afabf78308ea57d3cd9c2d6b7d89c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 11 Dec 2022 21:37:26 +0800 Subject: [PATCH 2/6] as per wxiaoguang --- modules/git/blame.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/git/blame.go b/modules/git/blame.go index 39e6d849dffe..802880fa3057 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -103,7 +103,7 @@ func (r *BlameReader) Close() error { // CreateBlameReader creates reader for given repository, commit and file func CreateBlameReader(ctx context.Context, repoPath, commitID, file string) (*BlameReader, error) { - cmd := NewCommandContextNoGlobals(ctx, "blame", CmdArg(commitID), "--porcelain") + cmd := NewCommandContextNoGlobals(ctx, "blame", CmdArgCheck(commitID), "--porcelain") cmd.AddDashesAndList(file) cmd.SetDescription(fmt.Sprintf("GetBlame [repo_path: %s]", repoPath)) reader, stdout, err := os.Pipe() From 682359202a7636d37175945e986d7473b53b165e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 11 Dec 2022 22:55:11 +0800 Subject: [PATCH 3/6] Fix test --- modules/git/blame_test.go | 119 +++----------------------------------- 1 file changed, 8 insertions(+), 111 deletions(-) diff --git a/modules/git/blame_test.go b/modules/git/blame_test.go index 94277b7c1d19..a2c8fe8e7565 100644 --- a/modules/git/blame_test.go +++ b/modules/git/blame_test.go @@ -5,139 +5,36 @@ package git import ( "context" - "os" "testing" "github.com/stretchr/testify/assert" ) -const exampleBlame = ` -4b92a6c2df28054ad766bc262f308db9f6066596 1 1 1 -author Unknown -author-mail -author-time 1392833071 -author-tz -0500 -committer Unknown -committer-mail -committer-time 1392833071 -committer-tz -0500 -summary Add code of delete user -previous be0ba9ea88aff8a658d0495d36accf944b74888d gogs.go -filename gogs.go - // Copyright 2014 The Gogs Authors. All rights reserved. -ce21ed6c3490cdfad797319cbb1145e2330a8fef 2 2 1 -author Joubert RedRat -author-mail -author-time 1482322397 -author-tz -0200 -committer Lunny Xiao -committer-mail -committer-time 1482322397 -committer-tz +0800 -summary Remove remaining Gogs reference on locales and cmd (#430) -previous 618407c018cdf668ceedde7454c42fb22ba422d8 main.go -filename main.go - // Copyright 2016 The Gitea Authors. All rights reserved. -4b92a6c2df28054ad766bc262f308db9f6066596 2 3 2 -author Unknown -author-mail -author-time 1392833071 -author-tz -0500 -committer Unknown -committer-mail -committer-time 1392833071 -committer-tz -0500 -summary Add code of delete user -previous be0ba9ea88aff8a658d0495d36accf944b74888d gogs.go -filename gogs.go - // Use of this source code is governed by a MIT-style -4b92a6c2df28054ad766bc262f308db9f6066596 3 4 -author Unknown -author-mail -author-time 1392833071 -author-tz -0500 -committer Unknown -committer-mail -committer-time 1392833071 -committer-tz -0500 -summary Add code of delete user -previous be0ba9ea88aff8a658d0495d36accf944b74888d gogs.go -filename gogs.go - // license that can be found in the LICENSE file. - -e2aa991e10ffd924a828ec149951f2f20eecead2 6 6 2 -author Lunny Xiao -author-mail -author-time 1478872595 -author-tz +0800 -committer Sandro Santilli -committer-mail -committer-time 1478872595 -committer-tz +0100 -summary ask for go get from code.gitea.io/gitea and change gogs to gitea on main file (#146) -previous 5fc370e332171b8658caed771b48585576f11737 main.go -filename main.go - // Gitea (git with a cup of tea) is a painless self-hosted Git Service. -e2aa991e10ffd924a828ec149951f2f20eecead2 7 7 - package main // import "code.gitea.io/gitea" -` - func TestReadingBlameOutput(t *testing.T) { - tempFile, err := os.CreateTemp("", ".txt") - if err != nil { - panic(err) - } - - defer tempFile.Close() - - if _, err = tempFile.WriteString(exampleBlame); err != nil { - panic(err) - } ctx, cancel := context.WithCancel(context.Background()) defer cancel() - blameReader, err := createBlameReader(ctx, "", "cat", tempFile.Name()) - if err != nil { - panic(err) - } + blameReader, err := CreateBlameReader(ctx, "./tests/repos/repo5_pulls", "f32b0a9dfd09a60f616f29158f772cedd89942d2", "README.md") + assert.NoError(t, err) defer blameReader.Close() parts := []*BlamePart{ { - "4b92a6c2df28054ad766bc262f308db9f6066596", - []string{ - "// Copyright 2014 The Gogs Authors. All rights reserved.", - }, - }, - { - "ce21ed6c3490cdfad797319cbb1145e2330a8fef", - []string{ - "// Copyright 2016 The Gitea Authors. All rights reserved.", - }, - }, - { - "4b92a6c2df28054ad766bc262f308db9f6066596", + "72866af952e98d02a73003501836074b286a78f6", []string{ - "// Use of this source code is governed by a MIT-style", - "// license that can be found in the LICENSE file.", - "", + "# test_repo", + "Test repository for testing migration from github to gitea", }, }, { - "e2aa991e10ffd924a828ec149951f2f20eecead2", - []string{ - "// Gitea (git with a cup of tea) is a painless self-hosted Git Service.", - "package main // import \"code.gitea.io/gitea\"", - }, + "f32b0a9dfd09a60f616f29158f772cedd89942d2", + []string{}, }, - nil, } for _, part := range parts { actualPart, err := blameReader.NextPart() - if err != nil { - panic(err) - } + assert.NoError(t, err) assert.Equal(t, part, actualPart) } } From 542927ff675c0dfa97e55d64aa98c08015fdd051 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 13 Dec 2022 23:23:15 +0800 Subject: [PATCH 4/6] Make git command safer --- modules/git/blame.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/modules/git/blame.go b/modules/git/blame.go index 802880fa3057..cf6d7f8e602d 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -103,9 +103,11 @@ func (r *BlameReader) Close() error { // CreateBlameReader creates reader for given repository, commit and file func CreateBlameReader(ctx context.Context, repoPath, commitID, file string) (*BlameReader, error) { - cmd := NewCommandContextNoGlobals(ctx, "blame", CmdArgCheck(commitID), "--porcelain") - cmd.AddDashesAndList(file) - cmd.SetDescription(fmt.Sprintf("GetBlame [repo_path: %s]", repoPath)) + cmd := NewCommandContextNoGlobals(ctx, "blame"). + AddDynamicArguments(commitID). + AddArguments("--porcelain"). + AddDashesAndList(file). + SetDescription(fmt.Sprintf("GetBlame [repo_path: %s]", repoPath)) reader, stdout, err := os.Pipe() if err != nil { return nil, err From e05232fc3c37017bc5bde19f7e8e4f98dea4f40f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 15 Dec 2022 11:41:43 +0800 Subject: [PATCH 5/6] Use context timeout in blame command --- modules/git/blame.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/modules/git/blame.go b/modules/git/blame.go index cf6d7f8e602d..ef98480a9a3c 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -117,9 +117,10 @@ func CreateBlameReader(ctx context.Context, repoPath, commitID, file string) (*B go func(cmd *Command, dir string, stdout io.WriteCloser, done chan error) { if err := cmd.Run(&RunOpts{ - Dir: dir, - Stdout: stdout, - Stderr: os.Stderr, + UseContextTimeout: true, + Dir: dir, + Stdout: stdout, + Stderr: os.Stderr, }); err == nil { stdout.Close() } From 271faf7d7bacac9f4990665aa9f9b1795e8299d3 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 15 Dec 2022 14:05:25 +0800 Subject: [PATCH 6/6] Update modules/git/blame.go Co-authored-by: wxiaoguang --- modules/git/blame.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/git/blame.go b/modules/git/blame.go index ef98480a9a3c..3b6e4c95db9f 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -103,9 +103,8 @@ func (r *BlameReader) Close() error { // CreateBlameReader creates reader for given repository, commit and file func CreateBlameReader(ctx context.Context, repoPath, commitID, file string) (*BlameReader, error) { - cmd := NewCommandContextNoGlobals(ctx, "blame"). + cmd := NewCommandContextNoGlobals(ctx, "blame", "--porcelain"). AddDynamicArguments(commitID). - AddArguments("--porcelain"). AddDashesAndList(file). SetDescription(fmt.Sprintf("GetBlame [repo_path: %s]", repoPath)) reader, stdout, err := os.Pipe()