Respect Co-authored-by trailers in the contributors graph #30925

kemzeb · 2024-05-09T20:11:58Z

Resolves #29183.

Added an automated test (and refactored the test code around it) and also manually tested the Contributors, Commit Frequency, and Recent Commits pages in a few small repos and the output appears to be fine.

kemzeb · 2024-05-09T20:18:20Z

Now that I am looking at into it further, I think Pulse may need to account for co-authors too. I think this function is the one that needs updating

kemzeb · 2024-05-09T21:17:05Z

services/repository/contributors_graph.go

+	}
+
+	email = strings.TrimRight(parts[1], ">")
+	if _, err := mail.ParseAddress(email); err != nil {


Since users can add anything between the <> brackets I believed that we could try to throw away lines that contain invalid email addresses.

Though while testing by creating blank repos and merging the commit history of existing projects into these repos I observed that some emails associated to bots will be rejected (e.g. the pre-commit bot on GitHub). I guess this happens because these emails don't follow RFC 5322. Not sure if I should do this validation check.

Depends on how we parse other trailer lines I suppose. I would make it consistent with those.

kemzeb · 2024-05-09T21:36:18Z

Hmm I'm not sure how common it is, but should I account for duplicate Co-authored-by entries i.e. entries that have the same email in a single commit?

silverwind · 2024-05-10T11:19:23Z

services/repository/contributors_graph.go

@@ -125,8 +127,7 @@ func getExtendedCommitStats(repo *git.Repository, revision string /*, limit int
 		_ = stdoutWriter.Close()
 	}()

-	gitCmd := git.NewCommand(repo.Ctx, "log", "--shortstat", "--no-merges", "--pretty=format:---%n%aN%n%aE%n%as", "--reverse")
-	// AddOptionFormat("--max-count=%d", limit)
+	gitCmd := git.NewCommand(repo.Ctx, "log", "--shortstat", "--no-merges", "--pretty=format:---%n%aN%n%aE%n%as%n%(trailers:key=Co-authored-by,valueonly=true)", "--reverse")


I checked compat on this trailers option and it does appear to be present in git 2.27 at least which is the oldest version where git's site has docs for, so it's likely fine to use:

https://git-scm.com/docs/git-log/2.27.0

kemzeb · 2024-05-11T23:08:35Z

Recent change made the co-author parsing logic more robust to inputs where the co-author's name may include a '<'.

I manually tested all the code paths for parseCoAuthorTrailerValue() and they appear fine. Even though it is unexported, should I just include unit tests for it since the other test I added doesn't cover all its paths?

silverwind · 2024-05-13T21:05:37Z

Hmm I'm not sure how common it is, but should I account for duplicate Co-authored-by entries i.e. entries that have the same email in a single commit?

I guess duplicates (by email) should count as one.

silverwind · 2024-05-13T21:08:42Z

Recent change made the co-author parsing logic more robust to inputs where the co-author's name may include a '<'.

I manually tested all the code paths for parseCoAuthorTrailerValue() and they appear fine. Even though it is unexported, should I just include unit tests for it since the other test I added doesn't cover all its paths?

I'm reminded of a recent vulnerability GitHub had in their commit message parser surrounding these meta lines. Hope we don't replicate it, maybe you can double-check and add a test.

silverwind · 2024-05-13T21:10:49Z

BTW is it feasible to move all commit message parsing into a shared function? As I see it, parsing these Co-Authored-By lines is not specific to Commit graph and could be useful elsewhere too.

kemzeb · 2024-05-14T06:23:30Z

Recent changes exports the trailer parsing logic and adds adds parameterized tests for it. We now also handle duplicated Co-authored-by entries (i.e. entries that have the same email).

kemzeb · 2024-05-14T07:04:56Z

I'm reminded of a recent vulnerability GitHub had in their commit message parser surrounding these meta lines. Hope we don't replicate it, maybe you can double-check and add a test.

Nice read! It appears it is caused by their parser's use of regex, where it iterates a commit's metadata lines looking for its first match with a line that starts with author. From what I understand the main problem is that you can author a commit without a name (while working in a Codespaces environment) where the parser would mistakenly not catch; it would then continue iterating over the commit metadata and eventually find a false author line that was added by the attacker.

I have included some tests for my parser and I'm open to include more possible inputs.

lunny · 2024-10-09T18:17:50Z

services/repository/contributors_graph.go

+					}
+					coAuthorName, coAuthorEmail, err := util.ParseCommitTrailerValueWithAuthor(line)
+					if err != nil {
+						continue


It's better to have a log here?

I'm not sure how frequent it is for commits to have invalid Co-authored-by trailers but if it won't cause a problem for admins we could have this as maybe an INFO/DEBUG level log

lunny · 2024-10-09T18:21:20Z

modules/util/commit_trailers.go

+// Note that it only parses the value and does not consider the trailer key i.e. we just
+// parse something like the following:
+//
+// Foo Bar <foobar@example.com>


Looks like it's a standard format, why not use mail.ParseAddress directly?

I think I originally was worried about using this since I wasn't too sure of what mail.ParseAddress accepted (i.e. wasn't familiar with RFC 5322) as input but looking at it now it seems to be a good solution compared to having us parsing the input manually.

It may still make sense to have a utility function around since it looks like we can accept a trailer with no name e.g.:

addrStr := "<hello@world>" addr, _ := mail.ParseAddress(addrStr) fmt.Printf("%s", addr.Address) // hello@world // Program exited.

Here we can throw an error if addr.Name is empty.

kemzeb · 2024-10-09T18:52:14Z

services/repository/contributors_graph.go

+					if err != nil {
+						continue
+					}
+					if _, exists := emailSet[coAuthorEmail]; exists {


We have a hash map here to account trailers with duplicate emails. Is it safe that this hash map is unbounded? If so, should we just ignore all the co-author data for this commit if they reach some certain bound?

kemzeb · 2024-10-09T18:54:25Z

I realized I need to account for the case where when we are iterating over the trailers we encounter co-author email = commit author email. The current implementation would count this as 2 commits for the commit author. Will make the necessary changes for this and address thte PR feedback as soon as I get the time.

kemzeb · 2024-10-27T02:20:33Z

Will need to do the following when I get the time:

Revert and reimplement changes to user2/repo2 to avoid adding too many git objects (and because I accidentally changed a commit that was already there)
Move the authorEmail == coAuthorEmail below the error check

Respect Co-authored-by trailers in the contributors graph

ea40999

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 9, 2024

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 9, 2024

github-actions bot added the modifies/go Pull requests that update Go code label May 9, 2024

kemzeb added 2 commits May 9, 2024 13:52

CI feedback

99953fa

Fix lint problem in test code

9ac9327

kemzeb commented May 9, 2024

View reviewed changes

lunny added this to the 1.23.0 milestone May 10, 2024

silverwind reviewed May 10, 2024

View reviewed changes

Have co-author parsing be more robust and some refactoring

cce7812

Shrink openEmailBracketIdx -> openBracketIdx

25a9f12

kemzeb added 2 commits May 13, 2024 22:35

Make commit trailer parsing public and add parameterized tests

8e3209b

Handle duplicated Co-authored-by entries

fd0bf3b

pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 14, 2024

kemzeb added 2 commits May 13, 2024 23:15

Add missing license comment

6c713da

Add a space between license comment and package keyword

78fecfc

lunny reviewed Oct 9, 2024

View reviewed changes

kemzeb commented Oct 9, 2024

View reviewed changes

lunny removed this from the 1.23.0 milestone Oct 13, 2024

lunny added this to the 1.24.0 milestone Oct 13, 2024

kemzeb and others added 4 commits October 25, 2024 21:48

Merge branch 'main' into respect-co-authors-in-contributors-graph

17205fa

Parse commit trailers using mail.ParseAddress()

22d8652

Add check for co-author email = author email

b880209

Remove error object that I no longer use

f2e114e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect Co-authored-by trailers in the contributors graph #30925

Respect Co-authored-by trailers in the contributors graph #30925

kemzeb commented May 9, 2024

kemzeb commented May 9, 2024

kemzeb May 9, 2024

silverwind May 13, 2024

kemzeb commented May 9, 2024

silverwind May 10, 2024 •

edited

Loading

kemzeb commented May 11, 2024

silverwind commented May 13, 2024

silverwind commented May 13, 2024 •

edited

Loading

silverwind commented May 13, 2024

kemzeb commented May 14, 2024

kemzeb commented May 14, 2024

lunny Oct 9, 2024

kemzeb Oct 9, 2024

lunny Oct 9, 2024

kemzeb Oct 9, 2024

kemzeb Oct 9, 2024

kemzeb commented Oct 9, 2024

kemzeb commented Oct 27, 2024

Respect Co-authored-by trailers in the contributors graph #30925

Are you sure you want to change the base?

Respect Co-authored-by trailers in the contributors graph #30925

Conversation

kemzeb commented May 9, 2024

kemzeb commented May 9, 2024

kemzeb May 9, 2024

Choose a reason for hiding this comment

silverwind May 13, 2024

Choose a reason for hiding this comment

kemzeb commented May 9, 2024

silverwind May 10, 2024 • edited Loading

Choose a reason for hiding this comment

kemzeb commented May 11, 2024

silverwind commented May 13, 2024

silverwind commented May 13, 2024 • edited Loading

silverwind commented May 13, 2024

kemzeb commented May 14, 2024

kemzeb commented May 14, 2024

lunny Oct 9, 2024

Choose a reason for hiding this comment

kemzeb Oct 9, 2024

Choose a reason for hiding this comment

lunny Oct 9, 2024

Choose a reason for hiding this comment

kemzeb Oct 9, 2024

Choose a reason for hiding this comment

kemzeb Oct 9, 2024

Choose a reason for hiding this comment

kemzeb commented Oct 9, 2024

kemzeb commented Oct 27, 2024

silverwind May 10, 2024 •

edited

Loading

silverwind commented May 13, 2024 •

edited

Loading