STRLCPY/scorecard

Remove unused code from changeset creation (#2776)
```
Signed-off-by: Azeem Shaikh <[email protected]>
```
Azeem Shaikh committed with GitHub 1 year ago

9a51f252

1 parent dfc24396

■ ■ ■ ■ ■ ■

checks/raw/code_review.go

		skipped 182 lines
183	183
184	184		// Changesets are returned in map order (i.e. randomized)
185	185		for ri := range changesetsByRevInfo {
186		-	// Ungroup all commits that don't have revision info
187		-	cs := changesetsByRevInfo[ri]
188		-	missing := revisionInfo{}
189		-	if ri == missing {
190		-	for i := range cs.Commits {
191		-	c := cs.Commits[i]
192		-	changesets = append(changesets, checker.Changeset{
193		-	Commits: []clients.Commit{c},
194		-	})
195		-	}
196		-	} else {
197		-	changesets = append(changesets, cs)
198		-	}
	186	+	changesets = append(changesets, changesetsByRevInfo[ri])
199	187		}
200	188
201	189		return changesets
		skipped 2 lines

■ ■ ■ ■ ■ ■

checks/raw/code_review_test.go

		skipped 14 lines
15	15		package raw
16	16
17	17		import (
18		-	"strings"
19	18		"testing"
20	19		"time"
21	20
22		-	"golang.org/x/exp/slices"
	21	+	"github.com/google/go-cmp/cmp"
	22	+	"github.com/google/go-cmp/cmp/cmpopts"
23	23
24	24		"github.com/ossf/scorecard/v4/checker"
25	25		"github.com/ossf/scorecard/v4/clients"
26	26		)
27	27
28		-	//nolint:gocritic
29		-	func assertCommitEq(t *testing.T, actual clients.Commit, expected clients.Commit) {
30		-	t.Helper()
31		-
32		-	if actual.SHA != expected.SHA {
33		-	t.Fatalf("commit shas mismatched\na.sha: %s\nb.sha %s", actual.SHA, expected.SHA)
34		-	}
35		-	}
36		-
37		-	func assertChangesetEq(t testing.T, actual, expected checker.Changeset) {
38		-	t.Helper()
39		-
40		-	if actual.ReviewPlatform != expected.ReviewPlatform {
41		-	t.Fatalf(
42		-	"changeset review platform mismatched\na.platform: %s\nb.platform: %s",
43		-	actual.ReviewPlatform,
44		-	expected.ReviewPlatform,
45		-	)
46		-	}
47		-
48		-	if actual.RevisionID != expected.RevisionID {
49		-	t.Fatalf(
50		-	"changeset revisionID mismatched\na.revid: %s\nb.revid %s",
51		-	actual.RevisionID,
52		-	expected.RevisionID,
53		-	)
54		-	}
55		-
56		-	if len(actual.Commits) != len(expected.Commits) {
57		-	t.Fatalf(
58		-	"changesets contain different numbers of commits\na:%d\nb:%d",
59		-	len(actual.Commits),
60		-	len(expected.Commits),
61		-	)
62		-	}
63		-
64		-	for i := range actual.Commits {
65		-	assertCommitEq(t, actual.Commits[i], expected.Commits[i])
66		-	}
67		-	}
68		-
69		-	//nolint:gocritic
70		-	func csless(a, b checker.Changeset) bool {
71		-	if cmp := strings.Compare(a.RevisionID, b.RevisionID); cmp != 0 {
72		-	return cmp < 0
73		-	}
74		-
75		-	return a.ReviewPlatform < b.ReviewPlatform
76		-	}
77		-
78		-	func assertChangesetArrEq(t *testing.T, actual, expected []checker.Changeset) {
79		-	t.Helper()
80		-
81		-	if len(actual) != len(expected) {
82		-	t.Fatalf("different number of changesets\na:%d\nb:%d", len(actual), len(expected))
83		-	}
84		-
85		-	slices.SortFunc(actual, csless)
86		-	slices.SortFunc(expected, csless)
87		-
88		-	for i := range actual {
89		-	assertChangesetEq(t, &actual[i], &expected[i])
90		-	}
91		-	}
92		-
93	28		// TestCodeReviews tests the CodeReviews function.
94	29		func Test_getChangesets(t *testing.T) {
95	30		t.Parallel()
	31	+	var (
	32	+	commitC = clients.Commit{
	33	+	SHA: "c",
	34	+	AssociatedMergeRequest: clients.PullRequest{
	35	+	Number: 3,
	36	+	MergedAt: time.Date(2023 /year/, time.March, 21, /day/
	37	+	13 /hour/, 42 /min/, 0 /sec/, 0 /nsec/, time.UTC),
	38	+	},
	39	+	Message: "merge commitSHA c form GitHub",
	40	+	}
	41	+	commitB = clients.Commit{
	42	+	SHA: "b",
	43	+	AssociatedMergeRequest: clients.PullRequest{
	44	+	Number: 2,
	45	+	MergedAt: time.Date(2023 /year/, time.March, 21, /day/
	46	+	13 /hour/, 41 /min/, 0 /sec/, 0 /nsec/, time.UTC),
	47	+	},
	48	+	Message: "merge commitSHA b from GitHub",
	49	+	}
	50	+	commitBUnsquashed = clients.Commit{
	51	+	SHA: "b_unsquashed",
	52	+	AssociatedMergeRequest: clients.PullRequest{
	53	+	Number: 2,
	54	+	MergedAt: time.Date(2023 /year/, time.March, 21, /day/
	55	+	13 /hour/, 40 /min/, 0 /sec/, 0 /nsec/, time.UTC),
	56	+	},
	57	+	Message: "unsquashed commitSHA b_unsquashed from GitHub",
	58	+	}
	59	+	commitA = clients.Commit{
	60	+	SHA: "a",
	61	+	AssociatedMergeRequest: clients.PullRequest{
	62	+	Number: 1,
	63	+	MergedAt: time.Date(2023 /year/, time.March, 21, /day/
	64	+	13 /hour/, 39 /min/, 0 /sec/, 0 /nsec/, time.UTC),
	65	+	},
	66	+	Message: "merge commitSHA a from GitHub",
	67	+	}
	68	+
	69	+	phabricatorCommitA = clients.Commit{
	70	+	Message: "\nDifferential Revision: 123\nReviewed By: user-123",
	71	+	SHA: "abc",
	72	+	}
	73	+	phabricatorCommitAUnsquashed = clients.Commit{
	74	+	Message: "\nDifferential Revision: 123\nReviewed By: user-123",
	75	+	SHA: "adef",
	76	+	}
	77	+	phabricatorCommitAUnsquashed2 = clients.Commit{
	78	+	Message: "\nDifferential Revision: 123\nReviewed By: user-456",
	79	+	SHA: "afab",
	80	+	}
	81	+	phabricatorCommitB = clients.Commit{
	82	+	Message: "\nDifferential Revision: 158\nReviewed By: user-123",
	83	+	SHA: "def",
	84	+	}
	85	+	phabricatorCommitC = clients.Commit{
	86	+	Message: "\nDifferential Revision: 2000\nReviewed By: user-456",
	87	+	SHA: "fab",
	88	+	}
	89	+	phabricatorCommitD = clients.Commit{
	90	+	Message: "\nDifferential Revision: 2\nReviewed By: user-456",
	91	+	SHA: "d",
	92	+	}
	93	+
	94	+	gerritCommitB = clients.Commit{
	95	+	Message: "first change\nReviewed-on: server.url \nReviewed-by:user-123",
	96	+	SHA: "abc",
	97	+	}
	98	+	gerritCommitA = clients.Commit{
	99	+	Message: "followup\nReviewed-on: server.url \nReviewed-by:user-123",
	100	+	SHA: "def",
	101	+	}
	102	+	)
96	103
97	104		tests := []struct {
98	105		name string
		skipped 1 lines
100	107		expected []checker.Changeset
101	108		}{
102	109		{
103		-	name: "commit history squashed during merge",
104		-	commits: []clients.Commit{
105		-	{
106		-	SHA: "a",
107		-	AssociatedMergeRequest: clients.PullRequest{Number: 3, MergedAt: time.Now()},
108		-	},
109		-	{
110		-	SHA: "b",
111		-	AssociatedMergeRequest: clients.PullRequest{Number: 2, MergedAt: time.Now()},
112		-	},
113		-	{
114		-	SHA: "c",
115		-	AssociatedMergeRequest: clients.PullRequest{Number: 1, MergedAt: time.Now()},
116		-	},
117		-	},
	110	+	name: "github: merge with squash",
	111	+	commits: []clients.Commit{commitC, commitB, commitA},
118	112		expected: []checker.Changeset{
119	113		{
120	114		ReviewPlatform: checker.ReviewPlatformGitHub,
121	115		RevisionID: "3",
122		-	Commits: []clients.Commit{{SHA: "a"}},
	116	+	Commits: []clients.Commit{commitC},
	117	+	Reviews: []clients.Review{
	118	+	{
	119	+	Author: &clients.User{},
	120	+	State: "APPROVED",
	121	+	},
	122	+	},
123	123		},
124	124		{
125	125		ReviewPlatform: checker.ReviewPlatformGitHub,
126	126		RevisionID: "2",
127		-	Commits: []clients.Commit{{SHA: "b"}},
	127	+	Commits: []clients.Commit{commitB},
	128	+	Reviews: []clients.Review{
	129	+	{
	130	+	Author: &clients.User{},
	131	+	State: "APPROVED",
	132	+	},
	133	+	},
128	134		},
129	135		{
130	136		ReviewPlatform: checker.ReviewPlatformGitHub,
131	137		RevisionID: "1",
132		-	Commits: []clients.Commit{{SHA: "c"}},
	138	+	Commits: []clients.Commit{commitA},
	139	+	Reviews: []clients.Review{
	140	+	{
	141	+	Author: &clients.User{},
	142	+	State: "APPROVED",
	143	+	},
	144	+	},
133	145		},
134	146		},
135	147		},
136	148		{
137		-	name: "commits in reverse chronological order",
138		-	commits: []clients.Commit{
139		-	{
140		-	SHA: "a",
141		-	AssociatedMergeRequest: clients.PullRequest{Number: 1, MergedAt: time.Now()},
142		-	},
143		-	{
144		-	SHA: "b",
145		-	AssociatedMergeRequest: clients.PullRequest{Number: 2, MergedAt: time.Now()},
146		-	},
147		-	{
148		-	SHA: "c",
149		-	AssociatedMergeRequest: clients.PullRequest{Number: 3, MergedAt: time.Now()},
150		-	},
151		-	},
	149	+	name: "github: merge with squash reverse chronological order",
	150	+	commits: []clients.Commit{commitA, commitB, commitC},
152	151		expected: []checker.Changeset{
153	152		{
154	153		ReviewPlatform: checker.ReviewPlatformGitHub,
155	154		RevisionID: "1",
156		-	Commits: []clients.Commit{
	155	+	Commits: []clients.Commit{commitA},
	156	+	Reviews: []clients.Review{
157	157		{
158		-	SHA: "a",
159		-	AssociatedMergeRequest: clients.PullRequest{Number: 1},
	158	+	Author: &clients.User{},
	159	+	State: "APPROVED",
160	160		},
161	161		},
162	162		},
163	163		{
164	164		ReviewPlatform: checker.ReviewPlatformGitHub,
165	165		RevisionID: "2",
166		-	Commits: []clients.Commit{
	166	+	Commits: []clients.Commit{commitB},
	167	+	Reviews: []clients.Review{
167	168		{
168		-	SHA: "b",
169		-	AssociatedMergeRequest: clients.PullRequest{Number: 2},
	169	+	Author: &clients.User{},
	170	+	State: "APPROVED",
170	171		},
171	172		},
172	173		},
173	174		{
174	175		ReviewPlatform: checker.ReviewPlatformGitHub,
175	176		RevisionID: "3",
176		-	Commits: []clients.Commit{
	177	+	Commits: []clients.Commit{commitC},
	178	+	Reviews: []clients.Review{
177	179		{
178		-	SHA: "c",
179		-	AssociatedMergeRequest: clients.PullRequest{Number: 3},
	180	+	Author: &clients.User{},
	181	+	State: "APPROVED",
180	182		},
181	183		},
182	184		},
183	185		},
184	186		},
185	187		{
186		-	name: "without commit squashing",
187		-	commits: []clients.Commit{
188		-	{
189		-	SHA: "foo",
190		-	AssociatedMergeRequest: clients.PullRequest{Number: 1, MergedAt: time.Now()},
191		-	},
	188	+	name: "github: merge without squash",
	189	+	commits: []clients.Commit{commitC, commitB, commitBUnsquashed},
	190	+	expected: []checker.Changeset{
192	191		{
193		-	SHA: "bar",
194		-	AssociatedMergeRequest: clients.PullRequest{Number: 2, MergedAt: time.Now()},
	192	+	ReviewPlatform: checker.ReviewPlatformGitHub,
	193	+	RevisionID: "3",
	194	+	Commits: []clients.Commit{commitC},
	195	+	Reviews: []clients.Review{
	196	+	{
	197	+	Author: &clients.User{},
	198	+	State: "APPROVED",
	199	+	},
	200	+	},
195	201		},
196	202		{
197		-	SHA: "baz",
198		-	AssociatedMergeRequest: clients.PullRequest{Number: 2, MergedAt: time.Now()},
	203	+	ReviewPlatform: checker.ReviewPlatformGitHub,
	204	+	RevisionID: "2",
	205	+	Commits: []clients.Commit{commitB, commitBUnsquashed},
	206	+	Reviews: []clients.Review{
	207	+	{
	208	+	Author: &clients.User{},
	209	+	State: "APPROVED",
	210	+	},
	211	+	},
199	212		},
200	213		},
	214	+	},
	215	+	{
	216	+	name: "github: merge without squash reverse chronological order",
	217	+	commits: []clients.Commit{commitA, commitBUnsquashed, commitB, commitC},
201	218		expected: []checker.Changeset{
202	219		{
203	220		ReviewPlatform: checker.ReviewPlatformGitHub,
204	221		RevisionID: "1",
205		-	Commits: []clients.Commit{
	222	+	Commits: []clients.Commit{commitA},
	223	+	Reviews: []clients.Review{
206	224		{
207		-	SHA: "foo",
208		-	AssociatedMergeRequest: clients.PullRequest{Number: 1},
	225	+	Author: &clients.User{},
	226	+	State: "APPROVED",
209	227		},
210	228		},
211	229		},
212	230		{
213	231		ReviewPlatform: checker.ReviewPlatformGitHub,
214	232		RevisionID: "2",
215		-	Commits: []clients.Commit{
	233	+	Commits: []clients.Commit{commitB, commitBUnsquashed},
	234	+	Reviews: []clients.Review{
216	235		{
217		-	SHA: "bar",
218		-	AssociatedMergeRequest: clients.PullRequest{Number: 2},
	236	+	Author: &clients.User{},
	237	+	State: "APPROVED",
219	238		},
	239	+	},
	240	+	},
	241	+	{
	242	+	ReviewPlatform: checker.ReviewPlatformGitHub,
	243	+	RevisionID: "3",
	244	+	Commits: []clients.Commit{commitC},
	245	+	Reviews: []clients.Review{
220	246		{
221		-	SHA: "baz",
222		-	AssociatedMergeRequest: clients.PullRequest{Number: 2},
	247	+	Author: &clients.User{},
	248	+	State: "APPROVED",
223	249		},
224	250		},
225	251		},
226	252		},
227	253		},
228	254		{
229		-	name: "some commits from external scm",
230		-	commits: []clients.Commit{
231		-	{
232		-	Message: "\nDifferential Revision: 123\nReviewed By: user-123",
233		-	SHA: "abc",
234		-	},
	255	+	name: "phabricator: merge with squash",
	256	+	commits: []clients.Commit{phabricatorCommitA, phabricatorCommitB, phabricatorCommitC},
	257	+	expected: []checker.Changeset{
235	258		{
236		-	Message: "\nDifferential Revision: 158\nReviewed By: user-456",
237		-	SHA: "def",
	259	+	RevisionID: "123",
	260	+	ReviewPlatform: checker.ReviewPlatformPhabricator,
	261	+	Commits: []clients.Commit{phabricatorCommitA},
238	262		},
239	263		{
240		-	Message: "this one from github, but unrelated to prev",
241		-	AssociatedMergeRequest: clients.PullRequest{Number: 158, MergedAt: time.Now()},
242		-	SHA: "fab",
	264	+	RevisionID: "158",
	265	+	ReviewPlatform: checker.ReviewPlatformPhabricator,
	266	+	Commits: []clients.Commit{phabricatorCommitB},
243	267		},
244	268		{
245		-	Message: "another from gh",
246		-	AssociatedMergeRequest: clients.PullRequest{Number: 158, MergedAt: time.Now()},
247		-	SHA: "dab",
	269	+	RevisionID: "2000",
	270	+	ReviewPlatform: checker.ReviewPlatformPhabricator,
	271	+	Commits: []clients.Commit{phabricatorCommitC},
248	272		},
249	273		},
	274	+	},
	275	+	{
	276	+	name: "phabricator: merge without squash",
	277	+	commits: []clients.Commit{phabricatorCommitA, phabricatorCommitAUnsquashed, phabricatorCommitAUnsquashed2},
250	278		expected: []checker.Changeset{
251	279		{
252		-	ReviewPlatform: checker.ReviewPlatformPhabricator,
253	280		RevisionID: "123",
254		-	Commits: []clients.Commit{{SHA: "abc"}},
255		-	},
256		-	{
257	281		ReviewPlatform: checker.ReviewPlatformPhabricator,
258		-	RevisionID: "158",
259		-	Commits: []clients.Commit{{SHA: "def"}},
260		-	},
261		-	{
262		-	ReviewPlatform: checker.ReviewPlatformGitHub,
263		-	RevisionID: "158",
264		-	Commits: []clients.Commit{
265		-	{SHA: "fab"},
266		-	{SHA: "dab"},
267		-	},
	282	+	Commits: []clients.Commit{phabricatorCommitA, phabricatorCommitAUnsquashed, phabricatorCommitAUnsquashed2},
268	283		},
269	284		},
270	285		},
271	286		{
272		-	name: "some commits from external scm with no revision id",
273		-	commits: []clients.Commit{
274		-	{
275		-	Message: "first change\nReviewed-on: server.url \nReviewed-by:user-123",
276		-	SHA: "abc",
277		-	},
278		-	{
279		-	Message: "followup\nReviewed-on: server.url \nReviewed-by:user-123",
280		-	SHA: "def",
281		-	},
282		-	{
283		-	Message: "commit thru gh",
284		-	AssociatedMergeRequest: clients.PullRequest{Number: 3, MergedAt: time.Now()},
285		-	SHA: "fab",
286		-	},
287		-	},
	287	+	name: "gerrit: merge with squash",
	288	+	commits: []clients.Commit{gerritCommitB, gerritCommitA},
288	289		expected: []checker.Changeset{
289	290		{
290	291		ReviewPlatform: checker.ReviewPlatformGerrit,
291	292		RevisionID: "abc",
292		-	Commits: []clients.Commit{{SHA: "abc"}},
	293	+	Commits: []clients.Commit{gerritCommitB},
293	294		},
294	295		{
295	296		ReviewPlatform: checker.ReviewPlatformGerrit,
296	297		RevisionID: "def",
297		-	Commits: []clients.Commit{{SHA: "def"}},
298		-	},
299		-	{
300		-	ReviewPlatform: checker.ReviewPlatformGitHub,
301		-	RevisionID: "3",
302		-	Commits: []clients.Commit{{SHA: "fab"}},
	298	+	Commits: []clients.Commit{gerritCommitA},
303	299		},
304	300		},
305	301		},
306	302		{
307		-	name: "external scm mirrored to github",
308		-	commits: []clients.Commit{
309		-	{
310		-	Message: "\nDifferential Revision: 123\nReviewed By: user-123",
311		-	SHA: "abc",
312		-	},
313		-	{
314		-	Message: "\nDifferential Revision: 158\nReviewed By: user-123",
315		-	SHA: "def",
316		-	},
317		-	{
318		-	Message: "\nDifferential Revision: 2000\nReviewed By: user-456",
319		-	SHA: "fab",
320		-	},
321		-	},
	303	+	name: "mixed: phabricator + gh",
	304	+	commits: []clients.Commit{phabricatorCommitA, phabricatorCommitD, commitB, commitBUnsquashed},
322	305		expected: []checker.Changeset{
323	306		{
324		-	RevisionID: "123",
325	307		ReviewPlatform: checker.ReviewPlatformPhabricator,
326		-	Commits: []clients.Commit{{SHA: "abc"}},
	308	+	RevisionID: "123",
	309	+	Commits: []clients.Commit{phabricatorCommitA},
327	310		},
328	311		{
329		-	RevisionID: "158",
330	312		ReviewPlatform: checker.ReviewPlatformPhabricator,
331		-	Commits: []clients.Commit{{SHA: "def"}},
	313	+	RevisionID: "2",
	314	+	Commits: []clients.Commit{phabricatorCommitD},
332	315		},
333	316		{
334		-	RevisionID: "2000",
335		-	ReviewPlatform: checker.ReviewPlatformPhabricator,
336		-	Commits: []clients.Commit{{SHA: "fab"}},
	317	+	ReviewPlatform: checker.ReviewPlatformGitHub,
	318	+	RevisionID: "2",
	319	+	Commits: []clients.Commit{commitB, commitBUnsquashed},
	320	+	Reviews: []clients.Review{{
	321	+	Author: &clients.User{},
	322	+	State: "APPROVED",
	323	+	}},
337	324		},
338	325		},
339	326		},
340	327		{
341		-	name: "external scm no squash",
342		-	commits: []clients.Commit{
343		-	{
344		-	Message: "\nDifferential Revision: 123\nReviewed By: user-123",
345		-	SHA: "abc",
346		-	},
347		-	{
348		-	Message: "\nDifferential Revision: 123\nReviewed By: user-123",
349		-	SHA: "def",
350		-	},
351		-	{
352		-	Message: "\nDifferential Revision: 123\nReviewed By: user-456",
353		-	SHA: "fab",
354		-	},
355		-	},
	328	+	name: "mixed: gerrit + gh",
	329	+	commits: []clients.Commit{gerritCommitB, gerritCommitA, commitC},
356	330		expected: []checker.Changeset{
357	331		{
358		-	RevisionID: "123",
359		-	ReviewPlatform: checker.ReviewPlatformPhabricator,
360		-	Commits: []clients.Commit{{SHA: "abc"}, {SHA: "def"}, {SHA: "fab"}},
	332	+	ReviewPlatform: checker.ReviewPlatformGerrit,
	333	+	RevisionID: "abc",
	334	+	Commits: []clients.Commit{gerritCommitB},
361	335		},
362		-	},
363		-	},
364		-	{
365		-	name: "single changeset",
366		-	commits: []clients.Commit{
367	336		{
368		-	SHA: "abc",
369		-	AssociatedMergeRequest: clients.PullRequest{Number: 1, MergedAt: time.Now()},
	337	+	ReviewPlatform: checker.ReviewPlatformGerrit,
	338	+	RevisionID: "def",
	339	+	Commits: []clients.Commit{gerritCommitA},
370	340		},
371		-	},
372		-	expected: []checker.Changeset{
373	341		{
374	342		ReviewPlatform: checker.ReviewPlatformGitHub,
375		-	RevisionID: "1",
376		-	Commits: []clients.Commit{{SHA: "abc"}},
	343	+	RevisionID: "3",
	344	+	Commits: []clients.Commit{commitC},
	345	+	Reviews: []clients.Review{
	346	+	{
	347	+	Author: &clients.User{},
	348	+	State: "APPROVED",
	349	+	},
	350	+	},
377	351		},
378	352		},
379	353		},
		skipped 2 lines
382	356		for _, tt := range tests {
383	357		t.Logf("test: %s", tt.name)
384	358		changesets := getChangesets(tt.commits)
385		-	assertChangesetArrEq(t, changesets, tt.expected)
	359	+	if !cmp.Equal(tt.expected, changesets,
	360	+	cmpopts.SortSlices(func(x, y checker.Changeset) bool {
	361	+	return x.RevisionID < y.RevisionID
	362	+	}),
	363	+	cmpopts.SortSlices(func(x, y clients.Commit) bool {
	364	+	return x.SHA < y.SHA
	365	+	})) {
	366	+	t.Log(cmp.Diff(tt.expected, changesets))
	367	+	t.Fail()
	368	+	}
386	369		}
387	370		}
388	371

Remove unused code from changeset creation (#2776)