go-git #2275: alternative design for opt-in packfile progress

1-implementation.md

Packfile progress: implementation sketch

Illustrative, not compile-ready. The goal is to show where the seams go.

1. The vocabulary — plumbing/progress

Imports stdlib only, so there is no cycle with plumbing, plumbing/format/packfile, plumbing/revlist, plumbing/storer or git.

// Package progress reports progress of local packfile work: counting,
// compressing, writing, receiving and resolving objects.
package progress

// Phase is a unit of work a caller can show progress for. Phases name
// operations rather than implementation steps, so that changes inside the
// delta selector or the index encoder do not alter this vocabulary.
type Phase uint8

const (
	Counting    Phase = iota // enumerating the objects to send
	Compressing              // selecting delta bases for outgoing objects
	Writing                  // encoding objects into the outgoing packfile
	Receiving                // reading the incoming packfile
	Resolving                // reconstructing deltas in the incoming packfile
)

func (p Phase) String() string

// Update is a snapshot of one phase. It is plain data and safe to copy.
type Update struct {
	Phase Phase

	// Current counts objects handled so far.
	Current uint64

	// Total counts objects expected. It is meaningful only when TotalKnown
	// is set. Counting does not learn its total until it finishes.
	Total      uint64
	TotalKnown bool

	// Bytes counts bytes transferred for phases that track them, which is
	// Receiving only. It is zero elsewhere.
	Bytes uint64

	// Done marks the last update for the phase and is the only completion
	// signal. A phase that fails never reports Done.
	Done bool
}

// Reporter receives progress updates.
//
// Report runs synchronously on the goroutine doing the work. That goroutine
// belongs to go-git, not to the caller of Fetch or Push, and during delta
// selection several of them report concurrently. An implementation must be
// safe for concurrent use, must return promptly, and must not call back into
// the operation it is reporting on.
//
// go-git stops calling Report before the operation returns, on success and on
// failure. go-git cannot interrupt a Reporter that blocks.
type Reporter interface {
	Report(Update)
}

// ReporterFunc adapts an ordinary function to Reporter.
type ReporterFunc func(Update)

func (f ReporterFunc) Report(u Update) { f(u) }

Why Done and TotalKnown are separate fields

Inferring completion from Current == Total has two failure modes. It is true of the zero value, so a phase with no objects reports complete on its first update. And it is never true for Counting, whose total is unknown until the walk ends. An explicit flag costs one byte and removes both.

2. The stock implementation — poll instead of push

// State is a Reporter that keeps the latest update per phase so a caller can
// poll at its own rate. It holds a mutex only long enough to store one value,
// and it stays usable after the operation returns.
//
// This is the right choice for a progress bar. A caller that needs every
// update rather than the latest should implement Reporter directly.
type State struct {
	mu     sync.Mutex
	latest [numPhases]Update
	seen   [numPhases]bool
}

func NewState() *State

func (s *State) Report(u Update) {
	s.mu.Lock()
	s.latest[u.Phase], s.seen[u.Phase] = u, true
	s.mu.Unlock()
}

// Snapshot returns the latest update for each phase that has reported, in
// phase order.
func (s *State) Snapshot() []Update

*State satisfies Reporter, so polling is not a second seam. It is one shipped implementation of the only seam.

3. A text renderer, for git-shaped output

// NewTextReporter returns a Reporter that writes git-style progress lines to
// w, at most one line per phase per interval, plus a final line per phase.
// Writes are serialised. A w that blocks still stalls the operation.
//
//	Resolving deltas:  45% (1234/2743)
func NewTextReporter(w io.Writer, interval time.Duration) Reporter

// SyncWriter serialises concurrent writes to w, so that local progress and
// the remote's sideband output can share one destination.
func SyncWriter(w io.Writer) io.Writer

This is how the design meets sideband.Progress without touching it. That type is already interface{ io.Writer }, so the two compose directly.

4. The hot path — internal/progress

// Tracker accumulates progress for one phase and forwards sampled updates to
// a Reporter. A nil Reporter disables it at a cost of one comparison per step.
type Tracker struct {
	r     progress.Reporter
	phase progress.Phase

	current  atomic.Uint64
	bytes    atomic.Uint64
	lastNano atomic.Int64
	// ...
}

// clockStride is how many steps pass between clock reads. Reading the clock
// per object costs more than the work being measured for small objects.
const clockStride = 256

