From 22bc4dc8ef4ffd4eea6f705753bd8f010804237f Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 26 Oct 2026 18:59:35 +0100 Subject: [PATCH 1/2] MergeInto names the changed files that are compiled programs, read in the workspace, so a check can refuse build output (issue 1577) --- go/buildstep/merge.go | 8 +++ go/buildstep/merge_integration_test.go | 9 ++-- go/buildstep/programs.go | 72 ++++++++++++++++++++++++++ go/buildstep/programs_test.go | 72 ++++++++++++++++++++++++++ 4 files changed, 158 insertions(+), 3 deletions(-) create mode 100644 go/buildstep/programs.go create mode 100644 go/buildstep/programs_test.go diff --git a/go/buildstep/merge.go b/go/buildstep/merge.go index 907009e..589c50e 100644 --- a/go/buildstep/merge.go +++ b/go/buildstep/merge.go @@ -15,6 +15,9 @@ type MergeResult struct { Base, Head, Merge string Conflicts []string Changed []string + // Programs are the changed files that are compiled programs (ExecutableKind of their first bytes, read in the workspace + // at the merge); never nil for a clean merge, so a caller tells none from a module that reports none (issue 1577). + Programs []Program } // MergeInto makes, in the workspace dir that FetchCommit left at commit, the merge of commit into the trunk commit @@ -98,6 +101,11 @@ func MergeInto(ctx context.Context, run forgekey.Runner, dir, remote, base, comm return res, fmt.Errorf("the paths the merge changes cannot be read: %w", err) } res.Changed = strings.Fields(changed) + raw, err := git("diff", "--raw", "-z", "--no-renames", "--diff-filter=AM", base, res.Merge) + if err != nil { + return res, fmt.Errorf("the files the merge adds or modifies cannot be read: %w", err) + } + res.Programs = programsIn(dir, raw) return res, nil } diff --git a/go/buildstep/merge_integration_test.go b/go/buildstep/merge_integration_test.go index 88b6d11..ce8518c 100644 --- a/go/buildstep/merge_integration_test.go +++ b/go/buildstep/merge_integration_test.go @@ -102,7 +102,7 @@ func (r *mergeRig) fetch(head string) { // holds both sides' changes. func TestMergeIntoACleanMergeAnswersTheMergeCommitAndTheChangedFiles(t *testing.T) { r := newMergeRig(t) - head := r.head(map[string]string{"b.txt": "head\n"}) + head := r.head(map[string]string{"b.txt": "head\n", "prog": "\x7fELF\x02\x01"}) base := r.advance(map[string]string{"a.txt": "one\ntwo\nthree\nfour\n"}) r.fetch(head) res, err := MergeInto(context.Background(), forgekey.ExecRunner, r.ws, r.remote, base, head, AllowFileRemote()) @@ -112,8 +112,11 @@ func TestMergeIntoACleanMergeAnswersTheMergeCommitAndTheChangedFiles(t *testing. if res.Base != base || res.Head != head || len(res.Merge) != 40 || res.Merge == base || res.Merge == head || len(res.Conflicts) != 0 { t.Fatalf("result = %+v", res) } - if !reflect.DeepEqual(res.Changed, []string{"b.txt"}) { - t.Errorf("changed = %v, want only what the head adds to the trunk: b.txt", res.Changed) + if !reflect.DeepEqual(res.Changed, []string{"b.txt", "prog"}) { + t.Errorf("changed = %v, want only what the head adds to the trunk: b.txt, prog", res.Changed) + } + if !reflect.DeepEqual(res.Programs, []Program{{"prog", "ELF"}}) { // issue 1577 + t.Errorf("programs = %v, want the ELF the head adds", res.Programs) } if at := realGit(t, r.ws, "rev-parse", "HEAD"); at != res.Merge { t.Errorf("the workspace is at %s, not at the merge %s", at, res.Merge) diff --git a/go/buildstep/programs.go b/go/buildstep/programs.go new file mode 100644 index 0000000..8d87dce --- /dev/null +++ b/go/buildstep/programs.go @@ -0,0 +1,72 @@ +package buildstep + +import ( + "bytes" + "io" + "os" + "path/filepath" + "strings" +) + +// Program is a changed file whose first bytes are an executable binary: a compiled program, which is build output and not +// source (novox/hq issue 1577: `go build` in a module's folder writes one beside go.mod, and `git add -A` takes it). +type Program struct { + Path string `json:"path"` + Kind string `json:"kind"` // ELF, Mach-O or PE +} + +// ExecutableKind says what executable format the first bytes of a file are, or "" when they are none. A PE needs its header +// ("PE\0\0" at the offset in bytes 0x3c) so a text starting with "MZ" is not one; a fat Mach-O (0xCAFEBABE) is told from a +// Java class by its architecture count (bytes 4 to 7, fewer than 0x20; a class has its version there). +func ExecutableKind(head []byte) string { + switch { + case bytes.HasPrefix(head, []byte("\x7fELF")): + return "ELF" + case bytes.HasPrefix(head, []byte("MZ")): + if len(head) >= 0x40 { + off := int(head[0x3c]) | int(head[0x3d])<<8 | int(head[0x3e])<<16 | int(head[0x3f])<<24 + if off >= 0 && off+4 <= len(head) && bytes.Equal(head[off:off+4], []byte("PE\x00\x00")) { + return "PE" + } + } + case bytes.HasPrefix(head, []byte{0xcf, 0xfa, 0xed, 0xfe}), bytes.HasPrefix(head, []byte{0xce, 0xfa, 0xed, 0xfe}), + bytes.HasPrefix(head, []byte{0xfe, 0xed, 0xfa, 0xcf}), bytes.HasPrefix(head, []byte{0xfe, 0xed, 0xfa, 0xce}), + bytes.HasPrefix(head, []byte{0xca, 0xfe, 0xba, 0xbe}) && len(head) >= 8 && head[4] == 0 && head[5] == 0 && head[6] == 0 && head[7] < 0x20: + return "Mach-O" + } + return "" +} + +// programHeadSize is how many bytes of a file are read to tell its format: enough for a PE header at its usual offsets. +const programHeadSize = 0x100 + +// programsIn reads the output of `git diff --raw -z --no-renames --diff-filter=AM` (": \0\0" per file) and names each added or modified regular file of the tree dir that is a compiled +// program, from its first bytes read locally. A symlink (mode 120000) and a submodule (160000) are not read. It never +// answers nil, so a caller can tell no programs from none reported. +func programsIn(dir, raw string) []Program { + out := []Program{} + f := strings.Split(raw, "\x00") + for i := 0; i+1 < len(f); i += 2 { + meta := strings.Fields(strings.TrimPrefix(f[i], ":")) + if len(meta) < 2 || (meta[1] != "100644" && meta[1] != "100755") { + continue + } + p := f[i+1] + full := filepath.Join(dir, filepath.FromSlash(p)) + if st, err := os.Lstat(full); err != nil || !st.Mode().IsRegular() { + continue + } + fh, err := os.Open(full) + if err != nil { + continue + } + head := make([]byte, programHeadSize) + n, _ := io.ReadFull(fh, head) + fh.Close() + if k := ExecutableKind(head[:n]); k != "" { + out = append(out, Program{Path: p, Kind: k}) + } + } + return out +} diff --git a/go/buildstep/programs_test.go b/go/buildstep/programs_test.go new file mode 100644 index 0000000..7cc26bc --- /dev/null +++ b/go/buildstep/programs_test.go @@ -0,0 +1,72 @@ +package buildstep + +import ( + "os" + "path/filepath" + "reflect" + "testing" +) + +func pe(offset int, magic string) []byte { + b := make([]byte, 0x100) + copy(b, "MZ") + b[0x3c] = byte(offset) + copy(b[offset:], magic) + return b +} + +func TestExecutableKindTellsProgramsFromText(t *testing.T) { // issue 1577 + for _, c := range []struct { + name string + head []byte + want string + }{ + {"elf", []byte("\x7fELF\x02\x01\x01"), "ELF"}, + {"mach-o 64", []byte{0xcf, 0xfa, 0xed, 0xfe, 7, 0, 0, 1}, "Mach-O"}, + {"mach-o 32 big", []byte{0xfe, 0xed, 0xfa, 0xce, 0, 0, 0, 7}, "Mach-O"}, + {"fat mach-o", []byte{0xca, 0xfe, 0xba, 0xbe, 0, 0, 0, 2}, "Mach-O"}, + {"java class", []byte{0xca, 0xfe, 0xba, 0xbe, 0, 0, 0, 0x34}, ""}, + {"pe", pe(0x80, "PE\x00\x00"), "PE"}, + {"mz text", []byte("MZ is a text file starting with two letters"), ""}, + {"mz without a pe header", pe(0x80, "XX\x00\x00"), ""}, + {"text", []byte("package main"), ""}, + {"empty", nil, ""}, + } { + if got := ExecutableKind(c.head); got != c.want { + t.Errorf("%s: kind = %q, want %q", c.name, got, c.want) + } + } +} + +func TestProgramsInNamesTheAddedAndModifiedRegularFilesThatArePrograms(t *testing.T) { // issue 1577 + dir := t.TempDir() + write := func(name string, b []byte) { + if err := os.MkdirAll(filepath.Dir(filepath.Join(dir, name)), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, name), b, 0o755); err != nil { + t.Fatal(err) + } + } + write("m/prog", []byte("\x7fELF\x02\x01")) + write("m/notes", []byte("plain text")) + write("m/mac", []byte{0xcf, 0xfa, 0xed, 0xfe, 7, 0, 0, 1}) + if err := os.Symlink("prog", filepath.Join(dir, "m/link")); err != nil { + t.Fatal(err) + } + line := func(mode, path string) string { return ":000000 " + mode + " 0000 1111 A\x00" + path + "\x00" } + for _, c := range []struct { + name, raw string + want []Program + }{ + {"none", "", []Program{}}, + {"a program and a text", line("100755", "m/prog") + line("100644", "m/notes"), []Program{{"m/prog", "ELF"}}}, + {"two programs", line("100644", "m/prog") + line("100755", "m/mac"), []Program{{"m/prog", "ELF"}, {"m/mac", "Mach-O"}}}, + {"a symlink and a submodule are not read", line("120000", "m/link") + line("160000", "m/sub"), []Program{}}, + {"a path the tree lacks", line("100644", "m/gone"), []Program{}}, + } { + if got := programsIn(dir, c.raw); !reflect.DeepEqual(got, c.want) { + t.Errorf("%s: programs = %v, want %v", c.name, got, c.want) + } + } +} -- 2.54.0 From fe11db9293973f098ef48eebe350a684574d2b39 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 26 Oct 2026 19:47:17 +0100 Subject: [PATCH 2/2] MergeInto also names programs a commit of the branch added and a later one removed: the merge commit would keep the blob in history (issue 1577) --- go/buildstep/merge.go | 5 +++ go/buildstep/merge_integration_test.go | 30 ++++++++++++++++-- go/buildstep/programs.go | 43 ++++++++++++++++++++++++++ go/buildstep/programs_test.go | 4 +-- 4 files changed, 77 insertions(+), 5 deletions(-) diff --git a/go/buildstep/merge.go b/go/buildstep/merge.go index 589c50e..86384b6 100644 --- a/go/buildstep/merge.go +++ b/go/buildstep/merge.go @@ -106,6 +106,11 @@ func MergeInto(ctx context.Context, run forgekey.Runner, dir, remote, base, comm return res, fmt.Errorf("the files the merge adds or modifies cannot be read: %w", err) } res.Programs = programsIn(dir, raw) + history, err := historyPrograms(git, base, commit, res.Programs) + if err != nil { + return res, fmt.Errorf("the commits of the branch cannot be read for compiled programs: %w", err) + } + res.Programs = append(res.Programs, history...) return res, nil } diff --git a/go/buildstep/merge_integration_test.go b/go/buildstep/merge_integration_test.go index ce8518c..f83ece8 100644 --- a/go/buildstep/merge_integration_test.go +++ b/go/buildstep/merge_integration_test.go @@ -79,6 +79,30 @@ func (r *mergeRig) head(files map[string]string) string { return h } +// headThatAddsAndRemoves is head with an earlier commit on the branch that adds a program and a later one that removes it: the +// branch's tree is clean, its history is not. It answers the head and the earlier commit. +func (r *mergeRig) headThatAddsAndRemoves(files map[string]string) (head, added string) { + realGit(r.t, r.work, "checkout", "-q", "-B", "pr", r.fork) + writeFile(r.t, r.work, "gone/prog", "\x7fELF\x02\x01") + realGit(r.t, r.work, "add", ".") + realGit(r.t, r.work, "commit", "-q", "-m", "oops") + added = realGit(r.t, r.work, "rev-parse", "HEAD") + realGit(r.t, r.work, "rm", "-q", "gone/prog") + return r.head2(files), added +} + +// head2 commits files on the current branch and pushes it as the pull request's head. +func (r *mergeRig) head2(files map[string]string) string { + for n, c := range files { + writeFile(r.t, r.work, n, c) + } + realGit(r.t, r.work, "add", ".") + realGit(r.t, r.work, "commit", "-q", "-m", "head") + h := realGit(r.t, r.work, "rev-parse", "HEAD") + realGit(r.t, r.work, "push", "-q", r.bare, "HEAD:refs/pull/1/head") + return h +} + // advance moves the trunk past the fork point with a commit changing files, pushes it and answers its sha. func (r *mergeRig) advance(files map[string]string) string { realGit(r.t, r.work, "checkout", "-q", "-B", "main", r.fork) @@ -102,7 +126,7 @@ func (r *mergeRig) fetch(head string) { // holds both sides' changes. func TestMergeIntoACleanMergeAnswersTheMergeCommitAndTheChangedFiles(t *testing.T) { r := newMergeRig(t) - head := r.head(map[string]string{"b.txt": "head\n", "prog": "\x7fELF\x02\x01"}) + head, added := r.headThatAddsAndRemoves(map[string]string{"b.txt": "head\n", "prog": "\x7fELF\x02\x01"}) base := r.advance(map[string]string{"a.txt": "one\ntwo\nthree\nfour\n"}) r.fetch(head) res, err := MergeInto(context.Background(), forgekey.ExecRunner, r.ws, r.remote, base, head, AllowFileRemote()) @@ -115,8 +139,8 @@ func TestMergeIntoACleanMergeAnswersTheMergeCommitAndTheChangedFiles(t *testing. if !reflect.DeepEqual(res.Changed, []string{"b.txt", "prog"}) { t.Errorf("changed = %v, want only what the head adds to the trunk: b.txt, prog", res.Changed) } - if !reflect.DeepEqual(res.Programs, []Program{{"prog", "ELF"}}) { // issue 1577 - t.Errorf("programs = %v, want the ELF the head adds", res.Programs) + if !reflect.DeepEqual(res.Programs, []Program{{"prog", "ELF", ""}, {"gone/prog", "ELF", added}}) { // issue 1577 + t.Errorf("programs = %v, want the ELF the head adds and the one an earlier commit added and a later one removed", res.Programs) } if at := realGit(t, r.ws, "rev-parse", "HEAD"); at != res.Merge { t.Errorf("the workspace is at %s, not at the merge %s", at, res.Merge) diff --git a/go/buildstep/programs.go b/go/buildstep/programs.go index 8d87dce..36f47e6 100644 --- a/go/buildstep/programs.go +++ b/go/buildstep/programs.go @@ -13,6 +13,9 @@ import ( type Program struct { Path string `json:"path"` Kind string `json:"kind"` // ELF, Mach-O or PE + // Commit is set for a program a commit of the branch added and a later one removed (or replaced): the merge's tree is clean, + // but the merge commit carries the blob into the trunk's history for good. Empty for a program the merge's tree holds. + Commit string `json:"commit,omitempty"` } // ExecutableKind says what executable format the first bytes of a file are, or "" when they are none. A PE needs its header @@ -40,6 +43,46 @@ func ExecutableKind(head []byte) string { // programHeadSize is how many bytes of a file are read to tell its format: enough for a PE header at its usual offsets. const programHeadSize = 0x100 +// historyPrograms names the compiled programs that a commit of base..head added or modified and the merge's tree no longer +// holds at that path (those it holds are in final already): each with the commit that added it. git is the merge's git runner. +func historyPrograms(git func(args ...string) (string, error), base, head string, final []Program) ([]Program, error) { + revs, err := git("rev-list", "--no-merges", "--reverse", base+".."+head) + if err != nil { + return nil, err + } + held := map[string]bool{} + for _, p := range final { + held[p.Path] = true + } + out, seen := []Program{}, map[string]bool{} + for _, c := range strings.Fields(revs) { + raw, err := git("diff-tree", "-r", "-z", "--no-commit-id", "--no-renames", "--root", "--diff-filter=AM", c) + if err != nil { + return nil, err + } + f := strings.Split(raw, "\x00") + for i := 0; i+1 < len(f); i += 2 { + meta := strings.Fields(strings.TrimPrefix(f[i], ":")) + if len(meta) < 4 || (meta[1] != "100644" && meta[1] != "100755") || held[f[i+1]] || seen[meta[3]] { + continue + } + seen[meta[3]] = true + blob, err := git("cat-file", "blob", meta[3]) + if err != nil { + return nil, err + } + head := []byte(blob) + if len(head) > programHeadSize { + head = head[:programHeadSize] + } + if k := ExecutableKind(head); k != "" { + out = append(out, Program{Path: f[i+1], Kind: k, Commit: c}) + } + } + } + return out, nil +} + // programsIn reads the output of `git diff --raw -z --no-renames --diff-filter=AM` (": \0\0" per file) and names each added or modified regular file of the tree dir that is a compiled // program, from its first bytes read locally. A symlink (mode 120000) and a submodule (160000) are not read. It never diff --git a/go/buildstep/programs_test.go b/go/buildstep/programs_test.go index 7cc26bc..e64dae2 100644 --- a/go/buildstep/programs_test.go +++ b/go/buildstep/programs_test.go @@ -60,8 +60,8 @@ func TestProgramsInNamesTheAddedAndModifiedRegularFilesThatArePrograms(t *testin want []Program }{ {"none", "", []Program{}}, - {"a program and a text", line("100755", "m/prog") + line("100644", "m/notes"), []Program{{"m/prog", "ELF"}}}, - {"two programs", line("100644", "m/prog") + line("100755", "m/mac"), []Program{{"m/prog", "ELF"}, {"m/mac", "Mach-O"}}}, + {"a program and a text", line("100755", "m/prog") + line("100644", "m/notes"), []Program{{"m/prog", "ELF", ""}}}, + {"two programs", line("100644", "m/prog") + line("100755", "m/mac"), []Program{{"m/prog", "ELF", ""}, {"m/mac", "Mach-O", ""}}}, {"a symlink and a submodule are not read", line("120000", "m/link") + line("160000", "m/sub"), []Program{}}, {"a path the tree lacks", line("100644", "m/gone"), []Program{}}, } { -- 2.54.0