git@cosmocr.at/gitweb.git

git@cosmocr.at/gitweb.git · summary refs

githttp: end the upload-pack negotiation round before sending a pack

commit 3bed207dd783401dd52a490f2af60f290d1719c8

Patrick Gallagher <rpg137@gmail.com> · 2026-09-20 20:28 -0400

Over stateless HTTP git negotiates across several requests. It offers its
haves in batches -- the first holds 16 -- and a batch that is not its last
ends with a flush-pkt and no "done". In that round the client is reading
ACK/NAK pkt-lines to decide what to offer next, so a packfile must not
follow. gitweb sent one anyway, and the client read the literal "PACK"
where a pkt-line length belongs:

    fatal: protocol error: bad line length character: PACK

Clone was unaffected: it sends its wants and "done" in a single request
with no haves, so the pack is expected. A fetch only tripped the bug once
the client had at least 16 haves to offer; below that it exhausts them and
appends "done" to the first request. That is why cloning worked while
fetching a 57-commit repo did not.

The cause is that UploadRequest.Decode stops at the flush-pkt ending the
want section, so the have/done section was never read off the body. Parse
it, answer a round without "done" with a bare NAK, and send the pack only
once the client has finished. We never ACK, so the client keeps offering
haves until it runs out and says "done".

Parsing the haves also fixes the full-history transfer the package doc
described as a go-git limitation. The session already subtracts everything
reachable from req.Haves; that field was simply always empty. A fetch now
sends the delta -- for the two-commit case in the test, 3 objects instead
of 6.

Tests: an end-to-end fetch through the real git binary over a 24-commit
history (deep enough to force a second negotiation round), and unit tests
pinning both halves of the pkt-line contract plus the have-trimming.

internal/githttp/githttp.go
	// Package githttp implements the git "smart HTTP" wire protocol on top of
	// go-git's server transport sessions. go-git ships the upload-pack/receive-pack
	// session logic and the pkt-line/packp encoders, but not a net/http handler, so
	// this package supplies the framing that stock `git` expects:
	//
	// GET <repo>/info/refs?service=git-upload-pack -> ref advertisement
	// GET <repo>/info/refs?service=git-receive-pack -> ref advertisement
	// POST <repo>/git-upload-pack -> pack negotiation (clone/fetch)
	// POST <repo>/git-receive-pack -> ref updates (push)
	//
	// Only protocol v0 is implemented. A client that sends "Git-Protocol: version=2"
	// gets no v2 response and transparently falls back to v0. go-git's server does
-	// not support shallow (--depth) clones, and it ignores the client's "have"
-	// lines, so an incremental fetch re-sends the full history — fine for clone,
-	// suboptimal for fetch.
+	// not support shallow (--depth) clones.
	package githttp

	import (
+	"bytes"
	"compress/gzip"
	"context"
	"fmt"
	"io"
	"net/http"

+	"github.com/go-git/go-git/v5/plumbing"
	"github.com/go-git/go-git/v5/plumbing/format/pktline"
	"github.com/go-git/go-git/v5/plumbing/protocol/packp"
	"github.com/go-git/go-git/v5/plumbing/storer"
	"github.com/go-git/go-git/v5/plumbing/transport"
	"github.com/go-git/go-git/v5/plumbing/transport/server"
	)

	// maxBody bounds a POST body (the packfile for push, or the negotiation for
	// fetch) to guard against a malicious or runaway client.
	const maxBody = 256 << 20 // 256 MiB

	// Service identifies a git smart-HTTP service.
	type Service string

	const (
	UploadPack Service = "git-upload-pack" // clone / fetch
	ReceivePack Service = "git-receive-pack" // push
	)

	// fixedLoader is a go-git transport loader that always serves one storer,
	// ignoring the requested endpoint. gitweb resolves the repo before dispatch, so
	// each request already has its storer.
	type fixedLoader struct{ s storer.Storer }

	func (l fixedLoader) Load(*transport.Endpoint) (storer.Storer, error) { return l.s, nil }

	func newUploadSession(s storer.Storer) (transport.UploadPackSession, error) {
	return server.NewServer(fixedLoader{s}).NewUploadPackSession(&transport.Endpoint{}, nil)
	}

	// NewReceivePackSession returns a receive-pack session bound to s. It is
	// exported so the push write-back path (which must run the session against a
	// private, writable storer) can reuse the same construction.
	func NewReceivePackSession(s storer.Storer) (transport.ReceivePackSession, error) {
	return server.NewServer(fixedLoader{s}).NewReceivePackSession(&transport.Endpoint{}, nil)
	}

	// InfoRefs writes the ref advertisement for svc over s, in smart-HTTP form:
	// the "# service=<svc>" banner and a flush, followed by the pkt-line ref list
	// (with capabilities). The caller must not have written a body yet.
	func InfoRefs(w http.ResponseWriter, s storer.Storer, svc Service) error {
	var ar *packp.AdvRefs
	var err error
	switch svc {
	case UploadPack:
	sess, e := newUploadSession(s)
	if e != nil {
	return e
	}
	defer sess.Close()
	ar, err = sess.AdvertisedReferencesContext(context.Background())
	case ReceivePack:
	sess, e := NewReceivePackSession(s)
	if e != nil {
	return e
	}
	defer sess.Close()
	ar, err = sess.AdvertisedReferencesContext(context.Background())
	default:
	return fmt.Errorf("githttp: unknown service %q", svc)
	}
	if err != nil {
	return err
	}

	w.Header().Set("Content-Type", fmt.Sprintf("application/x-%s-advertisement", svc))
	w.Header().Set("Cache-Control", "no-cache")

	enc := pktline.NewEncoder(w)
	if err := enc.Encodef("# service=%s\n", svc); err != nil {
	return err
	}
	if err := enc.Flush(); err != nil {
	return err
	}
	return ar.Encode(w)
	}

