Change git.cmd to RunWithContext

[martinscholz83]

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

[6543]

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

[martinscholz83]

You mean the PipelineFunc member of the RunContext struct?

[lunny]

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

[martinscholz83]

👍 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]

Codecov Report

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

@@           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.

[zeripath]

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]

👍 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]

I think i missed some places like this

[lunny]

I think i missed some places like this

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

[martinscholz83]

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]

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

[lunny]

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]

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

[martinscholz83]

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]

☝️ 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]

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]

☝️ that would be awesome

[lunny]

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

[6543]

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

[6543]

thanks

[6543]

hope you enjoined contributing 👍

[martinscholz83]

🎉 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]

☝️ yes 👍