[V6] Add some report-status and send-pack tests #1536
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds new tests for the report-status feature in the SendPack routine and improves error messaging in pktline parsing while introducing typed errors for report-status failures.
- Added tests for buildUpdateRequests covering various capability combinations.
- Introduced UnpackStatusErr and CommandStatusErr types for more informative error messages.
- Updated error messages in pktline write/read functions and hex length parsing for clarity.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plumbing/transport/build_update_requests_test.go | New test cases to validate capabilities for report-status, progress, atomic, and agent. |
| plumbing/protocol/packp/sideband/demux.go | Added check for empty pktline content with an explicit error. |
| plumbing/protocol/packp/report_status_test.go | Extended tests to cover failed command status errors and unpack with failed commands. |
| plumbing/protocol/packp/report_status.go | Refactored error message generation using new error types for unpack/command status errors. |
| plumbing/format/pktline/pktline.go | Updated Writef and Read functions to leverage new fmt.Appendf syntax and improved error text. |
| plumbing/format/pktline/length.go | Changed error messages for hex decoding related to pktline length. |
| plumbing/format/pktline/common.go | Minimal change in import structure. |
| internal/transport/test/receive_pack.go | Removed outdated test assertions tied to report-status in the SendPack tests. |
There was a problem hiding this comment.
@aymanbagabas thanks for working on tests. It largely LGTM, but I think we could drop some of the redudant comments.
| @@ -34,6 +34,9 @@ func (s *ReportStatusSuite) TestError() { | |||
| cs.Status = "ok" | |||
| s.NoError(rs.Error()) | |||
| cs.Status = "OK" | |||
| // According to git protocol, if unpack status is "ok", the overall status | |||
| // is ok even if some command statuses have errors. However, canonical Git | |||
| // still errors on the first received command-status error. | |||
There was a problem hiding this comment.
Is there any upstream link we can add to the comment or commit message?
There was a problem hiding this comment.
Yes, the gitprotocol-pack document states:
Updates can be unsuccessful for a number of reasons. The reference can have changed since the reference discovery phase was originally sent, meaning someone pushed in the meantime. The reference being pushed could be a non-fast-forward reference and the update hooks or configuration could be set to not allow that, etc. Also, some references can be updated while others can be rejected.
With this example:
...
S: 000eunpack ok\n
S: 0018ok refs/heads/debug\n
S: 002ang refs/heads/master non-fast-forward\n
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
This adds a new few tests for
report-statusduring theSendPackroutine. It also adds more informativepktlineerror messages, and typedreport-statuserrors.