Remove git.Command.Run and git.Command.RunInDir*

[wxiaoguang]

Follows

• Refactor git.Command.Run*, introduce RunWithContextString and RunWithContextBytes #19266
• replace all git.Command.RunInPipeline... with RunWithContext #18553

Close #18553

Now, only RunWithContext related functions.

This PR is a simple search & replace:

• before: stdout, err := RunInDir(path)
• now: stdout, _, err := RunStdString(&git.RunOpts{Dir:path})

[lunny]

I think it's better to have some rename.

• rename RunWithContext -> Run
• rename RunWithContextString -> RunString
• rename RunWithContextBytes -> RunBytes

[wxiaoguang]

I think it's better to have some rename.

* rename `RunWithContext` -> `Run`

* rename `RunWithContextString` -> `RunString`

* rename `RunWithContextBytes` -> `RunBytes`

Do most people like to rename them now? And I think if we do so, we should also rename RunContext, it's not a context. What's the proper name?

RunContext is OK, or RunOptions is better.

[wxiaoguang]

And the name Run is too short. I prefer to use RunCmd, RunStdString, RunStdBytes, and use CmdParams instead of RunContext

The longer names are friendly to be grep-ed out during refactoring.

[lunny]

And the name Run is too short. I prefer to use RunCmd, RunStdString, RunStdBytes, and use CmdParams instead of RunContext

The longer names are friendly to be grep-ed out during refactoring.

I don't think command.RunCmd is a good name.

[wxiaoguang]

Hmmm, RunCmd is not good indeed. Do we have choices for a two-word name?

[wxiaoguang]

I can not get a better name now. So how about:

• cmd.Run(&git.CmdOpts{Dir: "..."})
• cmd.RunStdString(&git.CmdOpts{Dir: "..."}) // std means it handles stdout/stderr
• cmd.RunStdBytes(&git.CmdOpts{Dir: "..."})

If no objection, I will do it.

[lunny]

I can not get a better name now. So how about:

• cmd.Run(&git.CmdOpts{Dir: "..."})
• cmd.RunStdString(&git.CmdOpts{Dir: "..."}) // std means it handles stdout/stderr
• cmd.RunStdBytes(&git.CmdOpts{Dir: "..."})

If no objection, I will do it.

I still think Std will not help for understanding the method meaning. And the options is for Run not for Command. Command have many Args methods. So CmdOpts is not better than RunOpts. In fact, I think move command to git/command sub package is better, so that RunOpts will be more clear than CmdOpts.

[wxiaoguang]

I pushed a demo for:

• cmd.Run(&git.RunOpts{Dir: "..."})
• cmd.RunStdString(&git.RunOpts{Dir: "..."}) // std means it handles stdout/stderr
• cmd.RunStdBytes(&git.RunOpts{Dir: "..."})

I think the Std is a must, because these two functions are totally different from Run, they handle stdout/stderr internally, and the returned error is totally different.

[wxiaoguang]

@lunny a real example, when you read:

err := cmd.Run(...)
_, _, err := cmd.RunString(...)

// then
if strings.Contains(err.Error(), "fatal: ...") {
}

Do you know that these 2 errors are different? The second one contains stderr output, while the first one doesn't, if misused, there would be bugs.

So _, _, err := cmd.RunStdString(...) is more clear in such case.

And I have designed a new error RunStdError, so in future you can do:

_, _, runErr := cmd.RunStdString(...);
if runErr != nil && runErr.Stderr() == "fatal: ..." { ... }

Access the stderr directly, more handy and clear.

[6543]

I pushed a demo for:

* `cmd.Run(&git.RunOpts{Dir: "..."})`

* `cmd.RunStdString(&git.RunOpts{Dir: "..."})`   // std means it handles stdout/stderr

* `cmd.RunStdBytes(&git.RunOpts{Dir: "..."})`

I think the Std is a must, because these two functions are totally different from Run, they handle stdout/stderr internally, and the returned error is totally different.

Ack for rename ...

[codecov-commenter]

Codecov Report

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

@@           Coverage Diff           @@
##             main   #19280   +/-   ##
=======================================
  Coverage        ?   47.40%           
=======================================
  Files           ?      939           
  Lines           ?   131080           
  Branches        ?        0           
=======================================
  Hits            ?    62134           
  Misses          ?    61471           
  Partials        ?     7475           

Impacted Files	Coverage Δ
cmd/hook.go	7.11% <0.00%> (ø)
modules/doctor/mergebase.go	10.25% <0.00%> (ø)
modules/doctor/misc.go	21.55% <0.00%> (ø)
modules/git/pipeline/lfs_nogogit.go	0.00% <0.00%> (ø)
modules/git/pipeline/namerev.go	0.00% <0.00%> (ø)
modules/git/repo_commitgraph.go	50.00% <0.00%> (ø)
modules/indexer/code/elastic_search.go	1.12% <0.00%> (ø)
routers/private/hook_verification.go	0.00% <0.00%> (ø)
routers/web/repo/pull.go	30.84% <0.00%> (ø)
services/asymkey/sign.go	38.46% <0.00%> (ø)
... and 56 more

---

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 3a73645...82f75be. Read the comment docs.