+	// negotiation is the tail of an upload-pack request: the "have" lines the
+	// client is offering, and whether it closed the request with "done".
+	type negotiation struct {
+	haves []plumbing.Hash
+	done bool
+	}
+
+	// decodeNegotiation reads the have/done section that follows the upload
+	// request. UploadRequest.Decode stops at the flush-pkt ending the want section,
+	// so this part of the body is still unread; without it req.Haves stays empty
+	// and every fetch re-sends the full history.
+	func decodeNegotiation(r io.Reader) (negotiation, error) {
+	var n negotiation
+	sc := pktline.NewScanner(r)
+	for sc.Scan() {
+	line := bytes.TrimSuffix(sc.Bytes(), []byte("\n"))
+	switch {
+	case len(line) == 0:
+	// flush-pkt: end of this batch of haves.
+	case bytes.Equal(line, []byte("done")):
+	n.done = true
+	return n, nil
+	case bytes.HasPrefix(line, []byte("have ")):
+	text := string(bytes.TrimPrefix(line, []byte("have ")))
+	h := plumbing.NewHash(text)
+	if h.IsZero() {
+	return n, fmt.Errorf("malformed have line %q", line)
+	}
+	n.haves = append(n.haves, h)
+	}
+	}
+	return n, sc.Err()
+	}
+
	// ServeUploadPack handles a POST git-upload-pack: it decodes the client's
	// negotiation from the (possibly gzip-compressed) body, runs upload-pack over
	// s, and streams the packfile response.
