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

Change git.cmd to RunWithContext #18693

Merged
merged 10 commits into from
Feb 11, 2022

Conversation

martinscholz83
Copy link
Contributor

Change all cmd...Pipeline commands from to cmd.RunWithContext.
#18553

@6543 6543 self-requested a review February 9, 2022 20:50
@6543 6543 added this to the 1.17.0 milestone Feb 9, 2022
@6543 6543 added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Feb 9, 2022
@6543
Copy link
Member

6543 commented Feb 9, 2022

Can you also delete tje ...pipeline... functions?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 9, 2022
@martinscholz83
Copy link
Contributor Author

You mean the PipelineFunc member of the RunContext struct?

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 9, 2022
@lunny
Copy link
Member

lunny commented Feb 10, 2022

Maybe you need add Timeout: -1 for those changes.

@martinscholz83
Copy link
Contributor Author

👍 Wasn't sure about the Timeout. Just saw now that these pipeline functions have different overloads to set Timeout to -1. @6543, you mean deleting the Pipeline methods for the command struct like func (c *Command) RunInDirTimeoutEnvFullPipeline, right?

@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@7489d96). Click here to learn what that means.
The diff coverage is 75.38%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #18693   +/-   ##
=======================================
  Coverage        ?   46.47%           
=======================================
  Files           ?      851           
  Lines           ?   122000           
  Branches        ?        0           
=======================================
  Hits            ?    56700           
  Misses          ?    58417           
  Partials        ?     6883           
Impacted Files Coverage Δ
modules/git/pipeline/revlist.go 50.00% <41.66%> (ø)
modules/gitgraph/graph.go 58.76% <53.33%> (ø)
modules/git/pipeline/catfile.go 75.36% <84.61%> (ø)
services/pull/merge.go 47.83% <92.30%> (ø)
modules/git/repo_attribute.go 70.60% <94.44%> (ø)
modules/git/repo.go 65.74% <100.00%> (ø)
modules/git/repo_index.go 53.33% <100.00%> (ø)
modules/git/repo_object.go 64.70% <100.00%> (ø)
modules/git/repo_tree.go 80.43% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7489d96...42e9a1b. Read the comment docs.

@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 Feb 10, 2022
@zeripath
Copy link
Contributor

zeripath commented Feb 10, 2022

Timeout is less of a problem now that the process go context is passed down to the git command.

TBH we should probably be moving the go context into the RunContext e.g.

// RunContext represents parameters to run the command
type RunContext struct {
        context.Context
	Env            []string
	Timeout        time.Duration
	Dir            string
	Stdout, Stderr io.Writer
	Stdin          io.Reader
	PipelineFunc   func(context.Context, context.CancelFunc) error
}

and dropping the parent context stuff from the Command.

@6543
Copy link
Member

6543 commented Feb 10, 2022

👍 Wasn't sure about the Timeout. Just saw now that these pipeline functions have different overloads to set Timeout to -1. @6543, you mean deleting the Pipeline methods for the command struct like func (c *Command) RunInDirTimeoutEnvFullPipeline, right?

Yes

@martinscholz83
Copy link
Contributor Author

I think i missed some places like this

@lunny
Copy link
Member

lunny commented Feb 10, 2022

I think i missed some places like this

How about to delete the method RunInDirPipeline if all of it have been replaced?

@martinscholz83
Copy link
Contributor Author

While deleting the RunInDirPipeline I realized that these RunInDir also need to replaced with RunWithContext. Is this correct? Like here. So the only method commandshould implement is RunWithContext?

@6543
Copy link
Member

6543 commented Feb 10, 2022

yes there are different RunInXY ... func that all can be replaced

@lunny
Copy link
Member

lunny commented Feb 10, 2022

While deleting the RunInDirPipeline I realized that these RunInDir also need to replaced with RunWithContext. Is this correct? Like here. So the only method commandshould implement is RunWithContext?

Right! There are also another x methods need to be replaced. That's why I just introduced the RunWithContext but not replace them in one PR.

@lunny
Copy link
Member

lunny commented Feb 10, 2022

I also have a related PR #18147 to do related things.

@martinscholz83
Copy link
Contributor Author

The thing is, if your looking at command.go. All these overloads ending up in RunInDirTimeoutEnvFullPipelineFunc. So I can not clean up these RunInDirPipeline methods without replacing in all places in the src code with RunWithContext.

@6543
Copy link
Member

6543 commented Feb 10, 2022

☝️ that's what has to be done :)

... replacing in all places in the src code with RunWithContext ...

as mentioned it's not hard to do or understand, just a huge refactor in terms of touched lines of code

@martinscholz83
Copy link
Contributor Author

That's why I just introduced the RunWithContext but not replace them in one PR.

I think i understood this wrong 😄. Thought you will do this. So i will replace everything and the clean up command.go.

@6543
Copy link
Member

6543 commented Feb 10, 2022

☝️ that would be awesome

@lunny
Copy link
Member

lunny commented Feb 11, 2022

I think we can refactor them step by step. This PR is enough. Let's merge it at first.

modules/git/repo_stats.go Outdated Show resolved Hide resolved
@6543
Copy link
Member

6543 commented Feb 11, 2022

@martinscholz83 thanks for the work, i only found one thing ☝️ and after it its good to go :)

modules/git/repo_stats.go Outdated Show resolved Hide resolved
modules/git/repo_stats.go Outdated Show resolved Hide resolved
@6543
Copy link
Member

6543 commented Feb 11, 2022

thanks

@6543 6543 merged commit 26718a7 into go-gitea:main Feb 11, 2022
@6543
Copy link
Member

6543 commented Feb 11, 2022

hope you enjoined contributing 👍

@martinscholz83
Copy link
Contributor Author

🎉 Definitely! Thx for your help! To replace the rest of the RunInDir functions I started to work on.

		_, err := git.NewCommand(git.DefaultContext, "read-tree", "--empty").RunInDir(path)

These I got easily replaced with a regex script to

		err := git.NewCommand(git.DefaultContext, "read-tree", "--empty").RunWithContext(&RunContext{Dir: path, Timeout: -1,})

and then maybe check if RunContext can be resolved or if it has to be git.RunContext. But a lot of calls use the returned string (stdout) from RunInDir for later usage. So for example this

treeSha, err := git.NewCommand(git.DefaultContext, "write-tree").RunInDir(path)
treeSha = strings.TrimSpace(treeSha)

has to become this for example

stdout := new(bytes.Buffer)
err := git.NewCommand(git.DefaultContext, "write-tree").RunWithContext(&RunContext{Dir: path, Timeout: -1, Stdout: &stdout})
treeSha = strings.TrimSpace(stdout.String())
...

Is this what you have in mind?

@6543
Copy link
Member

6543 commented Feb 11, 2022

☝️ yes 👍

zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 12, 2022
* giteaofficial/main:
  Send mail to issue/pr assignee/reviewer also when OnMention is set (go-gitea#18707)
  Reduce CI go module downloads, add make targets (go-gitea#18708)
  Add number in queue status to monitor page (go-gitea#18712)
  Fix source code line highlighting (go-gitea#18729)
  Fix forked repositories missed tags (go-gitea#18719)
  [skip ci] Updated translations via Crowdin
  Fix release typo (go-gitea#18728)
  Display template path of current page in dev mode (go-gitea#18717)
  Separate the details links of commit-statuses in headers (go-gitea#18661)
  Add LDAP group sync to Teams, fixes go-gitea#1395 (go-gitea#16299)
  Change git.cmd to RunWithContext (go-gitea#18693)
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
Change all `cmd...Pipeline` commands to `cmd.RunWithContext`.

go-gitea#18553

Co-authored-by: Martin Scholz <martin.scholz@versasec.com>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
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. 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