plumbing: packp, refactor AdvRefs and capability packages by aymanbagabas · Pull Request #1987 · go-git/go-git

aymanbagabas · 2026-04-13T15:01:57Z

This PR refactors the plumbing/protocol/packp package to simplify the AdvRefs encoder/decoder, clean up the capability package, and remove unnecessary constructors.

Motivation

AdvRefs loses information

The old AdvRefs stored references in two maps (References and Peeled) that lost wire-order information. Most consumers called MakeReferenceSlice() immediately—the maps were an intermediate representation nobody wanted. The git pack protocol defines advertised-refs as an ordered sequence where peeled refs follow their base ref. The current representation didn't preserve this.

Capability type forces conversions

Capability was a defined type requiring conversions like capability.Capability("custom-cap"). Protocol v2's arbitrary command capabilities multiply this friction. A type alias eliminates conversions while keeping the named constants.

capability.List validation is counterproductive

List.Add() and Set() returned errors that callers universally discarded. Every call site discards the error. The validation maps enforce rules that callers already know. Worse, the known map becomes a liability for v2: unknown capabilities bypass validation silently.

Constructors exist only to initialize maps

Every packp constructor (NewAdvRefs, NewUploadRequest, etc.) existed primarily to call capability.NewList(), which existed to make(map[...]). If List is zero-value safe, all constructors become unnecessary.

Changes

capability.List zero-value safety

Added lazy init to List.Add(), Set(), and other methods
Made List zero-value safe—first write creates the internal map
Removed NewList()—use &List{} or zero value

Capability type alias

Changed type Capability string to type Capability = string
Eliminated conversion friction for protocol v2
Removed validation maps and error types
Made Add/Set infallible (no error return)

Wire format moved out of List

Replaced List.Decode with package-level DecodeList
v0/v1 uses space-separated capabilities; v2 uses line-separated
Keeping List protocol-agnostic: it's now a pure container type

AdvRefs slice migration

Replaced References map[string]Hash and Peeled map[string]Hash with single References []*Reference slice
Added ReferenceName.IsPeeled() helper (consistent with IsTag(), IsBranch())
Removed NewAdvRefs(), AddReference(), MakeReferenceSlice(), IsEmpty()

AdvRefs encoder/decoder simplification

Removed state machine pattern from encoder (5 state functions → single Encode() method)
Removed state machine pattern from decoder (8 state functions → single Decode() method with loop)
Net reduction of 145 lines of code

Constructor removal and zero-value safety

Removed NewUploadRequest, NewUpdateRequests, NewReportStatus
Types are zero-value safe: nil slices work with append, Capabilities is value type

DepthRequest struct replaces Depth interface

The old Depth interface used a sum-type pattern (DepthCommits, DepthSince, DepthReference) that couldn't represent the full git protocol. Specifically:

Couldn't combine deepen-since with deepen-not (git allows this)
Couldn't specify multiple deepen-not refs (git uses an oidset)
Required a constructor just to set a non-nil default

The new DepthRequest struct matches how canonical git C handles it:

type DepthRequest struct {
    Commits int           // "deepen <n>" - 0 means no limit
    Since   time.Time     // "deepen-since <timestamp>"
    NotRefs []string      // Multiple "deepen-not <ref>" lines
}

This matches git's upload_pack_data struct which stores depth as separate fields, not a sum type. The zero value (all fields empty/zero) means "no depth constraint" which is the correct default for infinite depth.

Added validation: Following canonical git, the encoder and decoder now reject invalid combinations:

deepen <n> cannot be combined with deepen-since or deepen-not
Error message matches git: "deepen and deepen-since (or deepen-not) cannot be used together"

Benchmarks

Before (transport-migrate/base: 3 runs):

BenchmarkPlainClone-16    3    341770083 ns/op    3742216 B/op    34587 allocs/op
BenchmarkPlainClone-16    3    345509806 ns/op    3472776 B/op    34530 allocs/op
BenchmarkPlainClone-16    4    329814698 ns/op    3562374 B/op    34564 allocs/op

After (packp-refactoring: 3 runs):

BenchmarkPlainClone-16    3    346557236 ns/op    2984754 B/op    29572 allocs/op
BenchmarkPlainClone-16    3    335943014 ns/op    2855368 B/op    29477 allocs/op
BenchmarkPlainClone-16    3    349759278 ns/op    2876133 B/op    29320 allocs/op

