Replace RunInDir with RunWithContext #18978

martinscholz83 · 2022-03-02T18:17:57Z

Replace RunInDir with RunWIthContext.

#18553

6543 · 2022-03-02T19:23:58Z

@martinscholz83 please run make fmt and commit the changes

integrations/pull_merge_test.go

models/migrations/v134.go

modules/doctor/mergebase.go

modules/git/commit.go

modules/git/remote.go

modules/git/repo.go

modules/git/repo_blame.go

modules/git/repo_commit.go

modules/git/repo_commit_nogogit.go

modules/git/repo_index.go

lunny · 2022-03-03T06:18:34Z

I think strings.Builder should have better performance than bytes.Buffer. Otherwise L-G-T-M .

martinscholz83 · 2022-03-03T08:44:36Z

That repo_tree_gogit.go build failure I can ignore?

lunny · 2022-03-03T11:55:55Z

That repo_tree_gogit.go build failure I can ignore?

I don't think so.

martinscholz83 · 2022-03-03T11:58:59Z

I don't understand why it fails for arm64.

modules/git/repo_commit_gogit.go

martinscholz83 · 2022-03-23T08:21:10Z

One more thing for my understanding. I found a lot places like this where still the ctx error is logged instead of the output from stderr. So my question, do these places also need to be changed to log stderr instead of err?

6543 · 2022-03-23T20:24:14Z

@martinscholz83 a general roule: if err is just needed to check if it fails we dont need the stderr, if it is used in logs etc .. stderr is needed

and I agree to add a wrapper called RunWithContextString that return (stdout, stderr, err) - that way we dont need mouch boilerplate (this func should fail if Context set stdout and stderr, as it will init it's own)

I'll have a look at this pull soon^TM

zeripath · 2022-03-23T22:03:50Z

Hmm... although this fails in different places each time I think it's been consistently failing too many times and we need to double check that there isn't a real issue here.

# Conflicts: # services/mirror/mirror_pull.go # services/mirror/mirror_push.go

modules/git/command.go

modules/git/repo_commit_gogit.go

wxiaoguang · 2022-03-28T12:13:40Z

This PR does wrong from begnning ..... replacing RunInDir by RunWithContext is incorrect, because the error returned by RunInDir contains stderr, while RunWithContext doesn't. Such replacing causes many error checks doesn't work correctly.

If there is a chance, the refactoring should use RunWithContextString to replace RunInDir.

wxiaoguang · 2022-03-28T13:15:50Z

I reset the branch to the original one (cleaned my changes).

I think we need to make some decisions about how to continue ......

wxiaoguang · 2022-03-28T13:20:44Z

modules/git/command.go

-// RunInDir executes the command in given directory
-// and returns stdout in string and error (combined with stderr).
-func (c *Command) RunInDir(dir string) (string, error) {


The key point is here, it returns "stdout in string and error (combined with stderr)"

So the RunInXxx can not be replaced by RunWithContext directly

wxiaoguang · 2022-04-01T03:00:55Z

Hi @martinscholz83 , thank you for your contribution and give us the hint about the refactoring. Now we can close #18553 and we have a clear git command module in Gitea! 🎉

Although this PR is not merged, it really help us to know how to continue about the refactoring. Appreciate your work.

Now we can safely close this PR.

Replace RunInDir with RunWithContext

7126b04

6543 added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 2, 2022

6543 added this to the 1.17.0 milestone Mar 2, 2022

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 2, 2022