Expand conformance tests for SHA-256#61
Open
pjbgf wants to merge 24 commits into
Open
Conversation
Mirrors upstream Git's -C semantics: each -C chdirs relative to the previous cwd, missing argument exits 129, chdir failure exits 128 with "fatal: cannot change to '<dir>': <msg>". Handled via argv-peeking before cobra.Execute so it works with every subcommand. Required by upcoming sha256 conformance graduation, which verifies init outcomes via "git -C <dir> rev-parse --show-object-format". Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 813bd3dc97d9
go-git supports sha1 and sha256 at runtime, so there is no compile-time default. The conformance harness reads `default-hash` from this output before exporting GIT_DEFAULT_HASH; echoing the test-driven value when GIT_TEST_DEFAULT_HASH or GIT_DEFAULT_HASH is set keeps the DEFAULT_HASH_ALGORITHM prereq lit in both sha1 and sha256 passes. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 5cac3975981d
Resolution order matches upstream: --object-format wins, then GIT_DEFAULT_HASH, then sha1. Unknown algorithm names fail with `unknown hash algorithm "X"`. The resolved format is passed to go-git's PlainInit via WithObjectFormat. Required by the upcoming t0001-init.sh sha256 cases (52, 53, 56, 60), which init repos using both the flag and the env var. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: cc90fb9ba1b8
Prints the repository's object hash algorithm. Absent extensions.objectformat (sha1 repos) is reported as "sha1" instead of the empty UnsetObjectFormat string. Args relaxed to cobra.ArbitraryArgs so --show-object-format can be used without a rev argument; an explicit error replaces cobra's "requires at least 1 arg" when neither a rev nor a --show-* flag is provided. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 89dcbf41263d
--hash=<sha1|sha256> filters curated tests.txt entries by their @modes tag (entries without @ default to sha1-only) and exports GIT_TEST_DEFAULT_HASH per test invocation. The flag is stripped from positional args before the single-test path, so existing `./conformance/run.sh t0001-init.sh 53` calls keep working. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: c67539cc1120
`make conformance` now invokes run.sh twice. Either pass failing fails the target. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 9dcfab90d5a0
Adds five t0001-init.sh cases that exercise the new gogit init --object-format flag, GIT_DEFAULT_HASH env, rev-parse --show-object-format, and extensions.objectFormat validation. They run in both sha1 and sha256 modes via the @sha1,sha256 mode tag. Graduated cases (numbered post-for-loop-expansion): 52: init honors GIT_DEFAULT_HASH 53: init honors --object-format 56: --object-format overrides GIT_DEFAULT_HASH 60: extensions.objectFormat is not allowed with repo version 0 61: init rejects attempts to initialize with different hash Header in tests.txt documents the `@modes` token syntax for future entries. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: fbb2e4556116
Required by t1007-hash-object.sh, which calls `git init --quiet` in its push_repo helper outside any test_expect_success — meaning the helper runs at script-parse time and a missing --quiet flag breaks every subsequent case. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 26245ba9c2b7
Graduates 16 cases from t1007 that exercise gogit's core hash-object surface: hashing a file, --stdin, -w (write-to-store), bogus/truncated type-name rejection, --literally, and --stdin outside a repo. Excluded cases need flags gogit doesn't have (--stdin-paths, --path, --no-filters), crlf filter behaviour, or stricter malformed-object validation than gogit currently does. The upstream test bakes sha256 OIDs into its test_oid cache, so this entry exercises sha256 hashing identity end-to-end in the sha256 pass. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 566cae5bb242
Reads the on-disk index and emits one entry per matching pathspec. Supports the minimum surface upstream tests rely on: ls-files — list tracked paths ls-files <path|dir> — filter by literal pathspec ls-files -s / --stage — `<mode> <oid> <stage>\t<path>` ls-files --error-unmatch SPEC — exit non-zero on no match Pathspecs are matched literally (no glob expansion); a trailing slash on a directory spec is ignored. The --stage hash field naturally widens to 32 bytes in sha256 repos, so the same command exercises both formats end-to-end. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 4b1debc131f6
Graduates 12 cases from t3700 covering add's main interactions plus
index round-trip via ls-files. Case 3 in particular ("Post-check that
foo is in the index") exercises the full add → index-write →
index-read chain, which under sha256 mode verifies that index entries
carry 32-byte hashes end-to-end.
Excluded cases need gogit surface that isn't there yet: --chmod,
--refresh, --ignore-errors, core.fsyncmethod=batch, update-index --add,
unmerged-entry stages, and embedded-repo detection.
Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Entire-Checkpoint: 693f4caa6d61
12 self-contained cases from t7501-commit-basic-functionality.sh that
cover argument validation and error paths around `git commit`:
2 fail initial amend
6 paths and -a do not mix
9 using invalid commit with -C
11 --dry-run fails with nothing to commit
12 --short fails with nothing to commit
13 --porcelain fails with nothing to commit
14 --long fails with nothing to commit
26 commit message from non-existing file
27 empty commit message
34 set up editor
41 -m and -F do not mix
57 commit complains about completely bogus dates
Case 3 (the only case that successfully creates a commit) depends on
case 1 ("initial status") having added a file. Case 1 fails because
gogit's `status` output differs from upstream, breaking the dependency
for case 3 under selector mode. End-to-end init/add/commit round-trip
in sha256 is covered by go-git's own test suite, not by gogit
conformance.
Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Entire-Checkpoint: d86017ed622e
Two upstream-parity fixes to `gogit clone`:
* Bare local paths (`gogit clone src dst`, `gogit clone ./src dst`)
are resolved to absolute filesystem paths before being handed to
go-git's PlainClone, which only accepts schemed URLs or absolute
paths today. Explicit schemes (file://, http(s)://, ssh://, git://)
and scp-like host:path refs pass through unchanged.
* The target directory is checked before the clone runs. If it
already exists as a non-empty directory or as a non-directory file,
gogit fails with upstream's "destination path 'X' already exists
and is not an empty directory" message. Without this check go-git
happily merged the cloned worktree into whatever was there.
Required by t5601-clone.sh case 7 ("clone checks out files"), which
runs `git clone src dst` with a relative path, and by cases 22/23
which assert non-empty / non-directory targets are rejected.
Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Entire-Checkpoint: 2d4f5ce35af9
Graduates 8 cases from t5601-clone.sh covering local clone transport
in both sha1 and sha256 modes. Case 7 ("clone checks out files") is
the real transport probe — case 1's setup builds a source repo in
whichever hash mode is active, case 7 clones it via a local path and
verifies the file is checked out.
Excluded: ssh-wrapper cases (70-72 need the wrapper setup at line 337);
richer clone surface (--bare, --mirror, --separate-git-dir, hooks,
partial clone) gogit doesn't have; specific ref-format / shallow /
mirror tests; the explicit sha1<->sha256 cross-clone case which needs
compat-object-format we explicitly excluded.
Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Entire-Checkpoint: da112df47cb7
Upstream's HTTP conformance tests (t5551, t5561) are locked to Apache + git-http-backend via lib-httpd.sh and can't drive our standalone gogit-http-server. This adds an in-house equivalent under conformance/local/: a runner builds gogit + gogit-http-server, picks a free port, and runs each test script against them. The first test (http-clone.sh) builds a source repo in sha1 then sha256, serves it via gogit-http-server, clones over http://, and asserts the clone's object format and HEAD match the source. Confirms that go-git's existing object-format capability negotiation is end-to-end intact through gogit-http-server — no wiring in the server itself was needed. Wired into `make conformance` so the local pass runs after the two upstream-curated passes. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 61a462655c4a
Mirrors http-clone.sh in shape: seed a bare destination via clone --bare, serve it through gogit-http-server, then clone, commit, push from a client, and re-clone to confirm the server now holds the pushed commit. The push URL has dummy credentials baked in (test:test) because go-git's backend rejects receive-pack requests without an Authorization header as a sanity check — it doesn't validate the credentials. Cloning with the credential-bearing URL puts them in the cloned repo's origin config, so the subsequent `push origin` inherits the header automatically. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: e8c69b1a63c4
Thin wrapper over go-git's Repository.Merge with FastForwardMerge strategy. After the ref update the worktree is checked out at the new HEAD so the user sees the merged content; bare repos skip the worktree step. Non-FF merge is rejected (with a hint to use --ff-only). go-git doesn't implement other merge strategies, so for now --ff-only and plain `merge` behave the same — the flag is the upstream-compatible spelling for users who want to opt into FF-only explicitly. Also adds a local conformance test (conformance/local/ff-merge.sh) that exercises A -> B fast-forward in both sha1 and sha256. Branch setup uses direct ref writes because gogit doesn't have `branch` / `switch` / `update-ref` yet; those are orthogonal gaps. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 39622a34d2c8
Wraps go-git's Worktree.CherryPick. Resolves each positional argument
to a commit object (falling back to FromHex when go-git's
ResolveRevision rejects a full sha256 hex string — its full-hash
branch is gated on sha1 width), then runs the cherry-pick with the
chosen merge strategy.
--strategy-option {theirs,ours} maps onto go-git's
OrtMergeStrategyOption. theirs is the default (incoming changes win).
go-git's CherryPick is structurally a single-strategy merge rather
than an upstream-style cherry-pick of a single commit's diff against
its parent, so cherry-pick onto a different parent is best treated as
"merge B into current, picking theirs" and may pull in more than the
upstream version would.
Also adds a local conformance test (conformance/local/cherry-pick.sh)
that exercises the well-supported subset: A -> B introduces a new
file, rewind to A, cherry-pick B, verify the resulting commit, index,
and worktree.
Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Entire-Checkpoint: 1a49384c05b3
Wraps go-git's Worktree.Submodules() for status/init/update and
implements `submodule add` directly: clone the repository into the
chosen path, write the .gitmodules entry, then stage a gitlink entry
(mode 160000, hash = clone HEAD) in the parent's index by writing the
index Entry directly — go-git's Worktree.Add doesn't gitlink-stage.
Behaviour verified manually for both sha1 and sha256 repositories. Add
in sha256 parent stages a 32-byte gitlink hash; subsequent init/update
in a fresh clone of the parent fetches and checks out the submodule.
Known gaps that this commit doesn't close:
* Submodule data lives inline at <path>/.git, not at
.git/modules/<path>/, so the upstream gitfile indirection isn't
present. Tests that check .git/modules/<name>/ exist (e.g. t1600
case 6) won't graduate without that.
* No `submodule deinit`, `sync`, `summary`, `foreach`.
Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Entire-Checkpoint: 0b7494ea3d73
`gogit -c <key>=<value> <cmd>` used to populate an in-memory map only. Commands that consult that map directly (via hasConfigOverride / configBool) saw the override, but go-git's filesystem storage reads .git/config eagerly at PlainOpen-time and never saw it — so storage- governed config like index.skipHash was effectively unreachable from the CLI. Fix: in applyConfigOverridesFromFlags, after populating the map, also patch each override into .git/config so storage construction picks them up. Snapshot the original contents and restore on exit (success or error) so the file is unchanged from the user's perspective. The in-memory map still exists for callers that depend on it. If the process is killed mid-command (e.g. SIGKILL) the on-disk .git/config will be left with the overrides persisted. Acceptable tradeoff for the simplicity; documented inline. Verified: `gogit -c index.skipHash=true add f` now produces an index with a zero trailer; the parent's .git/config is unchanged after the command. Errors during the patched command still restore the original config. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 4f69273d3704
go-git's PlainCloneContext already invokes checkTargetDirIsEmpty (repository.go:582 → checkTargetDirIsEmpty) before initialising a clone, returning ErrTargetDirNotEmpty for any non-empty target and "path is not a directory" for non-directory targets. Our wrapper was re-implementing the same guard. Removing the duplicate also drops the upstream-shaped error string we were emitting; go-git's "destination path already exists and is not empty <path>" replaces it. The t5601 cases that check this path (22, 23) only assert exit codes, so the message change is behaviour-equivalent for our graduation. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: dc91b7137048
The `Add` operation is upstreamable as Worktree.AddSubmodule(url,
path) in go-git itself: clone the URL, write .gitmodules, stage a
gitlink in the parent's index. Moving it under internal/submodule/
makes the upstreaming target legible at a glance, in keeping with how
internal/plumbing/{format,object} mirror their eventual go-git homes.
cmd/gogit/submodule.go shrinks to the cobra wiring plus argument
parsing (path defaulting, URL resolution). internal/submodule.Add
takes a *git.Repository, the URL, and a path, and returns the cloned
submodule's repository handle.
No behaviour change — submodule add smoke still produces the same
.gitmodules entry, gitlink index entry, and parent worktree state in
both sha1 and sha256 modes.
Assisted-by: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>
Entire-Checkpoint: f3551598388e
go-git's checkTargetDirIsEmpty uses osfs.Default, which is bound to "/" — relative paths look up against the wrong root and silently report "empty", so the upstream guard never fires for a non-empty target like `./target-4`. Resolving the destination via filepath.Abs before calling PlainClone makes the guard work as documented. Verified: `gogit clone src target-4` where target-4 contains a file now fails with "destination path already exists and is not empty" and exit 1, restoring the t5601 case 22 / 23 behaviour we lost when we removed our duplicate wrapper. Empty and nonexistent relative targets still clone successfully. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 37e51c089d70
Three of the five originally sha1-only entries pass cleanly under sha256 too; tag them @sha1,sha256 so each runs in both modes: t2008-checkout-subdir.sh — basic checkout, hash-agnostic t5308-pack-detect-duplicates.sh — pack dedup, no oid pinning t1600-index.sh 1-5,7 — index v2/v3/v4 round-trip works at 32-byte width Two stay sha1-only with inline reasoning: t5325-reverse-index.sh — sha256 pack writer / reader trailer checksum mismatch (real gogit/go-git bug to file separately). t1601-index-bogus.sh — premise is "null sha1 in tree" / "read-tree refuses null sha1" — doesn't translate to sha256 since the same construction yields a non-null sha256 hash. Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io> Entire-Checkpoint: 2710bcc4ee2f
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands gogit’s SHA-256 conformance coverage and adds CLI surface needed by the new tests.
Changes:
- Runs curated upstream conformance tests in both SHA-1 and SHA-256 modes.
- Adds local conformance tests for HTTP clone/push, fast-forward merge, and cherry-pick.
- Adds or extends gogit commands/features such as init object format, rev-parse object format, ls-files, merge, cherry-pick, submodule, clone path handling, and config override persistence.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Runs SHA-1, SHA-256, and local conformance suites. |
| internal/submodule/add.go | Adds internal submodule add/gitlink staging implementation. |
| conformance/tests.txt | Expands curated upstream tests and hash-mode selectors. |
| conformance/run.sh | Adds hash-mode parsing and per-mode test selection. |
| conformance/local/run.sh | Adds local conformance test runner. |
| conformance/local/http-push.sh | Adds HTTP push local conformance test. |
| conformance/local/http-clone.sh | Adds HTTP clone local conformance test. |
| conformance/local/ff-merge.sh | Adds fast-forward merge local conformance test. |
| conformance/local/cherry-pick.sh | Adds cherry-pick local conformance test. |
| cmd/gogit/version.go | Makes build-options default hash reflect test/env hash mode. |
| cmd/gogit/submodule.go | Adds submodule status/init/update/add commands. |
| cmd/gogit/rev-parse.go | Adds --show-object-format. |
| cmd/gogit/merge.go | Adds fast-forward merge command. |
| cmd/gogit/main.go | Adds global -C handling and config override restoration. |
| cmd/gogit/ls-files.go | Adds basic index listing command. |
| cmd/gogit/init.go | Adds object format and quiet init support. |
| cmd/gogit/config.go | Persists temporary config overrides into .git/config. |
| cmd/gogit/clone.go | Resolves local clone URLs and absolute destinations. |
| cmd/gogit/cherry-pick.go | Adds cherry-pick command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return fmt.Errorf("open worktree: %w", err) | ||
| } | ||
|
|
||
| return w.Checkout(&git.CheckoutOptions{Branch: head.Name(), Force: true}) |
Comment on lines
+87
to
+100
| if arg == "-C" { | ||
| if i+1 >= len(args) { | ||
| fmt.Fprintln(os.Stderr, "fatal: option `-C' requires a value") | ||
| os.Exit(129) | ||
| } | ||
|
|
||
| if err := os.Chdir(args[i+1]); err != nil { | ||
| fmt.Fprintf(os.Stderr, "fatal: cannot change to '%s': %s\n", args[i+1], chdirErrMsg(err)) | ||
| os.Exit(128) | ||
| } | ||
|
|
||
| i++ | ||
|
|
||
| continue |
Comment on lines
+133
to
+137
| if configBackupCreated { | ||
| _ = os.Remove(configBackupPath) | ||
| } else { | ||
| _ = os.WriteFile(configBackupPath, configBackup, 0o644) | ||
| } |
Comment on lines
108
to
+114
| err := rootCmd.Execute() | ||
|
|
||
| // Restore any .git/config we patched on this command's behalf via `-c` | ||
| // overrides. Runs on both success and error paths so the on-disk file | ||
| // is back to its starting contents before the process exits. A SIGKILL | ||
| // or similar abrupt termination would skip this — accepted tradeoff. | ||
| restoreConfigBackup() |
Comment on lines
+128
to
+130
| cloneURL := resolveCloneURL(args[0]) | ||
|
|
||
| if _, err := internalsubmodule.Add(parent, cloneURL, relPath); err != nil { |
| } | ||
|
|
||
| clean := filepath.ToSlash(filepath.Clean(path)) | ||
| if filepath.IsAbs(clean) || strings.HasPrefix(clean, "../") || clean == ".." { |
Comment on lines
+82
to
+86
| func persistConfigOverridesToGitDir() error { | ||
| gitDir, err := findGitDir() | ||
| if err != nil { | ||
| return nil //nolint:nilerr // not in a repo: persistence is a no-op | ||
| } |
Comment on lines
+45
to
+49
|
|
||
| return nil | ||
| } | ||
|
|
||
| if len(args) == 0 { |
Comment on lines
+28
to
+33
| echo "Building gogit..." | ||
| ( cd "$REPO_ROOT" && go build -o "$BIN_DIR/gogit" ./cmd/gogit ) | ||
|
|
||
| echo "Building gogit-http-server..." | ||
| ( cd "$REPO_ROOT" && go build -o "$BIN_DIR/gogit-http-server" ./cmd/gogit-http-server ) | ||
|
|
| // upstream: the submodule lives at `<path>/.git`, not under | ||
| // `.git/modules/<path>/` with a gitfile pointer — implementing that | ||
| // indirection is a follow-up that should land alongside upstreaming. | ||
| func Add(repo *git.Repository, url, path string) (*git.Repository, error) { |
| // absolute) are pointed at the directory they name on disk; scp-like refs | ||
| // (host:path) and explicit schemes (file://, https://, ssh://, git://) pass | ||
| // through unchanged. | ||
| func resolveCloneURL(arg string) string { |
Member
There was a problem hiding this comment.
this feels like reimplementation of transport.ParseURL?
Comment on lines
11
to
+12
| "github.com/go-git/go-git/v6/config" | ||
| formatcfg "github.com/go-git/go-git/v6/plumbing/format/config" |
Member
There was a problem hiding this comment.
I would make this an interface over formatcfg and minimize the usage of config
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.