func (t *Tracker) Step() {
	if t.r == nil {
		return
	}
	if n := t.current.Add(1); n%clockStride == 0 {
		t.emitIfDue(n)
	}
}

func (t *Tracker) AddBytes(n uint64)

// Done emits the final update for the phase and is always delivered.
func (t *Tracker) Done()

Counters stay exact. Only the calls into caller code are rate-limited, and the limiting happens before the call, not inside the caller's callback.

This mirrors git: progress.c counts every object but renders only when the percentage changes or a timer fires.

5. Where each phase attaches

Note the rightmost column. Every seam extends a function that already exists, using a variadic option parameter, so no *WithStatus style twin is needed and every existing call site compiles unchanged.

Phase	Op	Seam	New exported funcs
Counting	push	revlist.Objects(…, opts ...revlist.Option)	0
Compressing, Writing	push	existing EncoderOption, add WithEncoderProgress	0
Receiving (bytes)	fetch	counting reader in WritePackfileToObjectStorage	0
Receiving, Resolving	fetch	existing ParserOption, add WithParserProgress	0
Resolving (fs storer)	fetch	storer.PackfileWriter becomes variadic, see §6	0

// Source compatible. Existing callers are untouched.
func Objects(
	s storer.EncodedObjectStorer,
	wants, haves []plumbing.Hash,
	opts ...Option,
) ([]plumbing.Hash, error)

Put the fetch seam inside Parse, not on Observer

Parser.Parse already has exactly git's two phases: the scan loop, then resolveDeltas after it. A seam placed inside Parse can label them Receiving and Resolving correctly.

The existing packfile.Observer cannot. Its OnInflatedObjectHeader fires for non-delta objects during the scan and for deltas during resolution, so it yields one aggregate count across both phases. There are two further reasons not to route progress through it:

• WithScannerObservers replaces the observer slice (p.observers = ob). Exposing it to callers lets them silently drop the mandatory idxfile.Writer observer and produce a corrupt index.
• Parse discards some observer errors (_ = p.onHeader(...), _ = p.storeOrCache(...)), so those hooks cannot be advertised as a cancellation mechanism.

6. The storer seam

packfile.UpdateObjectStorage asserts storer.PackfileWriter and calls pw.PackfileWriter(), which returns an opaque io.WriteCloser. The filesystem storer builds its parser inside dotgit.PackWriter.buildIndex, behind that writer. An interface method has a fixed signature, so nothing can be passed through it.

Receiving in bytes needs nothing here: WritePackfileToObjectStorage already holds the io.Reader, so a counting reader covers both storage paths for free. The gap costs exactly one thing: Resolving progress when the storer writes packfiles, which is the default path and the long phase of a big clone.

Chosen: make the existing methods variadic

type PackfileWriter interface {
	PackfileWriter(opts ...PackfileWriterOption) (io.WriteCloser, error)
}

type PromisorPackfileWriter interface {
	PromisorPackfileWriter(marker string, opts ...PackfileWriterOption) (io.WriteCloser, error)
}

// PackfileWriterOptions configures one packfile write.
type PackfileWriterOptions struct {
	// Progress receives progress for the objects this writer ingests.
	// A nil Reporter disables reporting.
	Progress progress.Reporter
}

type PackfileWriterOption func(*PackfileWriterOptions)

// WithProgress reports progress for the objects the writer ingests.
func WithProgress(r progress.Reporter) PackfileWriterOption {
	return func(o *PackfileWriterOptions) { o.Progress = r }
}

// BuildPackfileWriterOptions applies opts and returns the result.
// Implementations of PackfileWriter use it so option handling stays
// consistent across storers, including out of tree ones.
func BuildPackfileWriterOptions(opts ...PackfileWriterOption) PackfileWriterOptions {
	var o PackfileWriterOptions
	for _, fn := range opts {
		fn(&o)
	}
	return o
}

This is breaking for implementors only. Every call site compiles unchanged, because pw.PackfileWriter() remains a valid call. v6 is at v6.0.0-alpha.5, so the window for it is open.

Both interfaces go variadic. They are not merged. Folding the promisor marker into an option would look tidier and is wrong: remote.go:485 calls SupportsPromisorPacks(r.s) as a pre-flight check, before a filtered fetch starts, to refuse storage that writes packs but cannot mark them. That check is a type assertion on the separate interface. Merging would turn a capability query answered up front into a runtime error discovered after the fetch is under way. Keeping them separate also gets progress into the promisor path, which a filtered clone needs most.

