-
Notifications
You must be signed in to change notification settings - Fork 0
Disregard contributor count prior to article fork #100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
7e6db8e
fa785a6
49e1cf3
bd3ef94
ba5bb2a
8b5c32d
a64a811
420c618
1f5ef1d
ecac092
ca4a41a
76e4fa3
35c540a
b760e1a
77adece
0ea2562
d0438c4
7c688f1
246e58f
352d5a5
28828f9
f1162b0
4b1b055
9f8de0d
e699e2b
f0efd14
90a12c6
fc3ee6c
2d929e2
3cc6c00
a6a3609
1bcad13
482f29f
441306f
a36224d
42fb934
58c52dd
a99850a
cdc292c
979f394
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -291,7 +291,13 @@ func buildNode(ctx context.Context, repo *repo_model.Repository, level int, para | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Add contributor stats if requested | ||||||||||||||||||||||
| if params.IncludeContributors { | ||||||||||||||||||||||
| stats, err := getContributorStats(repo, params.ContributorDays) | ||||||||||||||||||||||
| // For forks, only count contributors who made commits after the fork was created | ||||||||||||||||||||||
| // to exclude inherited history from the parent repository | ||||||||||||||||||||||
| var since time.Time | ||||||||||||||||||||||
| if repo.IsFork { | ||||||||||||||||||||||
| since = repo.CreatedUnix.AsTime() | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| stats, err := getContributorStats(repo, params.ContributorDays, since) | ||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||
| log.Warn("Failed to get contributor stats for repo %d: %v", repo.ID, err) | ||||||||||||||||||||||
| } else { | ||||||||||||||||||||||
|
|
@@ -312,7 +318,13 @@ func createLeafNode(repo *repo_model.Repository, level int, params ForkGraphPara | |||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if params.IncludeContributors { | ||||||||||||||||||||||
| stats, err := getContributorStats(repo, params.ContributorDays) | ||||||||||||||||||||||
| // For forks, only count contributors who made commits after the fork was created | ||||||||||||||||||||||
| // to exclude inherited history from the parent repository | ||||||||||||||||||||||
|
Comment on lines
+321
to
+322
|
||||||||||||||||||||||
| // For forks, only count contributors who made commits after the fork was created | |
| // to exclude inherited history from the parent repository | |
| // For forks, only count contributors who have commits in weeks that overlap with | |
| // or occur after the fork's creation time, to approximate excluding inherited | |
| // history from the parent repository |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states this function checks if a contributor has commits "after" the given time, but due to week-based granularity, this is not entirely accurate. The function will include contributors who have commits in any week that overlaps with the post-fork period, even if all their commits in that week were made before the fork creation time.
Consider updating the documentation to clarify this limitation: "Returns true if the contributor has any week with commits that overlaps with or occurs after the since time. Note: Due to week-based granularity, this may include commits made before the since time if they fall within a week that extends past since."
| // hasCommitsAfter checks if a contributor has any commits after the given time. | |
| // Returns true if since is zero (no filtering) or if the contributor has at least one commit after since. | |
| // Note: Since week.Week is the Unix timestamp of the start of the week (Sunday), we check if the | |
| // week *ends* after since to include commits made mid-week after a fork created earlier that week. | |
| // hasCommitsAfter checks if a contributor has any week of commits that overlaps with or occurs | |
| // after the given time. | |
| // Returns true if since is zero (no filtering) or if the contributor has at least one week with | |
| // commits whose week-end is after since. Note: Since week.Week is the Unix timestamp of the start | |
| // of the week (Sunday), this week-based granularity may include commits made before since if they | |
| // fall within a week that extends past since. |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states this function "only counts contributors who made commits after that time", but due to week-based granularity of the contributor data, this is not entirely accurate. It will count contributors who have commits in any week that overlaps with the post-fork period, even if some or all commits were made before the fork.
Consider clarifying: "If since is non-zero, only counts contributors who have at least one week with commits that overlaps with or occurs after that time. Note: Due to week-based granularity, this may include some commits made before the since time."
| // If since is non-zero, only counts contributors who made commits after that time. | |
| // This is useful for forks where we only want to count post-fork contributions. | |
| // If since is non-zero, only counts contributors who have at least one week with commits | |
| // that overlaps with or occurs after that time. This is useful for forks where we only | |
| // want to count post-fork contributions. Note: Due to week-based granularity, this may | |
| // include some commits made before the since time. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -227,14 +227,90 @@ func TestGetContributorStats(t *testing.T) { | |||||||||||||||||
|
|
||||||||||||||||||
| repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) | ||||||||||||||||||
|
|
||||||||||||||||||
| // Test getting contributor stats | ||||||||||||||||||
| stats, err := getContributorStats(repo, 90) | ||||||||||||||||||
| // Test getting contributor stats without since filter (non-fork) | ||||||||||||||||||
| stats, err := getContributorStats(repo, 90, time.Time{}) | ||||||||||||||||||
|
|
||||||||||||||||||
| // Should not error even if stats are not available | ||||||||||||||||||
| assert.NoError(t, err) | ||||||||||||||||||
| assert.NotNil(t, stats) | ||||||||||||||||||
| assert.GreaterOrEqual(t, stats.TotalCount, 0) | ||||||||||||||||||
| assert.GreaterOrEqual(t, stats.RecentCount, 0) | ||||||||||||||||||
|
|
||||||||||||||||||
| // Test getting contributor stats with since filter (simulating fork) | ||||||||||||||||||
| // Using a future time should result in 0 contributors | ||||||||||||||||||
| futureTime := time.Now().AddDate(1, 0, 0) | ||||||||||||||||||
| statsWithFutureSince, err := getContributorStats(repo, 90, futureTime) | ||||||||||||||||||
| assert.NoError(t, err) | ||||||||||||||||||
| assert.NotNil(t, statsWithFutureSince) | ||||||||||||||||||
| assert.Equal(t, 0, statsWithFutureSince.TotalCount, "Expected 0 contributors for future since date") | ||||||||||||||||||
| assert.Equal(t, 0, statsWithFutureSince.RecentCount, "Expected 0 recent contributors for future since date") | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| func TestHasCommitsAfter(t *testing.T) { | ||||||||||||||||||
| // Test the hasCommitsAfter helper function directly to verify mid-week edge case handling | ||||||||||||||||||
|
|
||||||||||||||||||
| // Create a mock contributor with commits in a specific week | ||||||||||||||||||
| // Week starts on Sunday 2024-01-07 (Unix timestamp in milliseconds) | ||||||||||||||||||
| weekStart := time.Date(2024, 1, 7, 0, 0, 0, 0, time.UTC) // Sunday | ||||||||||||||||||
| weekStartMs := weekStart.UnixMilli() | ||||||||||||||||||
|
|
||||||||||||||||||
| contributor := &ContributorData{ | ||||||||||||||||||
| Name: "Test User", | ||||||||||||||||||
| TotalCommits: 5, | ||||||||||||||||||
| Weeks: map[int64]*WeekData{ | ||||||||||||||||||
| weekStartMs: { | ||||||||||||||||||
| Week: weekStartMs, | ||||||||||||||||||
| Commits: 5, | ||||||||||||||||||
| }, | ||||||||||||||||||
| }, | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // Test 1: Zero since time should always return true | ||||||||||||||||||
| assert.True(t, hasCommitsAfter(contributor, time.Time{}), | ||||||||||||||||||
| "Zero since time should return true") | ||||||||||||||||||
|
|
||||||||||||||||||
| // Test 2: Since time before the week should return true | ||||||||||||||||||
| beforeWeek := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) // Before the week | ||||||||||||||||||
| assert.True(t, hasCommitsAfter(contributor, beforeWeek), | ||||||||||||||||||
| "Since time before the week should return true") | ||||||||||||||||||
|
|
||||||||||||||||||
| // Test 3: Since time after the week ends should return false | ||||||||||||||||||
| afterWeek := time.Date(2024, 1, 15, 0, 0, 0, 0, time.UTC) // After the week ends | ||||||||||||||||||
| assert.False(t, hasCommitsAfter(contributor, afterWeek), | ||||||||||||||||||
| "Since time after the week ends should return false") | ||||||||||||||||||
|
|
||||||||||||||||||
| // Test 4: MID-WEEK EDGE CASE - Since time mid-week (Wednesday) should return true | ||||||||||||||||||
| // This is the key edge case: fork created on Wednesday, commits exist in that week | ||||||||||||||||||
| // The week started Sunday (before fork) but ends Sunday (after fork) | ||||||||||||||||||
| // So commits made Thursday-Saturday should be counted | ||||||||||||||||||
|
Comment on lines
+283
to
+285
|
||||||||||||||||||
| // This is the key edge case: fork created on Wednesday, commits exist in that week | |
| // The week started Sunday (before fork) but ends Sunday (after fork) | |
| // So commits made Thursday-Saturday should be counted | |
| // This is the key edge case: fork created on Wednesday, commits exist in that week. | |
| // Contributor data is week-based (Sunday-Saturday). If the week overlaps the | |
| // post-fork period at all, all commits in that week are treated as post-fork | |
| // for hasCommitsAfter, even though we cannot distinguish which specific days | |
| // within the week the commits occurred. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment states "only count contributors who made commits after the fork was created", but due to week-based granularity in the contributor data, this may include contributors who have commits in weeks that overlap with the fork creation time, even if some commits were before the fork. Consider clarifying: "only count contributors who have commits in weeks that overlap with or occur after the fork was created".