Skip to content

Fix git commit committer parsing and add some tests #35007

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Jul 10, 2025
Merged

Fix git commit committer parsing and add some tests #35007

merged 14 commits into from
Jul 10, 2025

Conversation

lunny
Copy link
Member

@lunny lunny commented Jul 9, 2025 •
edited by wxiaoguang
Loading

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 9, 2025
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Jul 9, 2025
@lunny lunny requested a review from wxiaoguang July 9, 2025 01:50
@lunny lunny mentioned this pull request Jul 9, 2025
Closed
@wxiaoguang

This comment was marked as resolved.

Sign in to view
@lunny

This comment was marked as resolved.

Sign in to view
@lunny lunny added type/bug backport/v1.24 This PR should be backported to Gitea 1.24 labels Jul 9, 2025
@wxiaoguang

This comment was marked as resolved.

Sign in to view
@lunny

This comment was marked as resolved.

Sign in to view
@wxiaoguang

This comment was marked as resolved.

Sign in to view
@lunny

This comment was marked as resolved.

Sign in to view
@wxiaoguang wxiaoguang changed the title Revert change in commit list table about User ID and adding some tests Fix git commit committer parsing and adding some tests Jul 9, 2025
@wxiaoguang wxiaoguang removed the backport/v1.24 This PR should be backported to Gitea 1.24 label Jul 9, 2025
@wxiaoguang wxiaoguang force-pushed the lunny/fix_bug_commits_list2 branch from 9a39900 to 53146a8 Compare July 9, 2025 05:28
wxiaoguang
wxiaoguang reviewed Jul 9, 2025
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done from my side

@wxiaoguang wxiaoguang changed the title Fix git commit committer parsing and adding some tests Fix git commit committer parsing and add some tests Jul 9, 2025
@wxiaoguang wxiaoguang mentioned this pull request Jul 9, 2025
Merged
lafriks
lafriks reviewed Jul 9, 2025
View reviewed changes
@silverwind
Copy link
Member

silverwind commented Jul 9, 2025 •
edited
Loading

I tested this branch, it seems to fully both issues from #34991. We should definitely backport this to 1.24.

@silverwind silverwind added the backport/v1.24 This PR should be backported to Gitea 1.24 label Jul 9, 2025
silverwind
silverwind reviewed Jul 9, 2025
View reviewed changes
silverwind
silverwind approved these changes Jul 9, 2025
View reviewed changes
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT from functional perspective.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 9, 2025
@wxiaoguang wxiaoguang removed the backport/v1.24 This PR should be backported to Gitea 1.24 label Jul 9, 2025
@lunny lunny added this to the 1.25.0 milestone Jul 9, 2025
@wxiaoguang wxiaoguang requested review from lafriks and delvh July 10, 2025 05:08
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 10, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 10, 2025
@silverwind
Copy link
Member

silverwind commented Jul 10, 2025 •
edited
Loading

The more serious issue is the missing email-to-user association (for which I have no steps to reproduce, but it happens everywhere in my instance)

Have you check your "noreply address"? That fix is simple, just models/user/user.go: 53146a8#diff-dacacda4403790aa4e9b4473c62b3af81b9c0a05d795dd31bd936866e26cfb13

Hmm no that can't be it. My NoReplyAddress resolves to like noreply.git.foo.com but my email address from the commit is on a different TLD entirely, like [email protected]. My user currently has multiple email addresses configured, maybe that is part of the problem.

Actually I found the issue. In configuration I had service.NO_REPLY_ADDRESS=bar.com which is incorrect and should have been noreply.bar.com. Changing it to that value fixed the committer association.

@silverwind silverwind mentioned this pull request Jul 10, 2025
Closed
@wxiaoguang
Copy link
Contributor

The more serious issue is the missing email-to-user association (for which I have no steps to reproduce, but it happens everywhere in my instance)

Have you check your "noreply address"? That fix is simple, just models/user/user.go: 53146a8#diff-dacacda4403790aa4e9b4473c62b3af81b9c0a05d795dd31bd936866e26cfb13

Hmm no that can't be it. My NoReplyAddress resolves to like noreply.git.foo.com but my email address from the commit is on a different TLD entirely, like [email protected]. My user currently has multiple email addresses configured, maybe that is part of the problem.

Actually I found the issue. In configuration I had service.NO_REPLY_ADDRESS=bar.com which is incorrect and should have been noreply.bar.com. Changing it to that value fixed the committer association.

Yep, this is my fourth time to fix the regressions from #33528, so I am pretty sure it is the cause.

I think I also know why your "noreply address" is incorrect, I guess in old days, Gitea used the DOMAIN as noreply address, then it caused problem, then the behavior was changed (a prefix as added). BUT, in old Gitea, Gitea also had another wrong behavior: it wrote garbage config options into the config file (I also fixed that), so very old users might be affected by incorrect config options.

@silverwind
Copy link
Member

Yeah it's possible that the incorrect value came from that incorrect config rewrite. In any case, I'm glad we figured it out.

@silverwind
Copy link
Member

@wxiaoguang is this good to merge from your side?

@wxiaoguang
Copy link
Contributor

The code written by me surely looks good to me. So I asked other maintainers to review.

@wxiaoguang wxiaoguang enabled auto-merge (squash) July 10, 2025 16:45
@wxiaoguang wxiaoguang disabled auto-merge July 10, 2025 18:04
@wxiaoguang wxiaoguang enabled auto-merge (squash) July 10, 2025 18:21
@wxiaoguang wxiaoguang merged commit 7a15334 into go-gitea:main Jul 10, 2025
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 10, 2025
@lunny lunny deleted the lunny/fix_bug_commits_list2 branch July 10, 2025 19:04
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 11, 2025
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
a9ed1ec 
* giteaofficial/main:
  Fix updating user visibility (go-gitea#35036)
  Fix git commit committer parsing and add some tests (go-gitea#35007)
  Refactor OpenIDConnect to support SSH/FullName sync (go-gitea#34978)
  Support base64-encoded agit push options (go-gitea#35037)
  Also display "recently pushed branch" alert on PR view (go-gitea#35001)
  Make submodule link work with relative path (go-gitea#35034)
  Update to go 1.24.5 (go-gitea#35031)
  Improve CLI commands (go-gitea#34973)
  Tweak eslint config, fix new issues (go-gitea#35019)

# Conflicts:
#	templates/repo/commits_list.tmpl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@silverwind silverwind silverwind approved these changes

@hiifong hiifong hiifong approved these changes

@lafriks lafriks Awaiting requested review from lafriks

@delvh delvh Awaiting requested review from delvh

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code modifies/internal modifies/templates This PR modifies the template files type/bug
Projects
None yet
Milestone
1.25.0
Development

Successfully merging this pull request may close these issues.

Commit list author rendering regression in v1.24+ Avatars not visible in commit view when no-reply address is configured to the real domain
6 participants
@lunny @wxiaoguang @silverwind @lafriks @hiifong @GiteaBot