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.

Sign up for GitHub

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

Jump to bottom

replace all git.Command.RunInPipeline... with RunWithContext #18553

Closed
2 tasks done
6543 opened this issue Feb 2, 2022 · 6 comments · Fixed by #19280
Closed
2 tasks done

replace all git.Command.RunInPipeline... with RunWithContext #18553

6543 opened this issue Feb 2, 2022 · 6 comments · Fixed by #19280
Assignees
@martinscholz83
Labels
good first issue Likely to be an easy fix type/refactoring Existing code has been cleaned up. There should be no new functionality.
Milestone
1.17.0

Comments

@6543
Copy link
Member

6543 commented Feb 2, 2022 •
edited by wxiaoguang
Loading

  • Change git.cmd to RunWithContext (Change git.cmd to RunWithContext #18693)
    cmd.RunInDirTimeoutPipeline,
    cmd.RunInDirTimeoutEnvFullPipeline,
    cmd.RunInDirFullPipeline,
    ...
  • Delete cmd.RunInDir... func()
@6543 6543 added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Feb 2, 2022
@6543 6543 added this to the 1.17.0 milestone Feb 2, 2022
@6543 6543 added the good first issue Likely to be an easy fix label Feb 2, 2022
@lunny
Copy link
Member

lunny commented Feb 3, 2022

If all has been replaced, then we can rename cmd.RunWithContext to cmd.Run

@martinscholz83
Copy link
Contributor

Can I take this up? I searched with a regex .*cmd\..*Pipeline for all places. I think that should find all.

@6543
Copy link
Member Author

6543 commented Feb 9, 2022 •
edited
Loading

@martinscholz83 yes would be awesome 👍

@6543
Copy link
Member Author

6543 commented Feb 9, 2022

PS: if you look closely all the RunInPipeline... func do already use RunWithContext ... so the code cleanup should be easy todo - if you have issues just tell us here :)

@martinscholz83 martinscholz83 mentioned this issue Feb 9, 2022
Merged
6543 pushed a commit that referenced this issue Feb 11, 2022
@martinscholz83
Change git.cmd to RunWithContext (#18693)
26718a7 
Change all `cmd...Pipeline` commands to `cmd.RunWithContext`.

#18553

Co-authored-by: Martin Scholz <martin.scholz@versasec.com>
@martinscholz83
Copy link
Contributor

I changed all the calls to RunWithContext so that I can now remove the RunInPipeline func. Shall I open a new PR?

@6543
Copy link
Member Author

6543 commented Mar 2, 2022

@martinscholz83 yes sure go on :)

as always if you need help we are a @ping away ;)

@martinscholz83 martinscholz83 mentioned this issue Mar 2, 2022
Closed
Chianina pushed a commit to Chianina/gitea that referenced this issue Mar 28, 2022
@martinscholz83
Change git.cmd to RunWithContext (go-gitea#18693)
36387df 
Change all `cmd...Pipeline` commands to `cmd.RunWithContext`.

go-gitea#18553

Co-authored-by: Martin Scholz <martin.scholz@versasec.com>
@wxiaoguang wxiaoguang mentioned this issue Mar 30, 2022
Merged
6543 pushed a commit that referenced this issue Mar 31, 2022
@wxiaoguang
Refactor git.Command.Run*, introduce RunWithContextString and `Ru…
b877504 
…nWithContextBytes` (#19266)

This follows 
* #18553

Introduce `RunWithContextString` and `RunWithContextBytes` to help the refactoring. Add related unit tests. They keep the same behavior to save stderr into err.Error() as `RunInXxx` before.

Remove `RunInDirTimeoutPipeline` `RunInDirTimeoutFullPipeline` `RunInDirTimeout` `RunInDirTimeoutEnv`  `RunInDirPipeline`  `RunInDirFullPipeline`  `RunTimeout`, `RunInDirTimeoutEnvPipeline`, `RunInDirTimeoutEnvFullPipeline`, `RunInDirTimeoutEnvFullPipelineFunc`.

Then remaining `RunInDir` `RunInDirBytes` `RunInDirWithEnv` can be easily refactored in next PR with a simple search & replace:
* before: `stdout, err := RunInDir(path)`
* next: `stdout, _, err := RunWithContextString(&git.RunContext{Dir:path})`

Other changes:
1. When `timeout <= 0`, use default. Because `timeout==0` is meaningless and could cause bugs. And now many functions becomes more simple, eg: `GitGcRepos` 9 lines to 1 line. `Fsck` 6 lines to 1 line.
2. Only set defaultCommandExecutionTimeout when the option `setting.Git.Timeout.Default > 0`
@wxiaoguang wxiaoguang mentioned this issue Mar 31, 2022
Merged
wxiaoguang added a commit that referenced this issue Apr 1, 2022
@wxiaoguang
Remove git.Command.Run and git.Command.RunInDir* (#19280)
124b072 
Follows #19266, #8553, Close #18553, now there are only three `Run..(&RunOpts{})` functions.
 * before: `stdout, err := RunInDir(path)`
 * now: `stdout, _, err := RunStdString(&git.RunOpts{Dir:path})`
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this issue Aug 24, 2022
@wxiaoguang @AbdulrhmnGhanem
Remove git.Command.Run and git.Command.RunInDir* (go-gitea#19280)
737ef3b 
Follows go-gitea#19266, go-gitea#8553, Close go-gitea#18553, now there are only three `Run..(&RunOpts{})` functions.
 * before: `stdout, err := RunInDir(path)`
 * now: `stdout, _, err := RunStdString(&git.RunOpts{Dir:path})`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@martinscholz83 martinscholz83

Labels
good first issue Likely to be an easy fix type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.17.0
Development

Successfully merging a pull request may close this issue.

Remove git.Command.Run and git.Command.RunInDir* wxiaoguang/gitea
3 participants
@lunny @martinscholz83 @6543