Skip to content

Commit 2d8aab5

Browse files
heiskrCopilot
andauthored
Stop update-internal-links from reformatting untouched frontmatter (#62758)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 026fdb7b-a47f-4af1-bf87-caa90fbbdf7f
1 parent b080e34 commit 2d8aab5

3 files changed

Lines changed: 148 additions & 3 deletions

File tree

‎src/links/lib/update-internal-links.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -701,3 +701,26 @@ function singleStartingQuote(text: string) {
701701
function isSimpleQuote(text: string) {
702702
return text.startsWith('"') && text.endsWith('"') && text.split('"').length === 3
703703
}
704+
/**
705+
* Write a Markdown page back out, preserving the original frontmatter text verbatim
706+
* whenever the frontmatter data itself didn't change.
707+
*
708+
* Round-tripping frontmatter through the YAML serializer reflows values that were
709+
* never touched: long `intro` strings become block scalars, `redirect_from` entries get
710+
* rewrapped, and quote styles change. That churn dwarfs the actual link fixes and makes
711+
* a bulk run unreviewable, which is why this only reserializes when it has to.
712+
*/
713+
export function serializeMarkdown(
714+
rawContent: string,
715+
content: string,
716+
newContent: string,
717+
newData: Record<string, unknown> | undefined,
718+
differentData: boolean,
719+
): string {
720+
// `content` is the tail of the file, so everything before it is the frontmatter
721+
// block exactly as the author wrote it, delimiters and all.
722+
if (!differentData && rawContent.endsWith(content)) {
723+
return rawContent.slice(0, rawContent.length - content.length) + newContent
724+
}
725+
return frontmatter.stringify(newContent, newData || {})
726+
}

‎src/links/scripts/update-internal-links.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,7 @@ import { program } from 'commander'
1414
import chalk from 'chalk'
1515
import { dump } from 'js-yaml'
1616

17-
import { updateInternalLinks } from '@/links/lib/update-internal-links'
18-
import frontmatter from '@/frame/lib/read-frontmatter'
17+
import { updateInternalLinks, serializeMarkdown } from '@/links/lib/update-internal-links'
1918
import walkFiles from '@/workflows/walk-files'
2019

2120
program
@@ -158,7 +157,11 @@ async function main(files: string[], opts: Options) {
158157
} else {
159158
// Remember the `content` and `newContent` is the "meat" of the
160159
// Markdown page. To save it you need the frontmatter data too.
161-
fs.writeFileSync(file, frontmatter.stringify(newContent || '', newData || {}), 'utf-8')
160+
fs.writeFileSync(
161+
file,
162+
serializeMarkdown(rawContent, content, newContent, newData, differentData),
163+
'utf-8',
164+
)
162165
}
163166
}
164167
}
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
import { describe, expect, test } from 'vitest'
2+
3+
import frontmatter from '@/frame/lib/read-frontmatter'
4+
import { serializeMarkdown } from '@/links/lib/update-internal-links'
5+
6+
const PAGE = `---
7+
title: About merge methods
8+
intro: 'You can allow contributors with push access to merge their pull requests on {% data variables.product.prodname_dotcom %} with different merge options.'
9+
redirect_from:
10+
- /articles/about-merge-methods-on-github
11+
- /github/administering-a-repository/about-merge-methods-on-github
12+
versions:
13+
fpt: '*'
14+
ghes: '*'
15+
---
16+
17+
Body text with a [link](/en/old-path).
18+
`
19+
20+
// `frontmatter()` only omits `content` when the YAML fails to parse, which none of
21+
// these fixtures do. Narrow it once here so each test can stay readable.
22+
function parse(raw: string): { content: string; data: Record<string, unknown> } {
23+
const { content, data } = frontmatter(raw)
24+
if (content === undefined) throw new Error('fixture failed to parse')
25+
return { content, data: data || {} }
26+
}
27+
28+
// The whole fix rests on gray-matter's `content` being an exact suffix of the raw file.
29+
// Test that against the real parser, not a hand-rolled stand-in.
30+
describe('frontmatter parse invariant', () => {
31+
const cases: [string, string][] = [
32+
['standard page', PAGE],
33+
['CRLF line endings', PAGE.replace(/\n/g, '\r\n')],
34+
['byte order mark', `\uFEFF${PAGE}`],
35+
['no frontmatter', 'Just a body with a [link](/en/old-path).\n'],
36+
['empty frontmatter', '---\n---\n\nBody.\n'],
37+
['thematic break in body', '---\ntitle: T\n---\n\nBefore.\n\n---\n\nAfter.\n'],
38+
['empty body', '---\ntitle: T\n---\n'],
39+
['no trailing newline', '---\ntitle: T\n---\n\nBody.'],
40+
]
41+
42+
test.each(cases)('%s: content is a suffix of rawContent', (_name, raw) => {
43+
const { content } = parse(raw)
44+
45+
expect(raw.endsWith(content)).toBe(true)
46+
})
47+
48+
test.each(cases)('%s: round-trips unchanged when nothing changed', (_name, raw) => {
49+
const { content, data } = parse(raw)
50+
51+
expect(serializeMarkdown(raw, content, content, data, false)).toBe(raw)
52+
})
53+
54+
test.each(cases)('%s: preserves the frontmatter block byte-for-byte', (_name, raw) => {
55+
const { content, data } = parse(raw)
56+
const newContent = content.replace('/en/old-path', '/en/new-path')
57+
58+
const result = serializeMarkdown(raw, content, newContent, data, false)
59+
60+
const originalFrontmatter = raw.slice(0, raw.length - content.length)
61+
expect(result).toBe(originalFrontmatter + newContent)
62+
})
63+
})
64+
65+
describe('serializeMarkdown', () => {
66+
test('rewrites the body without reflowing untouched YAML', () => {
67+
const { content, data } = parse(PAGE)
68+
const newContent = content.replace('/en/old-path', '/en/new-path')
69+
70+
const result = serializeMarkdown(PAGE, content, newContent, data, false)
71+
72+
expect(result).toContain('/en/new-path')
73+
expect(result).not.toContain('/en/old-path')
74+
// The single-quoted intro and the list-style redirect_from must survive untouched.
75+
expect(result).toContain(
76+
"intro: 'You can allow contributors with push access to merge their pull requests",
77+
)
78+
expect(result).toContain(' - /articles/about-merge-methods-on-github')
79+
})
80+
81+
test('reserializes when the frontmatter data actually changed', () => {
82+
const { content } = parse(PAGE)
83+
84+
const result = serializeMarkdown(
85+
PAGE,
86+
content,
87+
content,
88+
{ title: 'About merge methods', redirect_from: ['/new-redirect'] },
89+
true,
90+
)
91+
92+
expect(result).toContain('/new-redirect')
93+
expect(result).not.toContain('/articles/about-merge-methods-on-github')
94+
expect(result).toContain('Body text with a [link](/en/old-path).')
95+
})
96+
97+
test('falls back to reserializing if content is not a clean tail of the raw file', () => {
98+
const result = serializeMarkdown(
99+
PAGE,
100+
'not a suffix of the raw file',
101+
'New body.',
102+
{ title: 'Fallback' },
103+
false,
104+
)
105+
106+
expect(result).toContain('title: Fallback')
107+
expect(result).toContain('New body.')
108+
})
109+
110+
test('is idempotent across repeated runs', () => {
111+
const { content, data } = parse(PAGE)
112+
const newContent = content.replace('/en/old-path', '/en/new-path')
113+
114+
const once = serializeMarkdown(PAGE, content, newContent, data, false)
115+
const twice = serializeMarkdown(once, parse(once).content, newContent, data, false)
116+
117+
expect(twice).toBe(once)
118+
})
119+
})

0 commit comments

Comments
 (0)