dumb HTTP transport: fetch fails when object already present locally

While auditing plumbing/transport/http we found a set of related correctness bugs in the dumb-protocol path that cause fetch/clone to fail outright on inputs that should be valid. Filing these together since they share a root cause: the dumb-protocol path hasn't received the same hardening as the smart-HTTP path in this package.

1. fetchObject returns a zero-value object when it's already present, so the whole fetch aborts

plumbing/transport/http/dumb.go, fetchObject (~line 252):

func (r *fetchWalker) fetchObject(objHash plumbing.Hash, obj plumbing.EncodedObject) (err error) {
	if r.st.HasEncodedObject(objHash) == nil {
		return nil
	}
	...

HasEncodedObject returns nil when the object already exists in local storage. In that case this function returns nil without ever populating obj (no SetType/SetSize/Writer()), so obj.Type() stays plumbing.InvalidObject.

The caller, fetch() (~line 309), doesn't special-case this — it unconditionally does:

switch obj.Type() {
case plumbing.CommitObject: ...
case plumbing.TreeObject: ...
case plumbing.TagObject: ...
case plumbing.BlobObject: ...
default:
    return plumbing.ErrInvalidType   // ~line 407
}

Since obj.Type() is InvalidObject, this always hits default and the entire Fetch aborts with ErrInvalidType.

This is reachable in practice: process() (~line 241) unconditionally enqueues head with no HasEncodedObject gate — unlike the ref loop right below it, which does gate on HasEncodedObject(ref.Hash()) != nil. So any second dumb-HTTP fetch against a repo already cloned this way — including a no-op fetch, or an incremental fetch that shares history with what's already stored — hits this path for head (or any shared ancestor reached during the walk) and fails.

This matches the fact that dumbUploadPackSuite in dumb_test.go (~line 97) explicitly overrides TestUploadPackNoChanges, TestUploadPackPartial, TestUploadPackFull, TestUploadPackMulti, TestUploadPack, TestUploadPackInvalidReq with empty bodies — so this regression path is never exercised by the shared conformance suite.

Suggested fix: fetchObject should signal "already present, skip" distinctly (sentinel error or bool) so fetch() can continue without decoding an empty obj. process() should also gate head on HasEncodedObject, consistent with how other refs are already treated.

2. Inverted existence check means pack indexes/files are always re-downloaded

plumbing/transport/http/dumb.go:231 and dumb.go:359:

if _, err := r.fs.Stat(packIdx); errors.Is(err, fs.ErrExist) {

Stat returns nil (not fs.ErrExist) when the file exists, and an fs.ErrNotExist-wrapping error when it doesn't — it never returns fs.ErrExist. So this condition is always false, and the "already have it" branch is dead code: every fetch re-downloads every pack index and every full pack file over HTTP even when already cached on disk.

Suggested fix: should be err == nil (file exists) rather than errors.Is(err, fs.ErrExist).

3. A 404 on objects/info/packs is treated as "repository not found"

plumbing/transport/http/dumb.go:108 (getInfoPacks): a non-2xx status is propagated via checkError, which maps 404 to transport.ErrRepositoryNotFound. But a repo with no packs (all objects loose) legitimately 404s here. checkError's own doc comment states "404 is ordinary control flow for the dumb walk," and fetchObject treats a 404 on a loose object the same way — but getInfoPacks doesn't get the same treatment, so a valid pack-less repository fails to fetch/clone entirely.

Suggested fix: getInfoPacks should treat 404 as "no packs" (empty list) rather than propagating ErrRepositoryNotFound.

Happy to send a PR for any/all of these if useful.

Disclosure: these findings came from an AI-assisted code review (Claude) of this package. The core claims (the zero-value obj propagation, the inverted errors.Is(err, fs.ErrExist) check, and the 404-on-info/packs handling) were manually verified against the current source before filing. Happy to provide more detail or a reproduction if useful.