Backport #39531 by @CalvinTjoaquinn
## What
`BatchChecker.CheckPath` uses `time.After` inside its read loop. This
replaces it with a `time.Timer` that is stopped once the attribute
arrives.
## Why
```go
for i := 0; i < c.attributesNum; i++ {
select {
case <-time.After(5 * time.Second):
// there is no "hang" problem now. This code is just used to catch other potential problems.
return nil, reportTimeout()
case attr, ok := <-c.stdOut.ReadAttribute():
```
`time.After` has no way to be cancelled, so the timer it allocates stays
in the runtime timer heap for the full five seconds whichever case the
`select` picks. On the normal path the attribute arrives immediately and
the timer is abandoned while still pending.
The multiplier is what makes it worth changing rather than leaving as
noise. The loop runs `len(LinguistAttributes)` times, which is six, and
`CheckPath` is called once per file:
```go
// modules/git/languagestats/language_stats_get.go:95, in the loop over repository files
attrs, err := checker.CheckPath(f.Name())
// services/gitdiff/gitdiff.go:1408, in the loop over diff files
attrs, err := checker.CheckPath(diffFile.Name)
```
So a language-stats pass over a repository of N files holds up to 6N
pending five-second timers, and a large diff does the same per file. By
the comment's own account that timeout path does not fire in practice,
so every one of those timers is allocated and held for nothing.
## The change
`time.NewTimer` plus `Stop` on the paths that win, which keeps the
behaviour identical: each iteration still gets its own five-second
budget, and the timer is released as soon as the attribute or the
context arrives rather than five seconds later.
If you would rather have a single budget for the whole read, one timer
hoisted above the loop with `defer timeout.Stop()` is simpler and
stricter, since six attributes from an already running `git check-attr`
should arrive together. That changes the semantics from per-attribute to
per-call, so I left it alone and am happy to switch if you prefer it.
## Verification
```
go build ./modules/git/...
go vet ./modules/git/attribute/
go test -count=1 ./modules/git/attribute/ # 11 tests, 0 failures
golangci-lint run ./modules/git/attribute/...
gofmt -l modules/git/attribute/ # no output
```
The package's own `CheckPath` tests cover the success path, the
closed-stdout path and the context-cancelled path, which are the three
`select` arms touched here.
Found with a small AST pass over the tree looking for `time.After`
inside loop bodies. `staticcheck`'s SA1015 covers `time.Tick` and says
nothing about this shape, so no linter in the current set reports it. Of
the nine other hits in the tree the rest look deliberate or harmless,
and `modules/queue/workergroup.go` already guards against exactly this
by only creating a debounce timer when none is pending, so I only
changed this one.
<sub>Disclosure per the AI Contribution Policy: I used an AI tool to
help find this and to draft the description. The counts above are
`len(LinguistAttributes)` and the two call sites cited, so they can be
checked directly.</sub>
Signed-off-by: Calvin Tjoaquinn <calvintjoa23@gmail.com>
Co-authored-by: Calvin Tjoaquinn <66313400+CalvinTjoaquinn@users.noreply.github.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
The "path" details should be hidden to other packages
By the way, fix a resource leaking in gogit's CommitNodeIndex
(the file was not closed in CacheCommit)
before: gitrepo vs git packages
after: git package fully handle all git operations
by the way, use `WithRepo(repo)` instead of `WithDir(repo.Path)` to hide
path details.
benefits:
1. remove all unnecessary wrappers, developers no need to struggle with
"which package should be used"
2. simplify code, RepositoryFacade can (will) be used everywhere, all
"path" details are (will be) hidden
Most potential deadlock problems should have been fixed, and new code is
unlikely to cause new problems with the new design.
Also raise the minimum Git version required to 2.6.0 (released in 2015)
Refactor Git command functions to use WithXXX methods instead of
exposing RunOpts.
This change simplifies reuse across gitrepo and improves consistency,
encapsulation, and maintainability of command options.
---------
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
This PR moved git attributes related code to `modules/git/attribute` sub
package and moved language stats related code to
`modules/git/languagestats` sub package to make it easier to maintain.
And it also introduced a performance improvement which use the `git
check-attr --source` which can be run in a bare git repository so that
we don't need to create a git index file. The new parameter need a git
version >= 2.40 . If git version less than 2.40, it will fall back to
previous implementation.
---------
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: yp05327 <576951401@qq.com>