+	//
+	// Over stateless HTTP the client negotiates across several requests. It offers
+	// its haves in batches — the first holds 16 — and ends a batch that is not its
+	// last with a flush-pkt and no "done". In that round it is reading ACK/NAK
+	// pkt-lines to decide what to offer next, so a packfile must not be sent: the
+	// client would read the literal "PACK" where a pkt-line length belongs and fail
+	// with "protocol error: bad line length character: PACK". We answer such a
+	// round with a bare NAK. Since we never ACK, the client keeps offering haves
+	// until it runs out, then sends "done" and gets the pack.
	func ServeUploadPack(w http.ResponseWriter, r *http.Request, s storer.Storer) error {
	body, err := requestBody(r)
	if err != nil {
	return err
	}
	defer body.Close()

	req := packp.NewUploadPackRequest()
	if err := req.Decode(body); err != nil {
	return err
	}
+	neg, err := decodeNegotiation(body)
+	if err != nil {
+	return err
+	}
+	// The session excludes everything reachable from the haves, so passing them
+	// on is what keeps a fetch from re-sending history the client already has.
+	req.Haves = neg.haves
+
+	if !neg.done {
+	setUploadPackHeaders(w)
+	return pktline.NewEncoder(w).Encode([]byte("NAK\n"))
+	}

	sess, err := newUploadSession(s)
	if err != nil {
	return err
	}
	defer sess.Close()

	resp, err := sess.UploadPack(r.Context(), req)
	if err != nil {
	return err
	}

+	setUploadPackHeaders(w)
+	return resp.Encode(w)
+	}
+
+	func setUploadPackHeaders(w http.ResponseWriter) {
	w.Header().Set("Content-Type", fmt.Sprintf("application/x-%s-result", UploadPack))
	w.Header().Set("Cache-Control", "no-cache")
-	return resp.Encode(w)
	}

	// DecodeReceivePack parses a POST git-receive-pack body into a reference-update
	// request. The returned request's Packfile is a streaming reader over the rest
	// of the body; the caller runs the receive-pack session (see the push package)
	// and then calls WriteReceivePackResult.
	func DecodeReceivePack(r http.Request) (packp.ReferenceUpdateRequest, error) {
	body, err := requestBody(r)
	if err != nil {
	return nil, err
	}
	// Note: body is not closed here; req.Packfile streams from it and is closed
	// by the receive-pack session.
	req := packp.NewReferenceUpdateRequest()
	if err := req.Decode(body); err != nil {
	body.Close()
	return nil, err
	}
	// A pack follows the commands only when at least one ref is created or
	// updated; a delete-only push sends none (same rule as stock
	// receive-pack). Decode leaves the body reader in req.Packfile regardless,
	// and go-git's session unpacks whenever Packfile is non-nil, failing on
	// the packless stream ("empty packfile") — so nil it out. Detection must
	// come from the commands, not from probing the body: a read would block,
	// because the client transmits nothing further until it sees our response.
	needPack := false
	for _, cmd := range req.Commands {
	if cmd.Action() != packp.Delete {
	needPack = true
	break
	}
	}
	if !needPack {
	body.Close()
	req.Packfile = nil
	}
	return req, nil
	}

	// WriteReceivePackResult writes the report-status for a completed push. A nil
	// status (client did not request report-status) writes nothing.
	func WriteReceivePackResult(w http.ResponseWriter, rs *packp.ReportStatus) error {
	w.Header().Set("Content-Type", fmt.Sprintf("application/x-%s-result", ReceivePack))
	w.Header().Set("Cache-Control", "no-cache")
	if rs == nil {
	return nil
	}
	return rs.Encode(w)
	}

	// requestBody returns the request body, transparently decompressing it when the
	// client sent Content-Encoding: gzip (stock git gzips larger request bodies),
	// and capping the amount read.
	func requestBody(r *http.Request) (io.ReadCloser, error) {
	var body io.ReadCloser = http.MaxBytesReader(nil, r.Body, maxBody)
	if r.Header.Get("Content-Encoding") == "gzip" {
	zr, err := gzip.NewReader(body)
	if err != nil {
	body.Close()
	return nil, err
	}
	return gzipBody{zr: zr, under: body}, nil
	}
	return body, nil
	}

	// gzipBody couples a gzip reader with the underlying capped body so both are
	// closed together.
	type gzipBody struct {
	zr *gzip.Reader
	under io.ReadCloser
	}

	func (g gzipBody) Read(p []byte) (int, error) { return g.zr.Read(p) }
	func (g gzipBody) Close() error {
	err := g.zr.Close()
	if cerr := g.under.Close(); err == nil {
	err = cerr
	}
	return err
	}