Improvement:

Memory: ~3.6MB → ~2.9MB (~19% reduction in allocations)
Allocs: ~34,500 → ~29,500 (~14% reduction)

Migration Guide

Before	After
`packp.NewAdvRefs()`	`&packp.AdvRefs{}`
`ar.References["name"] = hash`	`ar.References = append(ar.References, plumbing.NewHashReference("name", hash))`
`capability.NewList()`	`&capability.List{}`
`caps.Decode(data)`	`capability.DecodeList(data, &caps)`
`depth := DepthCommits(3)`	`depth := DepthRequest{Commits: 3}`

Testing

All existing tests pass:

go test ./plumbing/protocol/packp/...
go test ./plumbing/protocol/capability/...
make validate passes (0 lint issues)

Supersedes: #1984

Copilot

Pull request overview

This PR refactors the Git pack-protocol plumbing to make capability handling zero-value safe and protocol-agnostic, and to preserve advertised-ref ordering by moving AdvRefs to a slice-based representation. It also removes several constructors whose primary job was map initialization and replaces the old depth “sum type” with a struct that can represent more protocol combinations.

Changes:

Move capabilities to plumbing/protocol/capability, make List zero-value safe, and make Capability a string alias; update call sites accordingly.
Refactor packp.AdvRefs to store references as an ordered slice (including peeled refs) and simplify AdvRefs encode/decode.
Replace the Depth interface with DepthRequest and add encoder/decoder validation for invalid deepen combinations; update transport server/client code.

Reviewed changes

Copilot reviewed 47 out of 48 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
remote.go	Switch capability import to new `plumbing/protocol/capability` package.
remote_test.go	Update tests for new capability import and AdvRefs zero-value usage.
plumbing/transport/url_test.go	Update capability list construction to zero-value-safe `List`.
plumbing/transport/upload_pack.go	Update to zero-value-safe request types and `DepthRequest` handling.
plumbing/transport/upload_pack_test.go	Adjust tests for constructor removals and new capability import.
plumbing/transport/serve.go	Update AdvertiseRefs capability setup; migrate AdvRefs reference storage to slice.
plumbing/transport/send_pack_test.go	Update capability list construction and imports.
plumbing/transport/receive_pack.go	Remove constructors for UpdateRequests/ReportStatus; update capability import.
plumbing/transport/push.go	Update SendPack signatures to accept capability `List` by value; remove constructors.
plumbing/transport/pack_stream.go	Store capabilities by value; switch to `ResolvedReferences()` for remote refs.
plumbing/transport/pack_session.go	Update capability import path.
plumbing/transport/negotiate.go	Update to new capability list type and `DepthRequest`.
plumbing/transport/negotiate_test.go	Update tests for zero-value-safe capability list.
plumbing/transport/http/handshake.go	Update AdvRefs creation, capabilities storage, and resolved refs retrieval.
plumbing/transport/http/dumb.go	Update HEAD handling to new AdvRefs API and slice-based refs.
plumbing/transport/fetch.go	Update FetchPack signature to accept capability `List` by value.
plumbing/transport/build_update_requests_test.go	Update tests to use new capability list type/value semantics.
plumbing/reference.go	Add `ReferenceName.IsPeeled()` helper for `^{}` suffix handling.
plumbing/protocol/packp/updreq.go	Make UpdateRequests zero-value safe; capabilities stored by value; remove constructor.
plumbing/protocol/packp/updreq_encode.go	Pass capability list by pointer where needed after value-type change.
plumbing/protocol/packp/updreq_encode_test.go	Update tests to remove constructor usage and new capability import.
plumbing/protocol/packp/updreq_decode.go	Switch to `capability.DecodeList` for parsing capabilities.
plumbing/protocol/packp/updreq_decode_test.go	Update decode tests for zero-value safe UpdateRequests.
plumbing/protocol/packp/ulreq.go	Replace `Depth` interface with `DepthRequest`; capabilities stored by value; remove constructor.
plumbing/protocol/packp/ulreq_encode.go	Encode `DepthRequest` combinations; add validation and support multiple `deepen-not`.
plumbing/protocol/packp/ulreq_encode_test.go	Update tests for `DepthRequest`; add tests for invalid deepen combinations.
plumbing/protocol/packp/ulreq_decode.go	Decode into `DepthRequest`; add validation for invalid deepen combinations; switch to `DecodeList`.
plumbing/protocol/packp/ulreq_decode_test.go	Update decode assertions for `DepthRequest`; add invalid-combination tests.
plumbing/protocol/packp/srvresp_test.go	Update (commented) tests to new capability list construction.
plumbing/protocol/packp/report_status.go	Remove constructor; document zero-value safety.
plumbing/protocol/packp/report_status_test.go	Update tests to instantiate ReportStatus directly.
plumbing/protocol/packp/inforefs.go	Change InfoRefs to store references as an ordered slice (incl. peeled).
plumbing/protocol/packp/common.go	Remove AdvRefs-specific constants now handled via new helpers/logic.
plumbing/protocol/packp/capability/list.go	Remove legacy capability list implementation under `packp/capability`.
plumbing/protocol/packp/capability/list_test.go	Remove tests for deleted legacy capability list.
plumbing/protocol/packp/advrefs.go	Migrate AdvRefs to slice-based refs; add head resolution helpers and resolved refs output.
plumbing/protocol/packp/advrefs_test.go	Update tests for new AdvRefs APIs (resolved refs/head heuristics).
plumbing/protocol/packp/advrefs_encode.go	Simplify AdvRefs encoding; generate peeled output based on `^{}` refs.
plumbing/protocol/packp/advrefs_encode_test.go	Update encode tests for new AdvRefs representation.
plumbing/protocol/packp/advrefs_decode.go	Simplify AdvRefs decoding; decode capabilities via new `DecodeList`.
plumbing/protocol/packp/advrefs_decode_test.go	Update decode tests for slice-based refs and new head accessor.
plumbing/protocol/capability/list.go	Introduce new protocol-level capability `List` and `DecodeList`.
plumbing/protocol/capability/list_test.go	Add tests for new capability list (currently not exercising `DecodeList`).
plumbing/protocol/capability/capability.go	Make `Capability` a `string` alias; remove validation tables.
plumbing/protocol/capability/capability_test.go	Add tests for `DefaultAgent` env var behavior.
internal/transport/test/upload_pack.go	Update capability import path.
internal/transport/test/receive_pack.go	Update capability import path.
backend/http_test.go	Update capability import path for expected advertise-refs output.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-13T15:12:07Z

