Avoid dead submodule tree links
Authored by
mfwolffe <wolffemf@dukes.jmu.edu>
- SHA
f2fb5dfad905624bbb3c58bf07e90b65b00ef993- Parents
-
9c71d87 - Tree
83ae5cf
tenseleyflow/shithub
/
f2fb5df
Browse files
6 changed files
| Status | File | + | - |
|---|---|---|---|
| M |
docs/internal/code-tab.md
|
6 | 2 |
| M |
internal/repos/git/logops.go
|
20 | 1 |
| M |
internal/repos/git/logops_test.go
|
26 | 2 |
| M |
internal/web/handlers/repo/code_tree_rows.go
|
1 | 1 |
| M |
internal/web/handlers/repo/code_tree_rows_test.go
|
9 | 0 |
| M |
internal/web/handlers/repo/submodule_links.go
|
64 | 4 |
docs/internal/code-tab.mdmodified@@ -67,8 +67,12 @@ directories first, then files alphabetically. | ||
| 67 | 67 | `commit` entries are git submodule pointers. When `.gitmodules` exists |
| 68 | 68 | on the rendered ref, the Code tab parses it once, matches entries by |
| 69 | 69 | submodule path, and links GitHub or configured shithub clone remotes to |
| 70 | -the local `/{owner}/{repo}/tree/{gitlink-oid}` route. Unknown, external, | |
| 71 | -or malformed remotes stay as plain `name @ shortsha` rows. | |
| 70 | +the local `/{owner}/{repo}/tree/{gitlink-oid}` route when the target | |
| 71 | +repo has that commit. If the target repo exists locally but does not | |
| 72 | +have the pinned commit object, the row links to the target repo's | |
| 73 | +default Code tab so independently-created mirrors don't produce dead | |
| 74 | +links. Unknown, external, absent, or malformed remotes stay as plain | |
| 75 | +`name @ shortsha` rows. | |
| 72 | 76 | |
| 73 | 77 | The S17 ship excludes the htmx-driven "last commit per entry" column |
| 74 | 78 | that the spec describes — an extra round-trip we can add later without |
internal/repos/git/logops.gomodified@@ -102,6 +102,19 @@ func CountCommits(ctx context.Context, gitDir, ref string) (int, error) { | ||
| 102 | 102 | return count, nil |
| 103 | 103 | } |
| 104 | 104 | |
| 105 | +// CommitExists reports whether sha resolves to a commit in this repository. | |
| 106 | +func CommitExists(ctx context.Context, gitDir, sha string) (bool, error) { | |
| 107 | + cmd := exec.CommandContext(ctx, "git", "-C", gitDir, "cat-file", "-e", sha+"^{commit}") | |
| 108 | + if _, err := cmd.Output(); err != nil { | |
| 109 | + var ee *exec.ExitError | |
| 110 | + if errors.As(err, &ee) && isMissingGitObjectError(ee.Stderr) { | |
| 111 | + return false, nil | |
| 112 | + } | |
| 113 | + return false, wrapExecErr(err) | |
| 114 | + } | |
| 115 | + return true, nil | |
| 116 | +} | |
| 117 | + | |
| 105 | 118 | // WeeklyCommitActivity counts commits by UTC week, oldest bucket first. |
| 106 | 119 | // It is intentionally small and read-only so list surfaces can render |
| 107 | 120 | // GitHub-style activity sparklines without parsing full commit rows. |
@@ -225,7 +238,7 @@ func GetCommit(ctx context.Context, gitDir, sha string) (CommitDetail, error) { | ||
| 225 | 238 | out, err := cmd.Output() |
| 226 | 239 | if err != nil { |
| 227 | 240 | var ee *exec.ExitError |
| 228 | - if errors.As(err, &ee) && bytes.Contains(ee.Stderr, []byte("unknown revision")) { | |
| 241 | + if errors.As(err, &ee) && isMissingGitObjectError(ee.Stderr) { | |
| 229 | 242 | return CommitDetail{}, ErrCommitNotFound |
| 230 | 243 | } |
| 231 | 244 | return CommitDetail{}, wrapExecErr(err) |
@@ -274,6 +287,12 @@ func GetCommit(ctx context.Context, gitDir, sha string) (CommitDetail, error) { | ||
| 274 | 287 | // resolve to a commit on this repo. |
| 275 | 288 | var ErrCommitNotFound = errors.New("git: commit not found") |
| 276 | 289 | |
| 290 | +func isMissingGitObjectError(stderr []byte) bool { | |
| 291 | + return bytes.Contains(stderr, []byte("unknown revision")) || | |
| 292 | + bytes.Contains(stderr, []byte("Not a valid object name")) || | |
| 293 | + bytes.Contains(stderr, []byte("bad object")) | |
| 294 | +} | |
| 295 | + | |
| 277 | 296 | // DiffStat returns the per-file change list for a SHA. We run two |
| 278 | 297 | // commands: --name-status for the letter and rename pairs, --numstat |
| 279 | 298 | // for +/- counts. Two passes is two forks, but the parsing stays |
internal/repos/git/logops_test.gomodified@@ -4,6 +4,7 @@ package git_test | ||
| 4 | 4 | |
| 5 | 5 | import ( |
| 6 | 6 | "context" |
| 7 | + "errors" | |
| 7 | 8 | "os/exec" |
| 8 | 9 | "strings" |
| 9 | 10 | "testing" |
@@ -69,8 +70,31 @@ func TestGetCommit_ReturnsErrCommitNotFound(t *testing.T) { | ||
| 69 | 70 | t.Parallel() |
| 70 | 71 | gitDir := buildSeedRepo(t) |
| 71 | 72 | _, err := gitops.GetCommit(context.Background(), gitDir, strings.Repeat("0", 40)) |
| 72 | - if err == nil { | |
| 73 | - t.Fatalf("expected error for unknown sha") | |
| 73 | + if !errors.Is(err, gitops.ErrCommitNotFound) { | |
| 74 | + t.Fatalf("GetCommit error = %v, want commit not found", err) | |
| 75 | + } | |
| 76 | +} | |
| 77 | + | |
| 78 | +func TestCommitExists(t *testing.T) { | |
| 79 | + t.Parallel() | |
| 80 | + gitDir := buildSeedRepo(t) | |
| 81 | + commits, err := gitops.Log(context.Background(), gitDir, gitops.LogOptions{Ref: "trunk"}) | |
| 82 | + if err != nil { | |
| 83 | + t.Fatalf("Log: %v", err) | |
| 84 | + } | |
| 85 | + exists, err := gitops.CommitExists(context.Background(), gitDir, commits[0].OID) | |
| 86 | + if err != nil { | |
| 87 | + t.Fatalf("CommitExists(existing): %v", err) | |
| 88 | + } | |
| 89 | + if !exists { | |
| 90 | + t.Fatal("CommitExists(existing) = false, want true") | |
| 91 | + } | |
| 92 | + exists, err = gitops.CommitExists(context.Background(), gitDir, strings.Repeat("0", 40)) | |
| 93 | + if err != nil { | |
| 94 | + t.Fatalf("CommitExists(missing): %v", err) | |
| 95 | + } | |
| 96 | + if exists { | |
| 97 | + t.Fatal("CommitExists(missing) = true, want false") | |
| 74 | 98 | } |
| 75 | 99 | } |
| 76 | 100 | |
internal/web/handlers/repo/code_tree_rows.gomodified@@ -36,7 +36,7 @@ func (h *Handlers) codeTreeEntryRows(ctx context.Context, cc *codeContext, entri | ||
| 36 | 36 | if e.Kind == repogit.EntrySubmod { |
| 37 | 37 | entryURL = "" |
| 38 | 38 | if sm, ok := submodules[fullPath]; ok { |
| 39 | - entryURL = h.submoduleTreeURL(cc, sm.URL, e.OID) | |
| 39 | + entryURL = h.submoduleTreeURL(ctx, cc, sm.URL, e.OID) | |
| 40 | 40 | } |
| 41 | 41 | } |
| 42 | 42 | row := codeTreeEntryRow{ |
internal/web/handlers/repo/code_tree_rows_test.gomodified@@ -3,6 +3,7 @@ | ||
| 3 | 3 | package repo |
| 4 | 4 | |
| 5 | 5 | import ( |
| 6 | + "strings" | |
| 6 | 7 | "testing" |
| 7 | 8 | |
| 8 | 9 | repogit "github.com/tenseleyFlow/shithub/internal/repos/git" |
@@ -79,6 +80,14 @@ func TestSubmoduleRouteURL_GitHubRemotesBecomeLocalTreeLinks(t *testing.T) { | ||
| 79 | 80 | if got != tt.want { |
| 80 | 81 | t.Fatalf("submoduleRouteURL(%q) = %q, want %q", tt.remote, got, tt.want) |
| 81 | 82 | } |
| 83 | + route, ok := submoduleRouteForRemote(cfg, tt.remote, oid) | |
| 84 | + if !ok { | |
| 85 | + t.Fatalf("submoduleRouteForRemote(%q) ok = false", tt.remote) | |
| 86 | + } | |
| 87 | + wantRepoURL := strings.TrimSuffix(tt.want, "/tree/"+oid) | |
| 88 | + if route.RepoURL != wantRepoURL { | |
| 89 | + t.Fatalf("RepoURL = %q, want %q", route.RepoURL, wantRepoURL) | |
| 90 | + } | |
| 82 | 91 | }) |
| 83 | 92 | } |
| 84 | 93 | } |
internal/web/handlers/repo/submodule_links.gomodified@@ -3,9 +3,12 @@ | ||
| 3 | 3 | package repo |
| 4 | 4 | |
| 5 | 5 | import ( |
| 6 | + "context" | |
| 6 | 7 | "net/url" |
| 7 | 8 | pathpkg "path" |
| 8 | 9 | "strings" |
| 10 | + | |
| 11 | + repogit "github.com/tenseleyFlow/shithub/internal/repos/git" | |
| 9 | 12 | ) |
| 10 | 13 | |
| 11 | 14 | type submoduleRouteConfig struct { |
@@ -15,21 +18,78 @@ type submoduleRouteConfig struct { | ||
| 15 | 18 | sshHost string |
| 16 | 19 | } |
| 17 | 20 | |
| 18 | -func (h *Handlers) submoduleTreeURL(cc *codeContext, remoteURL, oid string) string { | |
| 19 | - return submoduleRouteURL(submoduleRouteConfig{ | |
| 21 | +type submoduleRoute struct { | |
| 22 | + Owner string | |
| 23 | + RepoName string | |
| 24 | + TreeURL string | |
| 25 | + RepoURL string | |
| 26 | +} | |
| 27 | + | |
| 28 | +func (h *Handlers) submoduleTreeURL(ctx context.Context, cc *codeContext, remoteURL, oid string) string { | |
| 29 | + route, ok := submoduleRouteForRemote(submoduleRouteConfig{ | |
| 20 | 30 | owner: cc.owner, |
| 21 | 31 | repoName: cc.row.Name, |
| 22 | 32 | baseURL: h.d.CloneURLs.BaseURL, |
| 23 | 33 | sshHost: h.d.CloneURLs.SSHHost, |
| 24 | 34 | }, remoteURL, oid) |
| 35 | + if !ok { | |
| 36 | + return "" | |
| 37 | + } | |
| 38 | + gitDir, exists, err := h.localSubmoduleRepo(route.Owner, route.RepoName) | |
| 39 | + if err != nil { | |
| 40 | + if h.d.Logger != nil { | |
| 41 | + h.d.Logger.WarnContext(ctx, "code: submodule repo lookup", "error", err, "owner", route.Owner, "repo", route.RepoName) | |
| 42 | + } | |
| 43 | + return "" | |
| 44 | + } | |
| 45 | + if !exists { | |
| 46 | + return "" | |
| 47 | + } | |
| 48 | + existsAtCommit, err := repogit.CommitExists(ctx, gitDir, oid) | |
| 49 | + if err != nil { | |
| 50 | + if h.d.Logger != nil { | |
| 51 | + h.d.Logger.WarnContext(ctx, "code: submodule commit lookup", "error", err, "owner", route.Owner, "repo", route.RepoName, "oid", oid) | |
| 52 | + } | |
| 53 | + return route.RepoURL | |
| 54 | + } | |
| 55 | + if !existsAtCommit { | |
| 56 | + return route.RepoURL | |
| 57 | + } | |
| 58 | + return route.TreeURL | |
| 25 | 59 | } |
| 26 | 60 | |
| 27 | 61 | func submoduleRouteURL(cfg submoduleRouteConfig, remoteURL, oid string) string { |
| 62 | + route, ok := submoduleRouteForRemote(cfg, remoteURL, oid) | |
| 63 | + if !ok { | |
| 64 | + return "" | |
| 65 | + } | |
| 66 | + return route.TreeURL | |
| 67 | +} | |
| 68 | + | |
| 69 | +func submoduleRouteForRemote(cfg submoduleRouteConfig, remoteURL, oid string) (submoduleRoute, bool) { | |
| 28 | 70 | owner, repoName, ok := submoduleRepoTarget(cfg, remoteURL) |
| 29 | 71 | if !ok || oid == "" { |
| 30 | - return "" | |
| 72 | + return submoduleRoute{}, false | |
| 73 | + } | |
| 74 | + base := "/" + url.PathEscape(owner) + "/" + url.PathEscape(repoName) | |
| 75 | + return submoduleRoute{ | |
| 76 | + Owner: owner, | |
| 77 | + RepoName: repoName, | |
| 78 | + TreeURL: base + "/tree/" + url.PathEscape(oid), | |
| 79 | + RepoURL: base, | |
| 80 | + }, true | |
| 81 | +} | |
| 82 | + | |
| 83 | +func (h *Handlers) localSubmoduleRepo(owner, repoName string) (string, bool, error) { | |
| 84 | + gitDir, err := h.d.RepoFS.RepoPath(owner, repoName) | |
| 85 | + if err != nil { | |
| 86 | + return "", false, err | |
| 87 | + } | |
| 88 | + exists, err := h.d.RepoFS.Exists(gitDir) | |
| 89 | + if err != nil { | |
| 90 | + return "", false, err | |
| 31 | 91 | } |
| 32 | - return "/" + url.PathEscape(owner) + "/" + url.PathEscape(repoName) + "/tree/" + url.PathEscape(oid) | |
| 92 | + return gitDir, exists, nil | |
| 33 | 93 | } |
| 34 | 94 | |
| 35 | 95 | func submoduleRepoTarget(cfg submoduleRouteConfig, remoteURL string) (owner, repoName string, ok bool) { |