From c44cd6b1586d29b54114aaf3e22adb2361c6dec5 Mon Sep 17 00:00:00 2001 From: tamnd <1218621+tamnd@users.noreply.github.com> Date: Sat, 25 Jul 2026 21:43:36 +0700 Subject: [PATCH] Clear the linter, and fix the three things it found CI has been red on lint since the rewrite landed. Nineteen issues, and three of them were real. Page said it escaped every path segment and copied them across untouched. That works until the first branch with a space in its name. It escapes now. GetJSON handed a 406 or a 410 to the JSON decoder and reported whatever the decoder made of the HTML it got. Both statuses are routing answers, so it says which road instead: noJSONHere, which had been written for exactly this and never wired up. github archive -o wrote to a file with a deferred close whose error went nowhere, and a close is where the last of a write gets flushed. It closes by hand and returns the error. The rest is dead code and style. Two thirds of merge.go was a reflective field-level merge that nothing calls: records are assembled by decoding several blocks of one page into one struct, so there is nothing to merge. What is left is the three provenance helpers that are used, and the file is called provenance.go now because that is what it does. timeoutFor went with it, since the byte-plane commands it named do get a context. Then a De Morgan, two append loops, a tagged switch, an ineffectual assignment, and a comment that still called railsNext by its old name. --- cli/files.go | 15 ++- gh/client.go | 13 ++- gh/commit.go | 3 +- gh/contents.go | 2 +- gh/discover.go | 4 +- gh/domain.go | 5 - gh/merge.go | 249 ----------------------------------------------- gh/people.go | 2 +- gh/provenance.go | 69 +++++++++++++ gh/rdf.go | 6 +- gh/surface.go | 14 +-- 11 files changed, 101 insertions(+), 281 deletions(-) delete mode 100644 gh/merge.go create mode 100644 gh/provenance.go diff --git a/cli/files.go b/cli/files.go index 97a354b..b136949 100644 --- a/cli/files.go +++ b/cli/files.go @@ -207,18 +207,23 @@ func (c *archiveCmd) run(ctx context.Context, args []string) error { if err != nil { return err } - defer body.Close() + defer func() { _ = body.Close() }() - w := io.Writer(os.Stdout) if c.out != "" { f, err := os.Create(c.out) if err != nil { return err } - defer f.Close() - w = f + // Closed by hand rather than deferred, and the error is returned. An + // archive is written to a file someone means to keep, and a close that + // fails is the one that flushes the last of it. + if _, err := io.Copy(f, body); err != nil { + _ = f.Close() + return err + } + return f.Close() } - _, err = io.Copy(w, body) + _, err = io.Copy(os.Stdout, body) return err } diff --git a/gh/client.go b/gh/client.go index 8b776f6..9b8c05c 100644 --- a/gh/client.go +++ b/gh/client.go @@ -179,6 +179,13 @@ func (c *Client) GetJSON(ctx context.Context, rawURL string, s Surface, v any) ( if err != nil { return nil, err } + // 406 and 410 are routing answers, not failures, so the fetch let them + // through. For a caller that wanted JSON they are the end of the road, and + // saying which road is more use than the decoder's complaint about the + // HTML it was handed. + if resp.Status == http.StatusNotAcceptable || resp.Status == http.StatusGone { + return resp, noJSONHere(rawURL) + } if v != nil { if err := json.Unmarshal(resp.Body, v); err != nil { return resp, errs.New(errs.KindNetwork, "cannot decode the json from %s: %v", shortURL(rawURL), err) @@ -403,10 +410,14 @@ func (c *Client) Poll(ctx context.Context, rawURL string, s Surface) (*Response, // --- URL building --- // Page builds a github.com URL from path segments, escaping each one. +// +// It really does escape now. It used to copy the segments across untouched +// while the comment said otherwise, which is the kind of helper that works +// until the first branch with a space in its name. func Page(parts ...string) string { esc := make([]string, 0, len(parts)) for _, p := range parts { - esc = append(esc, p) + esc = append(esc, url.PathEscape(p)) } return BaseURL + "/" + strings.Join(esc, "/") } diff --git a/gh/commit.go b/gh/commit.go index 91fa584..9d8a1f3 100644 --- a/gh/commit.go +++ b/gh/commit.go @@ -706,7 +706,6 @@ func (c *Client) Release(ctx context.Context, repo, tag string, opts ReleaseOpti rel.setIdentity(KindRelease, repo+"@"+tag) } rel.addSource(res.FinalURL) - tag = rel.Tag if !opts.Body { rel.Body, rel.BodyHTML = "", "" } @@ -1114,7 +1113,7 @@ func isMailboxHeader(line string) bool { func isHex(s string) bool { for _, r := range s { - if !(r >= '0' && r <= '9' || r >= 'a' && r <= 'f') { + if (r < '0' || r > '9') && (r < 'a' || r > 'f') { return false } } diff --git a/gh/contents.go b/gh/contents.go index 2505439..053ec5a 100644 --- a/gh/contents.go +++ b/gh/contents.go @@ -478,7 +478,7 @@ func (c *Client) Download(ctx context.Context, repo, ref, path string, w io.Writ if err != nil { return 0, err } - defer body.Close() + defer func() { _ = body.Close() }() return io.Copy(w, body) } diff --git a/gh/discover.go b/gh/discover.go index 87bbb7b..658ce21 100644 --- a/gh/discover.go +++ b/gh/discover.go @@ -275,9 +275,7 @@ func (c *Client) TopicPage(ctx context.Context, slug string) (*Topic, error) { t.StargazerCount = intp(n) } } - for _, rel := range relatedTopics(doc, slug) { - t.Related = append(t.Related, rel) - } + t.Related = append(t.Related, relatedTopics(doc, slug)...) if t.DisplayName == "" && t.Description == "" { return nil, structureChanged(slug) } diff --git a/gh/domain.go b/gh/domain.go index a7f58b0..9bd3def 100644 --- a/gh/domain.go +++ b/gh/domain.go @@ -4,7 +4,6 @@ import ( "context" "net/http" "path/filepath" - "time" "github.com/tamnd/any-cli/kit" ) @@ -137,7 +136,3 @@ func newClientFor(_ context.Context, cfg kit.Config) (any, error) { c.HTTP = &http.Client{Timeout: conf.Timeout} return c, nil } - -// timeoutFor is the per-command ceiling for the walks that can run long, used -// by the byte-plane commands that do not get a kit-managed context. -const timeoutFor = 10 * time.Minute diff --git a/gh/merge.go b/gh/merge.go deleted file mode 100644 index fd2861e..0000000 --- a/gh/merge.go +++ /dev/null @@ -1,249 +0,0 @@ -package gh - -import ( - "encoding/json" - "fmt" - "reflect" - "sort" - "strings" -) - -// merge.go combines the several surfaces that answer for one record. -// -// The order is explicit and lives at the call site, one ordered list per record -// type, because a merge that cannot say which surface won is a merge that -// cannot be debugged. What lives here is only the per-field mechanics that all -// of them share: -// -// - a source that did not return a field never clears it, so merging is -// driven by a populated check and never by struct assignment, -// - Sources accumulates in merge order, so provenance is the merge history, -// - when two surfaces give different non-empty values for one field, the -// later one wins and the disagreement is recorded in Extra["_conflict"]. -// -// That last rule is the interesting one. The scenario suite asserts _conflict -// is empty, so two surfaces disagreeing becomes a named test failure and gets -// looked at, instead of being quietly averaged into a number nobody can trace. - -// mergeInto copies every populated field of src into dst, returning the fields -// where the two disagreed. dst and src must be pointers to the same struct -// type. -// -// Booleans are a known soft spot: a false bool is indistinguishable from an -// unset one, so a false never overwrites a true. Every bool in the model is -// phrased so that false is the safe default (IsFork, IsArchived, HasWiki), which -// makes that the right behaviour rather than a compromise. -func mergeInto(dst, src any) map[string][]string { - dv := reflect.ValueOf(dst) - sv := reflect.ValueOf(src) - if dv.Kind() != reflect.Pointer || sv.Kind() != reflect.Pointer { - return nil - } - if dv.Type() != sv.Type() { - return nil - } - conflicts := map[string][]string{} - mergeStruct(dv.Elem(), sv.Elem(), "", conflicts) - if len(conflicts) == 0 { - return nil - } - return conflicts -} - -func mergeStruct(dst, src reflect.Value, prefix string, conflicts map[string][]string) { - t := dst.Type() - for i := range t.NumField() { - f := t.Field(i) - if !f.IsExported() { - continue - } - d, s := dst.Field(i), src.Field(i) - - // Base is the envelope, not data. Its fields have their own merge rules - // and are handled by mergeBase. - if f.Anonymous && f.Type == reflect.TypeOf(Base{}) { - mergeBase(d.Addr().Interface().(*Base), s.Addr().Interface().(*Base)) - continue - } - // Any other embedded struct is part of the record: Thread inside Issue, - // Repo inside Trending, Account inside Org. - if f.Anonymous && f.Type.Kind() == reflect.Struct { - mergeStruct(d, s, prefix, conflicts) - continue - } - - name := jsonName(f) - if name == "-" { - continue - } - if prefix != "" { - name = prefix + "." + name - } - if !populated(s) { - continue - } - if populated(d) && !reflect.DeepEqual(d.Interface(), s.Interface()) { - conflicts[name] = []string{display(d), display(s)} - } - d.Set(s) - } -} - -// mergeBase keeps the identity of the first source and unions the provenance. -// A later surface never renames a record: if the id changed, the merge was -// between two different things and the caller made a mistake. -func mergeBase(dst, src *Base) { - if dst.Kind == "" { - dst.Kind = src.Kind - } - if dst.ID == "" { - dst.ID = src.ID - } - if dst.URI == "" { - dst.URI = src.URI - } - if dst.URL == "" { - dst.URL = src.URL - } - dst.addSource(src.Sources...) - for field, tier := range src.Via { - recordVia(dst, field, tier) - } - dst.Extra = mergeExtra(dst.Extra, src.Extra) -} - -// mergeExtra unions two unmodelled-key sets. Both are keys nobody claimed, so -// there is nothing smarter to do than keep them all; a key present in both -// keeps the later value, matching the field rule above. -func mergeExtra(dst, src json.RawMessage) json.RawMessage { - if len(src) == 0 { - return dst - } - if len(dst) == 0 { - return src - } - var a, b map[string]json.RawMessage - if json.Unmarshal(dst, &a) != nil || json.Unmarshal(src, &b) != nil { - return dst - } - for k, v := range b { - a[k] = v - } - out, err := json.Marshal(a) - if err != nil { - return dst - } - return out -} - -// recordConflicts writes the disagreements into Extra["_conflict"]. It is a -// reserved key and the suite asserts it is absent, which is the whole point: -// this is a tripwire, not a feature. -func recordConflicts(b *Base, conflicts map[string][]string) { - if len(conflicts) == 0 { - return - } - m := map[string]json.RawMessage{} - if len(b.Extra) > 0 { - _ = json.Unmarshal(b.Extra, &m) - } - existing := map[string][]string{} - if raw, ok := m["_conflict"]; ok { - _ = json.Unmarshal(raw, &existing) - } - for k, v := range conflicts { - existing[k] = v - } - raw, err := json.Marshal(existing) - if err != nil { - return - } - m["_conflict"] = raw - out, err := json.Marshal(m) - if err != nil { - return - } - b.Extra = out -} - -// recordVia notes which extraction tier produced a field. It is what tells you -// that a field which used to arrive from a JSON payload is now arriving from a -// class selector, which is the early warning that something moved. -func recordVia(b *Base, field, tier string) { - if b.Via == nil { - b.Via = map[string]string{} - } - b.Via[field] = tier -} - -// populated is the "did this surface actually say something" test. Absent is -// not zero anywhere in this tool, and this function is where that rule is -// enforced for the merge. -func populated(v reflect.Value) bool { - switch v.Kind() { - case reflect.Pointer, reflect.Interface, reflect.Map, reflect.Slice: - return !v.IsNil() && (v.Kind() != reflect.Slice && v.Kind() != reflect.Map || v.Len() > 0) - case reflect.String: - return v.String() != "" - case reflect.Bool: - return v.Bool() - case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - return v.Int() != 0 - case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - return v.Uint() != 0 - case reflect.Float32, reflect.Float64: - return v.Float() != 0 - case reflect.Struct: - return !v.IsZero() - default: - return !v.IsZero() - } -} - -func jsonName(f reflect.StructField) string { - tag := f.Tag.Get("json") - name, _, _ := strings.Cut(tag, ",") - if name == "" { - return f.Name - } - return name -} - -// display renders a value for a conflict entry. It stays short: the point is to -// let a human see which two surfaces disagreed, not to reproduce the payload. -func display(v reflect.Value) string { - if v.Kind() == reflect.Pointer && !v.IsNil() { - v = v.Elem() - } - switch v.Kind() { - case reflect.String: - return truncate(v.String(), 120) - case reflect.Slice, reflect.Map, reflect.Struct: - raw, err := json.Marshal(v.Interface()) - if err != nil { - return fmt.Sprint(v.Interface()) - } - return truncate(string(raw), 200) - default: - return fmt.Sprint(v.Interface()) - } -} - -func truncate(s string, n int) string { - if len(s) <= n { - return s - } - return s[:n] + "..." -} - -// sortedKeys keeps merge and conflict output deterministic. Map order is random -// in Go, and a record whose field order changes between runs makes every diff -// of two outputs useless. -func sortedKeys[V any](m map[string]V) []string { - out := make([]string, 0, len(m)) - for k := range m { - out = append(out, k) - } - sort.Strings(out) - return out -} diff --git a/gh/people.go b/gh/people.go index 2723ebe..bf47b5d 100644 --- a/gh/people.go +++ b/gh/people.go @@ -712,7 +712,7 @@ func firstTime(ss ...string) *time.Time { // --- the rails pager --- -// nextPageToken returns the page number to ask for next, or empty when the page +// railsNext returns the page number to ask for next, or empty when the page // says there is no next. // // Two templates write the same link two ways: the organization roster marks it diff --git a/gh/provenance.go b/gh/provenance.go new file mode 100644 index 0000000..1d9e0f2 --- /dev/null +++ b/gh/provenance.go @@ -0,0 +1,69 @@ +package gh + +import ( + "encoding/json" + "sort" +) + +// provenance.go records where a record's fields came from and where its sources +// disagreed. +// +// Records are assembled by decoding several blocks of one page into one struct, +// so there is no field-level merge to do. What is worth keeping is the note of +// which extraction tier answered for a field, and the tripwire for the case +// where two blocks say different things about the same field. The suite asserts +// _conflict is absent, so a disagreement becomes a named test failure and gets +// looked at, instead of being quietly resolved in favour of whichever decoder +// ran last. + +// recordConflicts writes the disagreements into Extra["_conflict"]. It is a +// reserved key and the suite asserts it is absent, which is the whole point: +// this is a tripwire, not a feature. +func recordConflicts(b *Base, conflicts map[string][]string) { + if len(conflicts) == 0 { + return + } + m := map[string]json.RawMessage{} + if len(b.Extra) > 0 { + _ = json.Unmarshal(b.Extra, &m) + } + existing := map[string][]string{} + if raw, ok := m["_conflict"]; ok { + _ = json.Unmarshal(raw, &existing) + } + for k, v := range conflicts { + existing[k] = v + } + raw, err := json.Marshal(existing) + if err != nil { + return + } + m["_conflict"] = raw + out, err := json.Marshal(m) + if err != nil { + return + } + b.Extra = out +} + +// recordVia notes which extraction tier produced a field. It is what tells you +// that a field which used to arrive from a JSON payload is now arriving from a +// class selector, which is the early warning that something moved. +func recordVia(b *Base, field, tier string) { + if b.Via == nil { + b.Via = map[string]string{} + } + b.Via[field] = tier +} + +// sortedKeys keeps merge and conflict output deterministic. Map order is random +// in Go, and a record whose field order changes between runs makes every diff +// of two outputs useless. +func sortedKeys[V any](m map[string]V) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} diff --git a/gh/rdf.go b/gh/rdf.go index cda8f99..f98b3bb 100644 --- a/gh/rdf.go +++ b/gh/rdf.go @@ -326,10 +326,10 @@ func slug(s string) string { s = strings.TrimSpace(s) var b strings.Builder for _, r := range s { - switch { - case r == ' ' || r == '/' || r == '\\': + switch r { + case ' ', '/', '\\': b.WriteByte('-') - case r == '<' || r == '>' || r == '"' || r == '{' || r == '}' || r == '|' || r == '^' || r == '`': + case '<', '>', '"', '{', '}', '|', '^', '`': // Characters an IRI may not carry. Dropping them beats escaping // them, because nobody wants gh:license/BSD%203-Clause. default: diff --git a/gh/surface.go b/gh/surface.go index bbdd552..97c02fb 100644 --- a/gh/surface.go +++ b/gh/surface.go @@ -175,10 +175,9 @@ func paginate[T any](ctx context.Context, limit int, fetch fetchPage[T], emit fu } } -// pageToken and cursorToken are the two token encodings. Keeping them as helper -// functions rather than inline strconv calls means the "which page am I on" -// logic reads the same in all nine listings that use it. - +// pageToken decodes the numbered-page token. Keeping it as a helper rather than +// an inline strconv call means the "which page am I on" logic reads the same in +// all nine listings that use it. func pageToken(token string) int { if token == "" { return 1 @@ -189,10 +188,3 @@ func pageToken(token string) int { } return n } - -func nextPageToken(current int, gotFull bool) string { - if !gotFull { - return "" - } - return strconv.Itoa(current + 1) -}