+// firstRef returns the reference to use as the first line (HEAD or first available).
+func (a *AdvRefs) firstRef() (string, plumbing.Hash) {
+	for _, ref := range a.References {
+		if ref.Name().IsPeeled() {
+			continue
+		}
+		if ref.Name() == plumbing.HEAD {
+			return ref.Name().String(), ref.Hash()
+		}
 	}
-	sort.Strings(ret)
-
-	return ret
-}
-
-func encodeFlush(e *advRefsEncoder) encoderStateFn {
-	e.err = pktline.WriteFlush(e.w)
-	return nil
+	for _, ref := range a.References {
+		if ref.Name().IsPeeled() {
+			continue
+		}
+		return ref.Name().String(), ref.Hash()
+	}
+	return "", plumbing.ZeroHash


firstRef() selects the first advertised ref based on the current slice order, but Encode sorts refs for output. If HEAD is absent (e.g. receive-pack), this can make the first pkt-line nondeterministic and can also drop the peeled line for that first ref (because the peeled emission only happens in the sorted loop that skips firstName). Consider choosing the first ref from the same sorted set used for encoding, and ensure a peeled ^{} line is emitted immediately after the first ref when applicable.

pjbgf

@aymanbagabas thanks for working on this. Here's some feedback on the refactoring.

pjbgf · 2026-04-23T10:45:48Z

+}
+
+// Add adds a capability, values are optional
+func (l *List) Add(c string, values ...string) {


The latest changes dropped some of the validation. The specs are fairly strict around capabilities:

server MUST NOT advertise capabilities it does not understand.

[session-id] ... must fit within a packet-line, and must not contain non-printable or whitespace characters

Let's bring back validation and ensure that we check:

Argument-less caps

Caps that require arguments

Any argument specific validation defined by the spec

Only known caps are accepted

Adding tests to confirm the above.

I added a capability.Validate method to validate v0/v1 capabilities, added the missing session-id capability with tests, and updated client-side and server-side capability validation during handshake.

pjbgf · 2026-04-23T10:54:31Z

+			req.Depth.DeepenNot = append(req.Depth.DeepenNot, string(line))
+			deepenRevList = true


Upstream does some validation here for ambiguity. Probably not something we'd at decoding time, but may be worth including as part of this PR.

IIRC, we still don't fully support deepen-not. We can revisit this later when we add support.

aymanbagabas · 2026-04-25T21:01:06Z

I think the existing idxfile issue that is shown in the CI should be fixed by #2027

Add internal init() method that creates the map on first write. NewList() is deprecated; its body now returns &List{}. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

…dation type Capability = string eliminates conversion friction for protocol v2. Remove known/requiresArgument/multipleArgument maps and error types. Add/Set no longer return errors; update all call sites. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

…String Remove method List.Decode, add package-level DecodeList for v0/v1 space-separated wire format. Keep List.String for encoding. Wire format decoding now lives in the capability package as a free function rather than a method, making List a pure container. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

Move plumbing/protocol/packp/capability to plumbing/protocol/capability to match git protocol structure and prepare for v2. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

Remove deprecated NewList() in favor of the zero value. Fix capability ordering to ensure deterministic encoding. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

Replace AdvRefs.References (map[string]Hash) and Peeled (map[string]Hash) with a single References []*plumbing.Reference slice. Remove NewAdvRefs(), AddReference(), IsEmpty(), and MakeReferenceSlice(). Change Capabilities from *capability.List to capability.List. This makes AdvRefs zero-value safe: slices are appendable from nil, and Capabilities is a value type. Add IsPeeled(), RefsToMap(), PeeledToMap() helpers to plumbing/reference.go. DecodeList and IsEmpty get nil-receiver guards. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

Remove NewUploadRequest, NewUpdateRequests, and NewReportStatus. Their types are now zero-value safe: slices are appendable from nil, and Capabilities is a value type. The encoder guards against nil Depth. Set Depth to DepthCommits(0) in the decoder so decoded requests always have a non-nil Depth. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

Replace the Depth sum-type interface (DepthCommits, DepthSince, DepthReference) with a single DepthRequest struct containing Commits, Since, and NotRefs fields. This matches git upload_pack_data struct, supports the full protocol (deepen-since + deepen-not combination, multiple deepen-not refs), and makes the zero value safe. Update encoder, decoder, and all consumers. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

…plumbing Remove exported IsPeeled, RefsToMap, and PeeledToMap from plumbing/reference.go. Move isPeeled and peeledToMap as internal functions in the packp package. Update tests to iterate References directly instead of using helper functions. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

Move the peeled check from internal packp function to ReferenceName.IsPeeled(), consistent with other reference name predicates like IsTag() and IsBranch(). Remove the internal isPeeled function and update all call sites to use ref.Name().IsPeeled() instead. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

Remove state machine pattern from AdvRefs encoder and decoder. The original implementation had complex state functions (encodeFirstLine, encodeRefs, encodeShallow, encodeFlush for encoder; decodeFirstHash, decodeFirstRef, decodeCaps, decodeOtherRefs, decodeShallow for decoder). New encoder: single Encode method with sequential steps. New decoder: single Decode method with sequential loop. Add ReferenceName.IsPeeled helper and use it throughout. Move isPeeled and peeledToMap as internal functions in packp package. Remove exported IsPeeled, RefsToMap, PeeledToMap from plumbing. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

Prevent combining deepen <n> with deepen-since or deepen-not, following canonical git's validation in upload-pack.c: `deepen and deepen-since (or deepen-not) cannot be used together` Add validation to both encoder and decoder, with tests for all invalid combinations. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

…atching protocol lines Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

…achine Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

…ents Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

pjbgf · 2026-05-04T12:41:59Z

 	ar := s.testDecodeOK(payloads)
-	s.Nil(ar.Head)
+	_, err := ar.Head()
+	s.ErrorIs(plumbing.ErrReferenceNotFound, err)


Suggested change

s.ErrorIs(plumbing.ErrReferenceNotFound, err)

s.ErrorIs(err, plumbing.ErrReferenceNotFound)

pjbgf

@aymanbagabas thanks for working on this. 🙇

aymanbagabas requested review from Copilot and pjbgf April 13, 2026 15:02

Copilot started reviewing on behalf of aymanbagabas April 13, 2026 15:04 View session

Copilot AI reviewed Apr 13, 2026

View reviewed changes

aymanbagabas force-pushed the packp-refactoring branch from cad62f8 to 57f02a3 Compare April 16, 2026 22:14

pjbgf requested changes Apr 23, 2026

View reviewed changes

aymanbagabas requested a review from pjbgf April 24, 2026 03:00

aymanbagabas force-pushed the packp-refactoring branch 2 times, most recently from 9c2ba12 to dbbab8e Compare April 24, 2026 19:37

aymanbagabas mentioned this pull request Apr 25, 2026

plumbing: pktline, use Scanner for decoding #2038

Open

aymanbagabas force-pushed the packp-refactoring branch from dbbab8e to 3c2a32e Compare April 30, 2026 11:50

aymanbagabas added 13 commits May 2, 2026 15:27

plumbing: capability, make List zero-value safe with lazy initialization

dfce449

Add internal init() method that creates the map on first write. NewList() is deprecated; its body now returns &List{}. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

plumbing: protocol/capability, move capability package adjacent to packp

360cae7

Move plumbing/protocol/packp/capability to plumbing/protocol/capability to match git protocol structure and prepare for v2. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

plumbing: capability, remove NewList and fix capability ordering

5ca8426

Remove deprecated NewList() in favor of the zero value. Fix capability ordering to ensure deterministic encoding. Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

plumbing: packp, fix lint error in advrefs_decode

bc1059b

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

aymanbagabas force-pushed the packp-refactoring branch from 3c2a32e to 7a9706e Compare May 2, 2026 19:29

aymanbagabas added 4 commits May 2, 2026 15:36

plumbing: transport, fix unreachable haves sideband progress test

2aaf753

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

plumbing: packp, rename DepthRequest fields to use "deepen" prefix, m…

e7b7c19

…atching protocol lines Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

plumbing: packp, refactor packp decoding and encoding, remove state m…

34f6afa

…achine Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

plumbing: capability, document that the List is not thread safe

1b82b9e

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

aymanbagabas added 7 commits May 2, 2026 15:36

plumbing: capability, add validation for capabilities and their argum…

9267aed

…ents Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

plumbing: capability, add SessionID capability with validation

43e2973

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

plumbing: transport, validate capabilities in handshake and serve

cc373a2

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

plumbing: capability, combine tests into table driven cases

8bc7054

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

plumbing: packp, advrefs: use ErrorIs in tests

445cf09

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

plumbing: packp, use deepen mutually exclusive error

7f1a0da

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

plumbing: packp, remove unused io.ReadCloser cast

75fdf13

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

aymanbagabas force-pushed the packp-refactoring branch from 7a9706e to 75fdf13 Compare May 2, 2026 19:36

pjbgf reviewed May 4, 2026

View reviewed changes

pjbgf approved these changes May 4, 2026

View reviewed changes

pjbgf merged commit 8909149 into go-git:main May 4, 2026
24 of 25 checks passed

		req.Depth.DeepenNot = append(req.Depth.DeepenNot, string(line))
		deepenRevList = true

	s.ErrorIs(plumbing.ErrReferenceNotFound, err)
	s.ErrorIs(err, plumbing.ErrReferenceNotFound)

Conversation

aymanbagabas commented Apr 13, 2026

Motivation

AdvRefs loses information

Capability type forces conversions

capability.List validation is counterproductive

Constructors exist only to initialize maps

Changes

capability.List zero-value safety

Capability type alias

Wire format moved out of List

AdvRefs slice migration

AdvRefs encoder/decoder simplification

Constructor removal and zero-value safety

DepthRequest struct replaces Depth interface

Benchmarks

Migration Guide

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pjbgf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pjbgf Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

aymanbagabas Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjbgf Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

aymanbagabas Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

aymanbagabas commented Apr 25, 2026

Uh oh!

pjbgf May 4, 2026

Choose a reason for hiding this comment

Uh oh!

pjbgf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aymanbagabas Apr 24, 2026 •

edited

Loading