can you look at the comments on https://github.com/entireio/git-sync/pull/66 and address them? · Entire

PR #66 Overview

Pushed

Commit e7e4a1a is on nodo/convert-sha256.

Comments Addressed

#	Reviewer	Comment	Resolution
1	cursor[bot]	Discovery ignores ctx cancel	Already fixed — `discoverReachable` rewrite (iterative DFS checking `ctx.Err()` per object) predates this; bot reviewed an old commit. Reply posted.
2	pjbgf	objfile no longer hardcodes SHA1 in alpha.4	Refactored to `dst.NewEncodedObject` + `SetEncodedObject`; removed `encodeBody`/`writeLoose`. Doc note updated.
3	pjbgf	prefer `--sign-mode` over `--sign`	Renamed `--sign` → `--sign-mode {none,tips}` (default `none`), validated in `Run`, room for a future `all`.
4	pjbgf	concerns signing tags in place?	Explained the premise (branch tips aren't signable objects → wrapped in `converted/<branch>` tags; in-place tag signing is the future `all` mode). Reply posted.

Verification

go build ./..., go test ./... — clean
Gated TestRun_GitHTTPBackend + TestRun_GitHTTPBackend_Sign (GITSYNC_E2E_SHA256_HTTP_BACKEND=1) — pass; full conversion via SetEncodedObject produces an fsck-clean SHA256 repo, and --sign-mode tips signs as before
go test -race — clean
golangci-lint — 0 issues
The on-disk sha256(content) invariant test still passes against the new write path, kept as a regression guard.

One judgment call worth flagging: the refactor changes loose-object compression from zlib.BestSpeed (level 1) to go-git's default (level 6), so very large conversions will spend somewhat more CPU compressing — the trade-off you opted into by choosing the refactor. If that ever bites on kernel-scale repos, git gc on the output packs it away regardless.

Additional Comments

I think we could be leaking credentials here:
    msg := fmt.Sprintf(
        "SHA1 → SHA256 conversion attestation for %s.\n\n"+
        "Source: %s\nProduced by git-sync convert-sha256.\n",
        refName, sourceURL)

Credential Leak — Fixed

The source URL can carry user:token@host, and it was being copied verbatim into three surfaces, the worst being the signed attestation tag message (permanent and pushed). Also the fetching … status line and the SourceURL field in both the text and JSON result.

Fix: a redactSourceURL helper strips the entire userinfo component before any of those surfaces. I deliberately did not use Go's url.URL.Redacted() — it only masks the password, so a token-in-username form like https://ghp_xxx@github.com/... (common for GitHub token auth) would survive untouched. The fetch path keeps the original req.SourceURL, so auth is unaffected.

Covered by a new TestRedactSourceURL (plain creds, token-as-username, x-access-token, and the unparseable-input → placeholder path). Full suite + gated E2E/signing tests + lint + gofmt all green.

Foreign Pull/Merge-Request Refs Handling

All code in a pull ref is foreign to the repository before a merge. Bringing that in and pushing to a different forge which likely has different semantics around refs/pull can be used to deliver malicious code from the repo target.

Change Complete and Clean

The change is complete and clean. Let me commit and push:

refs/pull/* (GitHub/Gitea), refs/pull-requests/* (Bitbucket), and refs/merge-requests/* (GitLab) hold code proposed from forks and other branches — foreign to the repository until merged. --all-refs pulled them in, and since the converted repo is typically mirrored onward with git push --mirror, a destination forge that doesn't treat those namespaces as read-only PR refs could republish unreviewed code as repo content.

Exclude those namespaces from --all-refs by default; --include-pull-refs opts back in (e.g. for a faithful archival mirror). The run reports how many such refs it dropped (status line + Result.SkippedPullRefs) so an --all-refs conversion never silently omits them.

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Entire-Checkpoint: 40a74bd2030d+238/-15