added internal/githttp/uploadpack_negotiation_test.go
+	package githttp
+
+	import (
+	"bytes"
+	"encoding/binary"
+	"fmt"
+	"net/http/httptest"
+	"testing"
+	"time"
+
+	"github.com/go-git/go-billy/v5/memfs"
+	"github.com/go-git/go-git/v5"
+	"github.com/go-git/go-git/v5/plumbing"
+	"github.com/go-git/go-git/v5/plumbing/object"
+	"github.com/go-git/go-git/v5/storage/memory"
+	)
+
+	// pktLine encodes s as a pkt-line.
+	func pktLine(s string) string { return fmt.Sprintf("%04x%s", len(s)+4, s) }
+
+	// twoCommitRepo builds an in-memory repo with two commits and returns its
+	// storer plus the two hashes, oldest first.
+	func twoCommitRepo(t testing.T) (memory.Storage, plumbing.Hash, plumbing.Hash) {
+	t.Helper()
+	st := memory.NewStorage()
+	repo, err := git.Init(st, memfs.New())
+	if err != nil {
+	t.Fatal(err)
+	}
+	wt, err := repo.Worktree()
+	if err != nil {
+	t.Fatal(err)
+	}
+	var hashes []plumbing.Hash
+	for i := range 2 {
+	f, err := wt.Filesystem.Create("f.txt")
+	if err != nil {
+	t.Fatal(err)
+	}
+	fmt.Fprintf(f, "v%d\n", i)
+	f.Close()
+	if _, err := wt.Add("f.txt"); err != nil {
+	t.Fatal(err)
+	}
+	h, err := wt.Commit(fmt.Sprintf("commit %d", i), &git.CommitOptions{
+	Author: &object.Signature{Name: "T", Email: "t@e.com", When: time.Unix(int64(1700000000+i), 0)},
+	})
+	if err != nil {
+	t.Fatal(err)
+	}
+	hashes = append(hashes, h)
+	}
+	return st, hashes[0], hashes[1]
+	}
+
+	// TestServeUploadPackNegotiationRoundSendsNoPackfile pins the pkt-line contract
+	// for a negotiation round that the client has not finished.
+	//
+	// Over stateless HTTP, git offers its haves in batches: the first batch ends
+	// with a flush-pkt and no "done", and the client then reads only ACK/NAK
+	// pkt-lines while it decides what to offer next. Emitting the packfile in that
+	// round desynchronises the client, which reads the literal bytes "PACK" where a
+	// 4-hex pkt-line length belongs and aborts with
+	//
+	// fatal: protocol error: bad line length character: PACK
+	//
+	// The packfile may only follow once the client has sent "done".
+	func TestServeUploadPackNegotiationRoundSendsNoPackfile(t *testing.T) {
+	st, first, tip := twoCommitRepo(t)
+
+	// want <tip>, flush, have <first>, flush -- deliberately no "done".
+	body := pktLine("want "+tip.String()+" ofs-delta agent=git/test\n") +
+	"0000" +
+	pktLine("have "+first.String()+"\n") +
+	"0000"
+
+	r := httptest.NewRequest("POST", "/x/git-upload-pack", bytes.NewReader([]byte(body)))
+	rec := httptest.NewRecorder()
+	if err := ServeUploadPack(rec, r, st); err != nil {
+	t.Fatalf("ServeUploadPack: %v", err)
+	}
+
+	got := rec.Body.Bytes()
+	if i := bytes.Index(got, []byte("PACK")); i >= 0 {
+	t.Errorf("response to an unfinished negotiation round contains a packfile at byte %d; "+
+	"only ACK/NAK pkt-lines are allowed until the client sends \"done\"\nresponse starts: %q",
+	i, got[:min(len(got), 32)])
+	}
+	}
+
+	// TestServeUploadPackDoneSendsPackfile is the companion: once the client says
+	// "done", the packfile is expected in the same response.
+	func TestServeUploadPackDoneSendsPackfile(t *testing.T) {
+	st, _, tip := twoCommitRepo(t)
+
+	body := pktLine("want "+tip.String()+" ofs-delta agent=git/test\n") +
+	"0000" +
+	pktLine("done\n")
+
+	r := httptest.NewRequest("POST", "/x/git-upload-pack", bytes.NewReader([]byte(body)))
+	rec := httptest.NewRecorder()
+	if err := ServeUploadPack(rec, r, st); err != nil {
+	t.Fatalf("ServeUploadPack: %v", err)
+	}
+	if !bytes.Contains(rec.Body.Bytes(), []byte("PACK")) {
+	t.Errorf("response to a finished negotiation has no packfile: %q",
+	rec.Body.Bytes()[:min(rec.Body.Len(), 32)])
+	}
+	}
+
+	// packObjectCount returns the object count from the header of the packfile
+	// embedded in an upload-pack response: "PACK", a 4-byte version, then a 4-byte
+	// big-endian count.
+	func packObjectCount(t *testing.T, resp []byte) int {
+	t.Helper()
+	i := bytes.Index(resp, []byte("PACK"))
+	if i < 0 {
+	t.Fatalf("response has no packfile: %q", resp[:min(len(resp), 32)])
+	}
+	if len(resp) < i+12 {
+	t.Fatalf("truncated packfile header")
+	}
+	return int(binary.BigEndian.Uint32(resp[i+8 : i+12]))
+	}
+
+	// TestServeUploadPackHonoursHaves checks that a "have" the server also holds
+	// trims the packfile. The haves arrive after the flush-pkt that ends the want
+	// section, which UploadRequest.Decode does not read past; left unparsed they
+	// never reach the session and every fetch re-sends the whole history.
+	func TestServeUploadPackHonoursHaves(t *testing.T) {
+	st, first, tip := twoCommitRepo(t)
+
+	post := func(body string) []byte {
+	t.Helper()
+	r := httptest.NewRequest("POST", "/x/git-upload-pack", bytes.NewReader([]byte(body)))
+	rec := httptest.NewRecorder()
+	if err := ServeUploadPack(rec, r, st); err != nil {
+	t.Fatalf("ServeUploadPack: %v", err)
+	}
+	return rec.Body.Bytes()
+	}
+
+	want := pktLine("want "+tip.String()+" ofs-delta agent=git/test\n") + "0000"
+
+	// A client with nothing gets the full history: 2 commits, 2 trees, 2 blobs.
+	full := packObjectCount(t, post(want+pktLine("done\n")))
+	if full != 6 {
+	t.Fatalf("full clone pack = %d objects, want 6", full)
+	}
+
+	// A client that already has the first commit gets only the second's objects.
+	delta := packObjectCount(t, post(want+pktLine("have "+first.String()+"\n")+pktLine("done\n")))
+	if delta >= full {
+	t.Errorf("pack with have %s = %d objects, full = %d: the have was ignored",
+	first.String()[:8], delta, full)
+	}
+	if delta != 3 {
+	t.Errorf("pack with have = %d objects, want 3 (one commit, tree, blob)", delta)
+	}
+	}
added internal/server/fetch_test.go
+	package server
+
+	import (
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"gitweb/internal/store"
+	)
+
+	// TestIncrementalFetch covers a fetch by a client that already holds most of
+	// the history. Clone never exercises negotiation: it sends "want" lines and
+	// "done" in one request, with no "have" lines at all. A fetch does, and git
+	// splits negotiation across requests — it offers its first batch of haves
+	// (INITIAL_FLUSH, 16) terminated by a flush-pkt and no "done", then waits for
+	// the server's ACK/NAK before deciding what to ask for next. A server that
+	// answers that round with a packfile breaks the client, which is still reading
+	// pkt-lines: it reports
+	//
+	// fatal: protocol error: bad line length character: PACK
+	//
+	// The history therefore has to be deep enough that git does not exhaust its
+	// haves (and so append "done") in the first round.
+	func TestIncrementalFetch(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+	t.Skip("git not available")
+	}
+	env := gitEnv(t)
+	ns := t.TempDir()
+
+	const owner = "u@d.com"
+	const repoPath = owner + "/deep.git"
+	bare := filepath.Join(ns, filepath.FromSlash(repoPath))
+
+	// A history comfortably deeper than git's 16-have first batch.
+	const depth = 24
+	work := t.TempDir()
+	mustGit(t, env, work, "init", "-q", "-b", "main")
+	for i := range depth {
+	writeTestFile(t, work, "f.txt", fmt.Sprintf("v%d\n", i))
+	mustGit(t, env, work, "add", "-A")
+	mustGit(t, env, work, "commit", "-qm", fmt.Sprintf("commit %d", i))
+	}
+	if err := os.MkdirAll(filepath.Dir(bare), 0o755); err != nil {
+	t.Fatal(err)
+	}
+	mustGit(t, env, work, "clone", "-q", "--bare", work, bare)
+	if err := os.WriteFile(filepath.Join(bare, "Access"), []byte("read,list: all\n*: "+owner+"\n"), 0o644); err != nil {
+	t.Fatal(err)
+	}
+
+	srv, err := New(Config{
+	Store: store.NewLocalStore(ns),
+	Web: http.FS(os.DirFS("../../web")),
+	CacheDir: t.TempDir(),
+	})
+	if err != nil {
+	t.Fatal(err)
+	}
+	ts := httptest.NewServer(srv.Handler())
+	defer ts.Close()
+
+	// The client clones the full history, so it has every commit to offer.
+	dst := filepath.Join(t.TempDir(), "client")
+	mustGit(t, env, ".", "clone", "-q", cloneURL(ts.URL, repoPath, "", ""), dst)
+
+	// One more commit lands in the bare repo, out of band.
+	writeTestFile(t, work, "f.txt", "tip\n")
+	mustGit(t, env, work, "add", "-A")
+	mustGit(t, env, work, "commit", "-qm", "tip")
+	mustGit(t, env, work, "push", "-q", bare, "main")
+	wantTip := strings.TrimSpace(mustGit(t, env, work, "rev-parse", "main"))
+
+	out, err := runGit(t, env, dst, "fetch", "origin")
+	if err != nil {
+	t.Fatalf("incremental fetch failed: %v\n%s", err, out)
+	}
+	if got := strings.TrimSpace(mustGit(t, env, dst, "rev-parse", "origin/main")); got != wantTip {
+	t.Errorf("origin/main = %s, want %s", got, wantTip)
+	}
+	}
+
+	func writeTestFile(t *testing.T, dir, name, content string) {
+	t.Helper()
+	if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil {
+	t.Fatal(err)
+	}
+	}