From 612f9987f576b0dee6c531bf4aa9f994f97ed3de Mon Sep 17 00:00:00 2001 From: closer-claudio Date: Thu, 22 Oct 2026 22:14:28 -0700 Subject: [PATCH] Count subject characters, and refuse a range git cannot resolve Two decisions on the command's contract. The short-subject rule counted bytes against its limits of 90 and 72, so a subject of 85 accented characters failed as 170 bytes, where GitHub's ellipsis and a reader count characters. The rule counts runes. A --range git could not resolve was silently replaced by the whole history the clone has, from its oldest commit: the fallback exists for a shallow CI clone whose depth does not reach the range's start, and it swallowed a typo and a missing ref with it, validating everything and passing. The fallback now applies to a shallow clone only (git rev-parse --is-shallow-repository); in a repository with its whole history the range is an error that names it and the git command that failed, unwrapping to git's exit as before, and nothing is validated. The contract cases for both land with the decision: an 89-character non-ASCII subject passes and a 90-character one fails; an unresolvable range exits non-zero with the range on stderr and no commit validated; a shallow clone with a range past its depth still runs over what it has. The two pins of the old behaviour move with it: the range test's bad range is refused, and the outside-a-repository error names git rev-list, the command that fails first there. Co-Authored-By: Claude Fable 5.1 Signed-off-by: closer-claudio --- cmd/git-validation/contract_decided_test.go | 100 ++++++++++++++++++++ git/commits.go | 32 ++++++- git/commits_test.go | 2 +- git/contract_test.go | 26 +++-- rules/shortsubject/shortsubject.go | 9 +- 5 files changed, 157 insertions(+), 12 deletions(-) create mode 100644 cmd/git-validation/contract_decided_test.go diff --git a/cmd/git-validation/contract_decided_test.go b/cmd/git-validation/contract_decided_test.go new file mode 100644 index 0000000..e9f0f8b --- /dev/null +++ b/cmd/git-validation/contract_decided_test.go @@ -0,0 +1,100 @@ +package main_test + +// Two decisions on the command's contract, each a case that fails on the +// behaviour before them: the short-subject rule counts characters, not +// bytes, so a subject of 89 non-ASCII characters passes and one of 90 +// fails; and a --range git cannot resolve is an error naming the range, +// with nothing validated, where a shallow clone whose history does not +// reach the range's start still runs over what it has. + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +//nolint:paralleltest // newRepo sets the environment with t.Setenv +func TestContractShortSubjectCountsRunes(t *testing.T) { + r := newRepo(t) + + r.commit("a", "a\n", "Root", true) + + under := strings.Repeat("é", 89) + r.commit("b", "b\n", under, true) + + if code, out := r.run( + nil, + "--run", + "short-subject", + ); code != 0 || !strings.Contains(out, "PASS") || + strings.Contains(out, "FAIL") { + t.Errorf("89 characters of two bytes each: exit %d, want the rule to pass\n%s", code, out) + } + + over := strings.Repeat("é", 90) + r.commit("c", "c\n", over, true) + + if code, out := r.run(nil, "--run", "short-subject"); code == 0 || !strings.Contains(out, "FAIL") { + t.Errorf("90 characters: exit %d, want the rule to fail\n%s", code, out) + } +} + +//nolint:paralleltest // newRepo sets the environment with t.Setenv +func TestContractUnresolvableRangeIsAnError(t *testing.T) { + r := newRepo(t) + + r.commit("a", "a\n", "Root", true) + r.commit("b", "b\n", "Change", true) + + cmd := exec.CommandContext(t.Context(), binary, "--range", "nosuchref..HEAD") + cmd.Dir = r.dir + cmd.Env = os.Environ() + + var stderr strings.Builder + + cmd.Stderr = &stderr + + out, err := cmd.Output() + if err == nil { + t.Fatalf("an unresolvable range validated the history:\n%s", out) + } + + if strings.Contains(string(out), " * ") { + t.Errorf("commits were validated under an unresolvable range:\n%s", out) + } + + if !strings.Contains(stderr.String(), "nosuchref") { + t.Errorf("the error does not name the range:\n%s", stderr.String()) + } +} + +// A shallow clone's history may not reach the range's start a CI +// configuration names; the run then covers what the clone has, as before. +// +//nolint:paralleltest // newRepo sets the environment with t.Setenv +func TestContractShallowCloneRunsOverWhatItHas(t *testing.T) { + r := newRepo(t) + + r.commit("a", "a\n", "Root", true) + base := r.git("rev-parse", "HEAD") + r.commit("b", "b\n", "Second", true) + r.commit("c", "c\n", "Third", true) + + shallow := filepath.Join(t.TempDir(), "shallow") + + clone := exec.CommandContext(t.Context(), "git", "clone", "-q", "--depth", "1", "file://"+r.dir, shallow) + if out, err := clone.CombinedOutput(); err != nil { + t.Fatalf("shallow clone: %v\n%s", err, out) + } + + cmd := exec.CommandContext(t.Context(), binary, "--dir", shallow, "--range", base+"..HEAD") + cmd.Dir = shallow + cmd.Env = os.Environ() + + out, err := cmd.Output() + if err != nil || !strings.Contains(string(out), "PASS") { + t.Errorf("shallow clone: %v\n%s", err, out) + } +} diff --git a/git/commits.go b/git/commits.go index 8c9eaec..b40ad8f 100644 --- a/git/commits.go +++ b/git/commits.go @@ -48,8 +48,16 @@ func Commits(commitrange string) ([]CommitEntry, error) { return commits, nil } -// Since the commitrange requested may be longer than the depth being cloned in CI, -// check for an error, if so do a git log to get the oldest available commit for a reduced range. +// errRange is a --range git cannot resolve in a repository whose history is +// whole: a typo, or a ref that is not there. It names the range. +var errRange = errors.New("the commit range does not resolve") + +// checkRevList returns the range to validate. A range git resolves is +// itself. One it does not resolve is an error in a repository with its +// whole history; in a shallow clone, the CI case, the range's start may lie +// past the depth fetched, and the run then covers what the clone has, from +// its oldest commit. Before, every unresolvable range took that fallback, +// so a typo validated the whole history and passed. func checkRevList(commitrange string) (string, error) { cmdArgs := []string{"git", "rev-list", commitrange} @@ -59,6 +67,18 @@ func checkRevList(commitrange string) (string, error) { return commitrange, nil } + if !shallowRepository() { + // The git command that failed is named, and its exit unwraps, as + // every git failure here does; git's own words follow. + detail := "" + + if exitErr, ok := errors.AsType[*exec.ExitError](err); ok && len(exitErr.Stderr) > 0 { + detail = " (" + strings.TrimSpace(string(exitErr.Stderr)) + ")" + } + + return "", fmt.Errorf("%w: %s: %w%s", errRange, strings.Join(cmdArgs, " "), err, detail) + } + cmdArgs = []string{"git", "log", "--pretty=oneline", "--no-show-signature"} output, err := command(cmdArgs[1:]...).Output() @@ -78,6 +98,14 @@ func checkRevList(commitrange string) (string, error) { return lastCommit + "..HEAD", nil } +// shallowRepository reports whether the repository was cloned with a depth, +// so that its history may not reach a range's start. +func shallowRepository() bool { + output, err := command("rev-parse", "--is-shallow-repository").Output() + + return err == nil && strings.TrimSpace(string(output)) == "true" +} + // FieldNames are for the formating and rendering of the CommitEntry structs. // Keys here are from git log pretty format "format:...". var FieldNames = map[string]string{ diff --git a/git/commits_test.go b/git/commits_test.go index 6576ed0..087b8e7 100644 --- a/git/commits_test.go +++ b/git/commits_test.go @@ -192,7 +192,7 @@ func TestCommitsOutsideARepository(t *testing.T) { t.Fatal("Commits outside a repository succeeded") } - if !strings.Contains(err.Error(), "git log --pretty=oneline --no-show-signature") { + if !strings.Contains(err.Error(), "git rev-list HEAD~1..HEAD") { t.Errorf("Commits error = %q, want it to name the git command", err) } diff --git a/git/contract_test.go b/git/contract_test.go index c021d3f..a84adb9 100644 --- a/git/contract_test.go +++ b/git/contract_test.go @@ -188,16 +188,28 @@ func TestContractCommitsListsARange(t *testing.T) { } for _, c := range []struct { - rng string - want []string + rng string + want []string + refused bool }{ - {hashes[1] + ".." + hashes[3], []string{"Subject 3", "Subject 2"}}, - {hashes[3], []string{"Subject 3", "Subject 2", "Subject 1", "Subject 0"}}, - {"HEAD", []string{"Subject 3", "Subject 2", "Subject 1", "Subject 0"}}, - {hashes[3] + ".." + hashes[3], nil}, - {"nonesuch..HEAD", []string{"Subject 3", "Subject 2", "Subject 1"}}, + {hashes[1] + ".." + hashes[3], []string{"Subject 3", "Subject 2"}, false}, + {hashes[3], []string{"Subject 3", "Subject 2", "Subject 1", "Subject 0"}, false}, + {"HEAD", []string{"Subject 3", "Subject 2", "Subject 1", "Subject 0"}, false}, + {hashes[3] + ".." + hashes[3], nil, false}, + // A range git cannot resolve is an error naming it in a repository + // with its whole history; it used to validate the whole history. + {"nonesuch..HEAD", nil, true}, } { got, err := git.Commits(c.rng) + if c.refused { + // The range, and git's own words on it. + if err == nil || !strings.Contains(err.Error(), c.rng) || !strings.Contains(err.Error(), "fatal:") { + t.Errorf("%s: error %v, want one naming the range with git's message", c.rng, err) + } + + continue + } + if err != nil { t.Errorf("%s: %v", c.rng, err) diff --git a/rules/shortsubject/shortsubject.go b/rules/shortsubject/shortsubject.go index 4cb22b1..726b341 100644 --- a/rules/shortsubject/shortsubject.go +++ b/rules/shortsubject/shortsubject.go @@ -2,6 +2,7 @@ package shortsubject import ( "strings" + "unicode/utf8" "github.com/forkcloser/git-validation/git" "github.com/forkcloser/git-validation/validate" @@ -29,7 +30,11 @@ func ValidateShortSubject(r validate.Rule, c git.CommitEntry) (vr validate.Resul return vr } - if len(c["subject"]) >= 90 { + // The limit is in characters, as GitHub's ellipsis and a reader count + // them; in bytes, 85 accented characters failed the rule. + length := utf8.RuneCountInString(c["subject"]) + + if length >= 90 { vr.Pass = false vr.Msg = "commit subject exceeds 90 characters" @@ -37,7 +42,7 @@ func ValidateShortSubject(r validate.Rule, c git.CommitEntry) (vr validate.Resul } vr.Pass = true - if len(c["subject"]) > 72 { + if length > 72 { vr.Msg = "commit subject is under 90 characters, but is still more than 72 chars" } else { vr.Msg = "commit subject is 72 characters or less! *yay*"