Fix Cancellation Masked by AlreadyUpToDate

I read issue #141 and the code it points at. Here's my assessment.

The bug is real but narrow, and it isn't a security issue.

• git.NoErrAlreadyUpToDate is only returned when there are no wants: fetchToStoreV2 and buildV1UploadPackBody exit early (internal/gitproto/fetch.go:192, :457) before any network I/O. So the joined error only appears if the context was already canceled when a no-op fetch ran.
• When that happens, errors.Join(ctx.Err(), NoErrAlreadyUpToDate) matches the sentinel, so every caller treats it as success. There are four callers, not the two the issue names:
  • internal/syncer/syncer.go:1081 (sync/replicate closure fetch). The cancel is probably caught by the next ctx-aware step (push).
  • internal/syncer/syncer.go:1562 (fetch-only). Here a canceled run counts objects and reports success. This is the worst case.
  • internal/strategy/materialized/materialized.go:86 (tag refill).
  • cmd/git-sync/internal/sha256convert/sha256convert.go:357.
• The bootstrap FetchPack paths (branch_group.go:242, bootstrap.go:1126) aren't affected, because FetchPack doesn't do the join.

Recommended fix: change FetchToStore itself, not each caller as the issue suggests. The join exists so that a parse or network error caused by cancellation reports the cancel first. "Already up to date" is never caused by cancellation, so the join shouldn't wrap it. Something like:

That fixes all four callers at once, so they can't drift apart again. A regression test in internal/gitproto/fetch_test.go would call FetchToStore with a pre-canceled ctx and empty desired refs (v1 and v2), then assert errors.Is(err, context.Canceled) and !errors.Is(err, git.NoErrAlreadyUpToDate).

Want me to put that on a branch with the test and open a PR that closes #141? I won't comment on the issue or open anything on GitHub until you say so.

I fixed issue #141 on a new local branch, soph/fetch-cancel-masks-uptodate, in one commit (dab4b828). I haven't pushed it or opened a PR.

The fix is in internal/gitproto/fetch.go. When the context is canceled and the fetch result is NoErrAlreadyUpToDate, FetchToStore now returns the cancellation alone, wrapped as fetch canceled: context canceled. Callers therefore no longer treat it as success. All other errors are still joined with the cancellation as before. Because the change is inside FetchToStore, it covers all four callers (the two syncer paths, materialized tag refill and sha256convert) without editing them.

Tests are in internal/gitproto/fetch_test.go, each covering v1 and v2:

• TestFetchToStoreCanceledNoWantsReportsCancel: with an already-canceled context and nothing to fetch, the error must match context.Canceled and must not match NoErrAlreadyUpToDate. It failed before the fix and passes now.
• TestFetchToStoreNoWantsUpToDate: with a live context, a fetch with nothing to fetch still returns the up-to-date error as before.

The full go test ./... and golangci-lint both pass.

The commit message includes Fixes #141, so the issue will close when this merges. Should I push and open the PR?

I opened the PR: https://github.com/entireio/git-sync/pull/142

It targets main and says "Fixes #141", so the issue will close when it merges. The description explains the bug and the fix and lists the two new tests. I didn't comment on the issue itself.

They're not all necessary, but one line in fetch.go is worth keeping.

In fetch.go: the 4-line comment explains two things:

1. Callers treat NoErrAlreadyUpToDate as success, and errors.Is still finds it inside a join. This is the part worth keeping. Without it, the special case looks redundant next to the join right below it, and someone could "simplify" it away and bring the bug back.
2. The sentinel is never a side effect of cancellation. This adds little, because the comment just above already says the join exists to put cancellation first.

I'd cut it to:

In the test: the doc comment on TestFetchToStoreCanceledNoWantsReportsCancel just repeats the test name and the assertions, so I'd drop it.

Want me to make both changes and push them to #142?