Skip to content

Refactor lfs requests #26783

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 16 commits into from
Sep 18, 2023
Merged

Refactor lfs requests #26783

merged 16 commits into from
Sep 18, 2023

Conversation

harryzcy
Copy link
Contributor

@harryzcy harryzcy commented Aug 29, 2023 •
edited
Loading

  • Refactor lfs request code
    • The original code uses performRequest function to create the request, uses a callback to modify the request, and then send the request.
    • Now it's replaced with createRequest that only creates request and performRequest that only sends the request.
  • Reuse createRequest and performRequest in http_client.go and transferadapter.go

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 29, 2023
KN4CK3R
KN4CK3R reviewed Aug 29, 2023
View reviewed changes
@wxiaoguang
Copy link
Contributor

No description? What's the purpose of this refactoring? Just refactor/improve the old code, or fix/improve something else by the way?

@harryzcy harryzcy closed this Sep 15, 2023
@harryzcy harryzcy deleted the refactor-lfs-req branch September 15, 2023 23:26
@wxiaoguang
Copy link
Contributor

Pity to see this is closed. As a refactoring PR, overall LGTM (but there is no notification when the it got updated description, I just got reminded by the "closed" message)

@harryzcy
Copy link
Contributor Author

harryzcy commented Sep 16, 2023 •
edited
Loading

I can reopen it, if this change is welcomed

@harryzcy harryzcy restored the refactor-lfs-req branch September 16, 2023 05:26
@harryzcy harryzcy reopened this Sep 16, 2023
wxiaoguang
wxiaoguang reviewed Sep 16, 2023
View reviewed changes
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 16, 2023 •
edited
Loading

Thank you. I think there could be some small fine tunes: set Content-Type if it doesn't exist harryzcy#15

  1. Only use req after checking err is nil or not
  2. Pass the header with Content-Type if it already knows what it should be

@harryzcy
Copy link
Contributor Author

harryzcy commented Sep 16, 2023 •
edited
Loading

  1. Only use req after checking err is nil or not

Good catch, weird that golangci didn't catch it.

Edit: err is checked after req.Header.Set, that's why. golangci only checks if variable is used

@wxiaoguang wxiaoguang mentioned this pull request Sep 16, 2023
Merged
@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 Sep 16, 2023
@wxiaoguang wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Sep 16, 2023
@wxiaoguang wxiaoguang added this to the 1.21.0 milestone Sep 16, 2023
lunny
lunny approved these changes Sep 16, 2023
View reviewed changes
Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

Except https://github.com/go-gitea/gitea/pull/26783/files#r1327936606

@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 Sep 16, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 18, 2023
@lunny lunny enabled auto-merge (squash) September 18, 2023 08:10
@lunny lunny merged commit 9631958 into go-gitea:main Sep 18, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 18, 2023
silverwind added a commit to silverwind/gitea that referenced this pull request Sep 18, 2023
@silverwind
Merge remote-tracking branch 'origin/main' into primary
9a23d22 
* origin/main:
  Fix the incorrect route path in the user edit page. (go-gitea#27007)
  Refactor lfs requests (go-gitea#26783)
  Display archived labels specially when listing labels (go-gitea#26820)
  Remove a `gt-float-right` and some unnecessary helpers (go-gitea#27110)
  [skip ci] Updated licenses and gitignores
  Fix token endpoints ignore specified account (go-gitea#27080)
  Make SSPI auth mockable (go-gitea#27036)
@harryzcy harryzcy deleted the refactor-lfs-req branch September 18, 2023 18:14
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 20, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
add8afe 
* giteaofficial/main:
  Improve actions docs related to `pull_request` event (go-gitea#27126)
  Remove outdated paragraphs when comparing Gitea Actions to GitHub Actions (go-gitea#27119)
  Fix: treat tab "overview" as "repositories" in user profiles without readme (go-gitea#27124)
  Fix incorrect test code for error handling (go-gitea#27139)
  Increase auth provider icon size on login page (go-gitea#27122)
  fix pagination for followers and following (go-gitea#27127)
  services/wiki: Close() after error handling (go-gitea#27129)
  Use fetch helpers instead of fetch (go-gitea#27026)
  Change green buttons to primary color (go-gitea#27099)
  Fix wrong xorm get usage on migration (go-gitea#27111)
  Fix the incorrect route path in the user edit page. (go-gitea#27007)
  Refactor lfs requests (go-gitea#26783)
  Display archived labels specially when listing labels (go-gitea#26820)
  Remove a `gt-float-right` and some unnecessary helpers (go-gitea#27110)
  [skip ci] Updated licenses and gitignores
  Fix token endpoints ignore specified account (go-gitea#27080)
  Make SSPI auth mockable (go-gitea#27036)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KN4CK3R KN4CK3R KN4CK3R left review comments

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.
5 participants
@harryzcy @wxiaoguang @lunny @KN4CK3R @GiteaBot