SupportsPromisorPacks in common.go:70 is therefore unchanged.

Considered and rejected

• A third interface, ProgressPackfileWriter, additive and non-breaking. Rejected because the family already has two members and this pattern needs a new one per capability. It also forces a type assertion in storage/transactional, whose else branch silently drops the reporter.
• Bytes-only Receiving, no Resolving on the filesystem path. Zero API change, worst UX on the case that needs it most.
• A SetProgress setter on the returned writer. newPackWrite launches the parser goroutine at construction, so any later set is a race.
• A context-carried reporter. The interface method takes no ctx, and context cancellation cannot interrupt a blocked callback anyway.
• Bypassing the PackfileWriter path when progress is on. That would silently change on-disk layout from packed to loose objects.
• A reporter installed on the storer. Storers are shared across concurrent operations, so the reporter must be scoped to one write.

7. Blast radius

Changed by the variadic decision

File	Change
plumbing/storer/object.go	Both interfaces, plus PackfileWriterOptions / PackfileWriterOption / WithProgress / BuildPackfileWriterOptions. New import of plumbing/progress.
storage/filesystem/object.go	PackfileWriter (:435) and PromisorPackfileWriter (:444) take opts and forward the reporter to dotgit.
storage/filesystem/dotgit/dotgit.go	NewObjectPack (:441) and NewPromisorObjectPack (:463) become variadic.
storage/filesystem/dotgit/writers.go	PackWriter gains a reporter field, newPackWrite takes it, buildIndex passes packfile.WithParserProgress.
storage/transactional/storage.go	:147 and :164 become one-line forwards, return s.pw.PackfileWriter(opts...).
plumbing/format/packfile/common.go	UpdateObjectStorage, WritePackfileToObjectStorage and UpdatePromisorObjectStorage take and forward opts. The counting reader for Receiving bytes lands here. SupportsPromisorPacks unchanged.
remote_test.go:549	Mock adopts the new signature.
plumbing/format/packfile/promisor_test.go:22,31	Both mocks adopt the new signatures.

func (s *packageWriter) PackfileWriter(opts ...storer.PackfileWriterOption) (io.WriteCloser, error) {
	return s.pw.PackfileWriter(opts...)
}

func (w *PackWriter) buildIndex() {
	w.writer = new(idxfile.Writer)

	w.parser = packfile.NewParser(w.synced,
		packfile.WithScannerObservers(w.writer),
		packfile.WithObjectFormat(w.format),
		packfile.WithParserProgress(w.progress), // nil is a no-op
	)
	// ...
}

Compiles unchanged

repository.go:2146-2157, storage/tests/storage_test.go:104,144, plumbing/format/packfile/parser_test.go:191, storage/transactional/storage.go:72, storage/transactional/storage_test.go:62-63, storage/filesystem/storage_test.go:37.

Needed regardless of how the storer seam is solved

internal/transport/transport.go (a LocalProgress field on FetchRequest), internal/transport/v2.go:268,270, plumbing/transport/fetch.go:50,53, plumbing/transport/http/dumb.go:422, plus options.go and remote.go for the LocalProgress fields and the revlist and NewEncoder seams. plumbing/transport/receive_pack.go:195 is server-side unpack and is out of scope.

8. Producer lifetime, as its own commit first

Progress is only safe if reporting provably stops before the operation returns. On push it does not, once a new parking spot exists.

pushHashes spawns the encoder goroutine and, when sess.Push fails, returns after rd.Close() without draining done. That is benign today: the only blocking point is wr.Write, which closing the read end unblocks. Any progress seam adds a parking spot that rd.Close() cannot reach, after which the goroutine outlives Push and a caller that closes its own channel sees a panic.

Fix the join first, independently of this feature:

if err := sess.Push(ctx, s, req); err != nil {
	_ = rd.Close()
	<-done // the encoder goroutine must not outlive Push
	return err
}

On fetch the equivalent join already exists: PackWriter.Close waits on waitBuildIndex.

2-client-api.md

Packfile progress: what it looks like from the outside

Five ways to consume it. go-git starts no goroutine for any of them. The library only calls a method. Whether a goroutine exists is entirely the caller's decision.

1. Nothing at all

