Skip to content
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

Use the headline comment of pull-request as the squash commit's message #13071

Merged
merged 6 commits into from
Dec 21, 2020

Conversation

typeless
Copy link
Contributor

@typeless typeless commented Oct 8, 2020

Originally, it was filled by the commit messages of the involved
commits. In this change, we use the headline comment of the pull
request as the commit message when it is a squash merge.

Thanks to @zeripath for suggesting the idea.

Fixes #12365

@lunny lunny added the type/enhancement An improvement of existing functionality label Oct 8, 2020
@typeless typeless changed the title Use the text of pull-request as the squash commit's message WIP: Use the text of pull-request as the squash commit's message Oct 8, 2020
@typeless
Copy link
Contributor Author

typeless commented Oct 8, 2020

This is not quite correct yet. Because the texts provided by GetCommitMessagtes include the Co-authored lines which are still wanted.

Fixed now. It's manually tested.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 8, 2020
@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #13071 (7ce4543) into master (29d12cf) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #13071   +/-   ##
=======================================
  Coverage   42.32%   42.33%           
=======================================
  Files         726      726           
  Lines       77689    77682    -7     
=======================================
  Hits        32885    32885           
+ Misses      39406    39404    -2     
+ Partials     5398     5393    -5     
Impacted Files Coverage Δ
routers/repo/pull.go 31.78% <100.00%> (ø)
services/pull/pull.go 42.85% <100.00%> (+0.98%) ⬆️
modules/indexer/stats/db.go 40.00% <0.00%> (-20.00%) ⬇️
modules/indexer/stats/queue.go 64.70% <0.00%> (-11.77%) ⬇️
modules/git/repo_commit_nogogit.go 63.33% <0.00%> (-1.67%) ⬇️
models/error.go 38.49% <0.00%> (-0.49%) ⬇️
modules/git/blame.go 67.14% <0.00%> (+1.42%) ⬆️
modules/queue/unique_queue_disk_channel.go 55.38% <0.00%> (+1.53%) ⬆️
modules/git/command.go 89.42% <0.00%> (+1.92%) ⬆️
modules/git/tree_entry_nogogit.go 93.75% <0.00%> (+6.25%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a500cf...7ce4543. Read the comment docs.

@typeless typeless force-pushed the pr-squash-comment branch 2 times, most recently from d54c86e to b0a229e Compare October 8, 2020 03:20
@typeless typeless changed the title WIP: Use the text of pull-request as the squash commit's message Use the text of pull-request as the squash commit's message Oct 8, 2020
@typeless typeless changed the title Use the text of pull-request as the squash commit's message Use the headline comment of pull-request as the squash commit's message Oct 8, 2020
@lafriks
Copy link
Member

lafriks commented Oct 10, 2020

I'm ok of having first commit skipped but imho other commits could still be relevant...

@typeless
Copy link
Contributor Author

@lafriks What the pr does is not about skipping the first commit, but rather using the PR's title and text as the squashed commit's message as #12365 (comment) describes.

@lafriks
Copy link
Member

lafriks commented Oct 12, 2020

@typeless yes I know that... but this way it loses information from other commits that could contain needed information...

@typeless
Copy link
Contributor Author

@lafriks After some contemplation, I think the circumstances that a user would want to keep the titles of the individual commits, while choosing to do squash merges at the same time, should be relatively rare than otherwise. Of course, I can be wrong.

@zeripath
Copy link
Contributor

I think @typeless is correct here - realistically if you're performing a squash commit the issue description is far more likely to be a better description of the PR commit message than the commit messages of all the squashed commits.

I guess what we need is some sort of template reading thing.

@ffbh123456
Copy link

@typeless
Please post the picture, I want to see the effect of the current square merge
I suggest that gitea's square merge is exactly the same as github's square merge, because most people should use github first,so it would be better to be consistent with github

@typeless
Copy link
Contributor Author

typeless commented Dec 9, 2020

@ZhouJiaZhi
I am using a patched Gitea that has this PR applied, so if you want to see how the vanilla version works, you should probably look at https://try.gitea.io/.

圖片

@ffbh123456
Copy link

@typeless
This looks much better than before. Is the following information automatically generated? Will this information be recorded in the commit message after square merge? I remember that the square merge in github will only keep the title information in the commit message
截屏2020-12-09 12 56 34

@typeless
Copy link
Contributor Author

typeless commented Dec 9, 2020

@ZhouJiaZhi

. Is the following information automatically generated?

No, it is copied from the PR description.
Although it can be made more clever to automatically fill the description by default when the PR has only a single commit.

Will this information be recorded in the commit message after square merge?

All commit messages will be discarded.

@typeless typeless requested a review from 6543 December 9, 2020 05:06
@typeless
Copy link
Contributor Author

typeless commented Dec 9, 2020

@6543 I accidentally pressed the "request review" button. :/ No worries.

@ffbh123456
Copy link

@typeless
Good job, but it seems your code has conflicts
截屏2020-12-09 21 54 44

@lunny lunny added this to the 1.14.0 milestone Dec 9, 2020
Originally, it was filled by the commit messages of the involved
commits. In this change, we use the headline comment of the pull
request as the commit message when it is a squash merge.

Thanks to @zeripath for suggesting the idea.

Fixes go-gitea#12365
@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 Dec 11, 2020
@ffbh123456
Copy link

Can anyone help approve this PR

@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 Dec 19, 2020
@lafriks
Copy link
Member

lafriks commented Dec 19, 2020

Tests fail

@6543
Copy link
Member

6543 commented Dec 19, 2020

merge conflict relicts?

@typeless
Copy link
Contributor Author

--- FAIL: TestAPIGitTags (0.20s)
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:88:1() [I] Started GET /user/settings/applications for 
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:95:1() [I] Completed GET /user/settings/applications 0  in 4.738624ms
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:88:1() [I] Started POST /user/settings/applications for 
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:95:1() [I] Completed POST /user/settings/applications 0  in 20.162784ms
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:88:1() [I] Started GET /user/settings/applications for 
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:95:1() [I] Completed GET /user/settings/applications 0  in 4.595815ms
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:88:1() [I] Started GET /api/v1/repos/user2/repo1/git/tags/3431c83e7aa091f921788022dc7a8573fcf8e431?token=385fbbcdbeb5ecdf8e8a1bfd434dbcdb81d31089 for 
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:95:1() [I] Completed GET /api/v1/repos/user2/repo1/git/tags/3431c83e7aa091f921788022dc7a8573fcf8e431?token=385fbbcdbeb5ecdf8e8a1bfd434dbcdb81d31089 0  in 34.457315ms
    api_repo_git_tags_test.go:54: 
        	Error Trace:	api_repo_git_tags_test.go:54
        	Error:      	Not equal: 
        	            	expected: "user2"
        	            	actual  : "drone"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-user2
        	            	+drone
        	Test:       	TestAPIGitTags
    api_repo_git_tags_test.go:55: 
        	Error Trace:	api_repo_git_tags_test.go:55
        	Error:      	Not equal: 
        	            	expected: "user2@example.com"
        	            	actual  : "noreply@drone"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-user2@example.com
        	            	+noreply@drone
        	Test:       	TestAPIGitTags
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:88:1() [I] Started GET /api/v1/repos/user2/repo1/git/tags/65f1bf27bc3bf70f64657658635e66094edbcb4d?token=385fbbcdbeb5ecdf8e8a1bfd434dbcdb81d31089 for 

Hmmm, that's confusing.

@lunny
Copy link
Member

lunny commented Dec 21, 2020

--- FAIL: TestAPIGitTags (0.20s)
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:88:1() [I] Started GET /user/settings/applications for 
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:95:1() [I] Completed GET /user/settings/applications 0  in 4.738624ms
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:88:1() [I] Started POST /user/settings/applications for 
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:95:1() [I] Completed POST /user/settings/applications 0  in 20.162784ms
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:88:1() [I] Started GET /user/settings/applications for 
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:95:1() [I] Completed GET /user/settings/applications 0  in 4.595815ms
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:88:1() [I] Started GET /api/v1/repos/user2/repo1/git/tags/3431c83e7aa091f921788022dc7a8573fcf8e431?token=385fbbcdbeb5ecdf8e8a1bfd434dbcdb81d31089 for 
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:95:1() [I] Completed GET /api/v1/repos/user2/repo1/git/tags/3431c83e7aa091f921788022dc7a8573fcf8e431?token=385fbbcdbeb5ecdf8e8a1bfd434dbcdb81d31089 0  in 34.457315ms
    api_repo_git_tags_test.go:54: 
        	Error Trace:	api_repo_git_tags_test.go:54
        	Error:      	Not equal: 
        	            	expected: "user2"
        	            	actual  : "drone"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-user2
        	            	+drone
        	Test:       	TestAPIGitTags
    api_repo_git_tags_test.go:55: 
        	Error Trace:	api_repo_git_tags_test.go:55
        	Error:      	Not equal: 
        	            	expected: "user2@example.com"
        	            	actual  : "noreply@drone"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-user2@example.com
        	            	+noreply@drone
        	Test:       	TestAPIGitTags
    testlogger.go:78: 2020/12/19 10:18:32 ...outers/routes/chi.go:88:1() [I] Started GET /api/v1/repos/user2/repo1/git/tags/65f1bf27bc3bf70f64657658635e66094edbcb4d?token=385fbbcdbeb5ecdf8e8a1bfd434dbcdb81d31089 for 

Hmmm, that's confusing.

It's a drone configuration problem. I have update the PR.

@6543
Copy link
Member

6543 commented Dec 21, 2020

🚀

@6543 6543 merged commit 09304db into go-gitea:master Dec 21, 2020
@ffbh123456
Copy link

Great, I wait it for a long time, When will this commit be published to docker?

@6543
Copy link
Member

6543 commented Dec 22, 2020

it is already on :latest i think @ZhouJiaZhi

@go-gitea go-gitea locked and limited conversation to collaborators Feb 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate headlines in commit message after pull merge
8 participants