[v6] Update and fix transport tests #1496
Conversation
When encountering an empty repository, we should not error during the Handshake routine, instead, it should be during the GetRemoteRefs routine.
Non existent repositories should always return an error during a session handshake.
When a zero hash is used as the head of an advrefs, it should be considered empty. For example, when an empty repository is cloned using the dumb http protocol, the head will be set to a zero hash.
Respect context cancellation in the transport layer.
There was a problem hiding this comment.
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
plumbing/transport/http/dumb.go:71
- [nitpick] Consider renaming the local variable 'url' to avoid shadowing the imported package 'net/url' for better clarity.
url, err := url.JoinPath(r.ep.String(), "objects", "info", "packs")
plumbing/transport/http/common.go:341
- [nitpick] Consider renaming the local variable 'url' to avoid shadowing the imported package 'net/url', which can prevent potential confusion.
url, err := url.JoinPath(s.ep.String(), infoRefsPath)
1c495e0 to
db78ae2
Compare
db78ae2 to
8f75151
Compare
071a96a to
287ccf7
Compare
|
Will mark as ready once we figure out why these tests are failing. Looks like some sort of race conditions 🤔 |
There was a problem hiding this comment.
@aymanbagabas thank you for working on this.
For the failing tests, the error on Windows seems to be related to a file not being closed:
C:\Users\RUNNER~1\AppData\Local\Temp\TestDumbSuiteTestUploadPackWithContext3676574280\001\go-git-http-63925\basic.git\objects\pack\pack-a3fed42da1e8189a077c0e6846c040dcf73fc9dd.idx.temp3558511417: The process cannot access the file because it is being used by another process.
Looking into the source, I'd say the problem is the lack of a defer f.Close() after this.
For MacOS, my take the issue is that either by the time the daemon actually starts running the port is no longer available or the OS is not releasing the port fast enough for it to be reused.
PS: On a second execution the MacOS issue self-resolved. If this becomes a recurrent issue we may need to revisit this by potentially choosing not to bind the port in FreePort for that OS.
| // s.Require().NotNil(report, comment) | ||
| // s.Require().NoError(report.Error(), comment) |
There was a problem hiding this comment.
Is this commented while ReportStatus cap is implemented?
There was a problem hiding this comment.
This used to test the ReportStatus response when it was returned during the ReceivePackSession routine. Now, ReportStatus responses are not exposed and handled during the new transport.ReceivePack. We can add tests to cover this case in follow-up PRs.
| // s.Equal("ok", report.UnpackStatus) | ||
| // s.Len(report.CommandStatuses, 1) | ||
| // s.Equal(plumbing.ReferenceName("refs/heads/master"), report.CommandStatuses[0].ReferenceName) | ||
| // s.Require().Equal("ok", report.UnpackStatus) |
There was a problem hiding this comment.
Can we move this to teh SendPack tests instead?
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
Co-authored-by: Paulo Gomes <pjbgf@linux.com>
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
Co-authored-by: Paulo Gomes <pjbgf@linux.com>
Co-authored-by: Paulo Gomes <pjbgf@linux.com>
No description provided.