The default. One nil comparison per object, no allocation, no behaviour change.

err := repo.Fetch(&git.FetchOptions{RemoteName: "origin"})

2. A progress bar — poll from your own goroutine

*State implements Reporter, so this needs no adapter.

state := progress.NewState()
done := make(chan struct{})

go func() {
	t := time.NewTicker(80 * time.Millisecond)
	defer t.Stop()
	for {
		select {
		case <-t.C:
			render(state.Snapshot())
		case <-done:
			render(state.Snapshot()) // final frame
			return
		}
	}
}()

err := repo.Fetch(&git.FetchOptions{
	RemoteName:    "origin",
	LocalProgress: state,
})
close(done)

State never blocks the operation for longer than one mutex store, and it stays valid after Fetch returns, so the ordering above is not delicate.

3. A channel, with your backpressure policy rather than ours

ch := make(chan progress.Update, 64)

r := progress.ReporterFunc(func(u progress.Update) {
	select {
	case ch <- u:
	default: // drop. Your call.
	}
})

err := repo.Fetch(&git.FetchOptions{LocalProgress: r})

If you would rather block than drop, write ch <- u and own the consequence. The point is that the choice is four visible lines in your code rather than a policy welded into a library type.

4. Metrics or structured logs — no goroutine

opts.LocalProgress = progress.ReporterFunc(func(u progress.Update) {
	metrics.Gauge("git.pack." + u.Phase.String()).Set(float64(u.Current))
	if u.Done {
		log.Info("phase complete", "phase", u.Phase, "objects", u.Current)
	}
})

5. git-shaped text, sharing a writer with the remote's output

sideband.Progress is interface{ io.Writer }, so the stock renderer composes with it directly. No change to that type is required.

out := progress.SyncWriter(os.Stderr)

err := repo.Fetch(&git.FetchOptions{
	// Unchanged meaning: the remote's human readable output.
	//   remote: Counting objects:  100% (2743/2743)
	Progress: out,

	// New: local structured progress, rendered as git-style lines.
	//   Receiving objects:  45% (1234/2743), 5.2 MiB
	//   Resolving deltas:   12% (  42/ 341)
	LocalProgress: progress.NewTextReporter(out, 80*time.Millisecond),
})

Filtered clones report too. Because PromisorPackfileWriter stays a separate interface and gains the same options, a --filter fetch is not a silent gap.

Options surface

type FetchOptions struct {
	// ...

	// Progress is where the human readable information sent by the server is
	// stored. Leaving it nil also sends no-progress to the server.
	Progress sideband.Progress

	// LocalProgress receives structured progress for work done on this side:
	// receiving and resolving objects. Nil disables it.
	LocalProgress progress.Reporter
}

type PushOptions struct {
	// ...
	Progress sideband.Progress

	// LocalProgress receives structured progress for work done on this side:
	// counting, compressing and writing objects. Nil disables it.
	LocalProgress progress.Reporter
}

Why not reuse the existing Progress field

Progress is not only a sink, it is a protocol switch. internal/transport/v2.go sets NoProgress: req.Progress == nil, so a non-nil value tells the server to send progress over the wire. Overloading it would make "I want a local progress bar" silently imply "and send me remote progress too". Two unrelated decisions should not share one field.

A naming question worth settling while v6 is in alpha: if LocalProgress lands, Progress arguably wants to become RemoteProgress.

Implementing a storer

The only audience affected by the breaking change. An out-of-tree storer that implements storer.PackfileWriter adopts the new signature, and the compiler points at every site:

func (s *MyStorage) PackfileWriter(opts ...storer.PackfileWriterOption) (io.WriteCloser, error) {
	o := storer.BuildPackfileWriterOptions(opts...)
	// Ignoring o.Progress is fine. Progress is best effort.
	return s.newPackWriter(o.Progress)
}

3-notes.md

Packfile progress: why this shape, and what it costs

What the design is reacting to

The channel-based design in #2275 has four defects that are properties of the shape rather than bugs to patch:

1. StatusObserver.OnInflatedObjectHeader does a blocking channel send per object. On the filesystem path that runs inside buildIndex, so a consumer slower than the parser parks that goroutine, and PackWriter.Close then parks on waitBuildIndex. Fetch hangs with no context, timeout or error.
2. pushHashes can let the encoder goroutine outlive Push. The caller then closes its channel and the parked sender panics. Note that making sends non-blocking does not fix this: select/default still panics on a closed channel. This is a lifetime problem, not a send-policy problem.
3. The completion send in DeltaSelector.walk happens while holding the shared updateMu, so one parked worker stalls every other delta worker.
4. ObjectsDone == ObjectsTotal is not a completion signal. It is true of the zero value, and it is never true for the counting stage, which increments ObjectsTotal rather than ObjectsDone.

And two structural costs: seven *WithStatus twins across six packages, and an 11-constant enum in plumbing that encodes idxfile's three internal write passes and the delta selector's internal steps. The lowest-level package ends up owning the highest-churn vocabulary.

Why this approach is better

No parallel API surface. Every seam is a variadic option on a function that already exists. Seven new exported functions become zero. No new interface is added at all.

It uses the option families go-git already has. EncoderOption and ParserOption exist and are already how the encoder and parser are configured. Progress is one more option, not a new mechanism.

Phases name operations, not internals. Five stable phases matching what git prints, instead of eleven that pin idxfile's and the delta selector's current implementation into the public API. Reworking either stays invisible.

The phase boundaries are correct. Putting the fetch seam inside Parse distinguishes Receiving (the scan loop) from Resolving (resolveDeltas), which is what git shows and what Observer structurally cannot express, since it fires in both phases.

The library imposes no concurrency contract. It calls a method. There is no "you must drain this concurrently and must not close it", because there is nothing to drain or close. The caller who wants a channel writes four visible lines and picks their own policy.

The safe path is the default path. *State is the recommended consumer and cannot block or panic. Reaching the dangerous behaviour requires deliberately writing a blocking Reporter. In #2275 the library's own type blocks.

One method cannot be half-implemented. storage/transactional becomes a one-line forward. The optional-interface version needs a type assertion there whose else branch silently drops the reporter, handing the caller a channel that never fires. There is no else branch to get wrong.

Filtered clones are not a silent gap. PromisorPackfileWriter gains the same options, so a --filter fetch reports like any other.

Zero cost when off, one comparison per object. When on, exact counters with sampled delivery, which is how progress.c works.

It composes with sideband.Progress without touching it. That type is already an io.Writer, so a stock text renderer targets it directly.

Downsides, honestly

It is a breaking change to two interfaces. storer.PackfileWriter and storer.PromisorPackfileWriter gain a variadic parameter, so every implementation must be updated. In-tree that is three real implementations and two test mocks, and every call site compiles untouched. Out of tree, anyone with a custom storer has to edit one line per method. The compiler finds all of them, and v6 is at alpha.5, but it is still a break in a line people are already building against.

A caller-supplied Reporter can still block or panic. This relocates the policy to the caller and documents it. It does not make it impossible. func(u){ ch <- u } hangs exactly as before. The mitigation is that State is the recommended path, not that the hazard is gone.

Sampling means you do not get every update. Phase transitions and Done are always delivered, intermediates are not. Reporter is a progress feed, not an audit log. Anyone needing exact per-object notification should use the existing low-level Observer directly.

A caller-supplied ObjectSelector gets no Compressing progress. This is deliberate: requiring custom selectors to implement a hidden progress interface would be worse, and batching the selection to observe it would change the delta window and therefore change the pack output.

Variadic options change function type identity. Anyone assigning revlist.Objects to a func(...) variable breaks. Rare, but real, and worth saying out loud rather than claiming the change is purely additive.

Two option names in one package. WithEncoderProgress and WithParserProgress, because packfile holds two option families and Go has no overloading. Mildly awkward. Arguably a hint that parser options want their own package.

More concepts than a channel. Phase, Update, Reporter, State, Tracker against one channel type. The extra surface buys safety and stability, but it is not free, and a reviewer is entitled to weigh it.

It does not fix producer lifetime by itself. The pushHashes join is a prerequisite, not a consequence. Any transport has this requirement.

Open questions for discussion

1. Is Receiving in bytes sufficient, or is the object count worth tracking separately? Git shows both.
2. Should Progress be renamed RemoteProgress alongside adding LocalProgress, while v6 is still in alpha?
3. Should internal/progress.Tracker be exported, so that out-of-tree storer implementations can report consistently rather than rolling their own sampling?
4. Does repack (repository.go:newPackWriter) want Writing progress too? It compiles unchanged today, so this can land later without further churn.