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

Add merge files files to GetCommitFileStatus #20515

Merged
merged 6 commits into from
Aug 24, 2023

Conversation

jasugun
Copy link
Contributor

@jasugun jasugun commented Jul 28, 2022

Hi,

We'd like to add merge files files to GetCommitFileStatus fucntions so API returns the list of all the files associated to a merged pull request commit, like GitHub API does.
The list of affectedFiles for an API commit is fetched from toCommit() function in routers/api/v1/repo/commits.go, and API was returning no file in case of a pull request with no conflict, or just files associated to the confict resolution, but NOT the full list of merged files.

This would lead to situations where a CI polling a repo for changes could miss some file changes due to API returning an empty / partial list in case of such merged pull requests. (Hope this makes sense :) )

NOTE: I'd like to add a unittest in integrations/api_repo_git_commits_test.go but failed to understand how to add my own test bare repo so I can make a test on a merged pull request commit to check for affectedFiles.
Is there a merged pull request in there that I could use maybe?
Could someone please direct me to the relevant ressources with informations on how to do that please?

Thanks for your time,
Laurent.

@jasugun jasugun force-pushed the dontnod_diffmerges_for_api_commits branch from ebbb28f to 3504f4c Compare July 28, 2022 12:56
@tdesveaux tdesveaux force-pushed the dontnod_diffmerges_for_api_commits branch from 3504f4c to dff160f Compare October 12, 2022 16:33
@lafriks lafriks added the type/enhancement An improvement of existing functionality label Oct 12, 2022
@tdesveaux tdesveaux force-pushed the dontnod_diffmerges_for_api_commits branch 2 times, most recently from bfb7560 to 917abde Compare October 13, 2022 07:28
@tdesveaux
Copy link
Contributor

Hey,
I took this over from Laurent.
I managed to add and run a test on this but I'm not sure how to update test data for unit-testing to work.

What I did locally was to add two commits to the user2/repo16 test data.
One which only commit a new-file-to-merge.txt empty file and a second merging it into master with git merge --no-ff to force a merge commit.

If someone could give any pointer on how to update this, I'd be grateful.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 13, 2022
@lafriks lafriks changed the title Add merge files files to GetCommitFileStatus fucntion Add merge files files to GetCommitFileStatus Oct 13, 2022
@tdesveaux tdesveaux force-pushed the dontnod_diffmerges_for_api_commits branch from 917abde to 78a823e Compare November 4, 2022 17:32
@lunny
Copy link
Member

lunny commented May 10, 2023

You can just checkout the bare repo and commit and push.

@tdesveaux tdesveaux force-pushed the dontnod_diffmerges_for_api_commits branch from 78a823e to 8ccc2f3 Compare May 12, 2023 10:17
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 12, 2023
Add new test repo repo6_merge
@tdesveaux tdesveaux force-pushed the dontnod_diffmerges_for_api_commits branch from 8ccc2f3 to 637125c Compare May 14, 2023 12:22
@tdesveaux
Copy link
Contributor

I added a new bare repository in modules/git/tests/repos/repo6_merge as none of the existing felt appropriate, let me know if you see one you'd prefer to add this to.

This repo has this history:

$ git log --all --graph --oneline
*---.   022f4ce621 (HEAD -> main, origin/main) Merge remote-tracking branches 'origin/merge/add_file', 'origin/merge/remove_file' and
'origin/merge/modify_file'
|\ \ \
| | | * d179264139 (origin/merge/modify_file, merge/modify_file) modify file
| |_|/
|/| |
| | * 38ec3e0cdc (origin/merge/remove_file, merge/remove_file) remove file
| |/
|/|
| * ae4b035e7c (origin/merge/add_file, merge/add_file) add file
|/
* 37d35c7ed3 setup test files

I choose an octopus merge to test a case where a commit has >2 parents.

I also choose to do a test in the git module rather than an integration test as the issue is contained to the GetCommitFileStatus function.

Lastly, in addition to the -c -> -m change. I changed --parents to --first-parent. Without it, a file from the main branch which was not changed at all in the merged branch would be listed as well.
This change allowed the command output to stay the same so I removed the change to parseCommitFileStatus.

(Sorry, I forgot about the guildelines to merge main and not rebase the history of a PR. I should be able to find the previous branch if necessary. Although, this is basically a new PR at this point).

@lunny
Copy link
Member

lunny commented May 14, 2023

Please remove those unnecessary files like description.

@lunny lunny added this to the 1.20.0 milestone May 14, 2023
@tdesveaux
Copy link
Contributor

I think I stripped everything I could

@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 May 14, 2023
@tdesveaux
Copy link
Contributor

The e2e-tests failure seem unrelated to the changes AFAIK.
Managed to reproduce in one run out of 10, strange.

@delvh delvh removed this from the 1.20.0 milestone Jun 7, 2023
@lunny lunny added this to the 1.21.0 milestone Jul 14, 2023
@denyskon denyskon added the giteabot/update-branch Hint for the bot that it should update a PR with the latest state on main label Aug 3, 2023
@GiteaBot
Copy link
Contributor

GiteaBot commented Aug 3, 2023

@jasugun please fix the merge conflicts. 🍵

@GiteaBot GiteaBot removed the giteabot/update-branch Hint for the bot that it should update a PR with the latest state on main label Aug 3, 2023
@GiteaBot
Copy link
Contributor

GiteaBot commented Aug 3, 2023

@jasugun please fix the merge conflicts. 🍵

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.

PRs from an org can't be updated/edited by maintainers.

So either keep the PR update-to-date by the author (to make sure it passes all checks), or let owners merge it without the update-to-date check.

@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 Aug 24, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 24, 2023
@lunny lunny enabled auto-merge (squash) August 24, 2023 06:08
@tdesveaux
Copy link
Contributor

PRs from an org can't be updated/edited by maintainers.

Didn't know that! Will keep it mind if I open another PR.

@lunny lunny merged commit b21b63c into go-gitea:main Aug 24, 2023
23 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 24, 2023
@tdesveaux tdesveaux deleted the dontnod_diffmerges_for_api_commits branch August 24, 2023 14:12
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 25, 2023
* giteaofficial/main:
  Fix review bar misalignment (go-gitea#26711)
  Use "small-loading-icon" insead of "btn-octicon is-loading" (go-gitea#26710)
  Improve Image Diff UI (go-gitea#26696)
  Make issue template field template access correct template data (go-gitea#26698)
  add Upload URL to release API (go-gitea#26663)
  Add merge files files to GetCommitFileStatus (go-gitea#20515)
  PATCH branch-protection updates check list even when checks are disabled (go-gitea#26351)
  Add `member`, `collaborator`, `contributor`, and `first-time contributor` roles and tooltips (go-gitea#26658)
  chore(actions): support cron schedule task (go-gitea#26655)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 22, 2023
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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants