Avoid creating unnecessary temporary cat file sub process

[lunny]

Extract from #33934

In the same goroutine, we should reuse the exist cat file sub process which exist in git.Repository to avoid creating a unnecessary temporary subprocess.

This PR reuse the exist cate file writer and reader in getCommitFromBatchReader.
It also move prepareLatestCommitInfo before creating dataRc which will hold the writer so other git operation will create a temporary cat file subprocess.

[wxiaoguang]

The design is not right.

You should make the cat-file reader be thread-safe and be shared by different callers. But not keeping asking developers to strictly arrange the callers orders.

I do not see why the cat-file reader can't be shared, if I understand correctly, it is a standard request-response service provider. Correct me if I am wrong.

[lunny]

The design is not right.

You should make the cat-file reader be thread-safe and be shared by different callers. But not keeping asking developers to strictly arrange the callers orders.

I do not see why the cat-file reader can't be shared, if I understand correctly, it is a standard request-response service provider. Correct me if I am wrong.

There is no new design in this PR. The new design could be in #33934 or other new PRs. This PR just fix the unnecessary temporary subprocess according to original design so that it could be backport to v1.23.

Regarding the original design: since cat-file relies on a strict input-output order — for example, input1 corresponds to output1, input2 to output2, and so on — I don’t believe it can be easily shared between two goroutines without guaranteed order. The tight coupling between input and output makes concurrent access without coordination problematic.

Regarding the drawback of the original design: it spawns at least one Git subprocess for every Git-related http request. This frequent creation and destroy of subprocesses can be inefficient. Introducing a managed subprocess pool in the background could mitigate this by reusing existing subprocesses, thereby reducing overhead. This approach would function similarly to how Nginx manages worker processes. However, implementing such a mechanism adds significant complexity.

dimiss

[wxiaoguang]

OK, I won't block it, but I do not think it's worth to use this temporary fix or backport since it isn't fully tested or proven to fix any known user end bugs. The risk of backporting it is much higher than the benefit.

If let me decide, just use a complete fix in main branch and just keep 1.23 as-is. Do not use this temp fix.

[TheFox0x7]

Isn't cat-file command tied to a repository? I don't see how a pool would help, unless there's a constant (or on demand) command running per repo with req-reply interface in front. Question then is what if single cat-file instance is overloaded (I'm assuming it's possible - but I might be wrong)?

Current solution, though inefficient when concurrent requests happen has a clearly defined lifecycle (matching context, so in turn request).

---

I'm kind of stuck on the idea of NATS replacing queue system in gitea but the req-reply it provides would fit here - assuming we figure out how to bring the cat-file instances up and down on demand and scale if needed. I'm not sure if the OVHD would be worth it though. Just a thought

[wxiaoguang]

Isn't cat-file command tied to a repository?

Yes, it is repo-based command. At the moment, in most cases it is tied to each "request context", and it could also be used independently in a "search indexer". I do not think a pool would help and there is no pool, only some per-request caches.

Question then is what if single cat-file instance is overloaded (I'm assuming it's possible - but I might be wrong)?

Slows down or block repo browsing. cat-file is used to retrieve git entry information.

[lunny]

Isn't cat-file command tied to a repository?

Yes, it is repo-based command. At the moment, in most cases it is tied to each "request context", and it could also be used independently in a "search indexer". I do not think a pool would help and there is no pool, only some per-request caches.

Question then is what if single cat-file instance is overloaded (I'm assuming it's possible - but I might be wrong)?

Slows down or block repo browsing. cat-file is used to retrieve git entry information.

We can have a process pool which keep some idle cat file processes at the background about 5 minutes. So every request will pick one idle or create a new one according to it's repository directory. This method will reduce creating and destroy sub process frequently. The current design will create and destroy cat file sub process per request. This may resolve the windows non-gogit version's performance problem because windows process is heavy.

[yp05327]

I'm afraid this is hard to maintain.
There are too many codes and functions below, it is hard to keep this rule in the future.
Also, what are repository functions?

[lunny]

Yes, it’s a bit complicated, but that’s the current design. This PR doesn’t refactor it—it just improves upon what’s already there. PR #33934 might rework cat-file and help simplify things in the future.