Skip to content

Conversation

lunny
Copy link
Member

@lunny lunny commented Jan 12, 2020

As title. Inspired by @zeripath 's comment.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jan 12, 2020
Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

This should improve the speed quite a bit.

Just a nit (and the CI that's acting up).

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 12, 2020
@lunny lunny force-pushed the lunny/save_owner_name_repo branch from 68a2398 to af584e0 Compare January 12, 2020 02:48
@zeripath
Copy link
Contributor

zeripath commented Jan 12, 2020

I don't see any code changing the owner name during transfer repository - have you covered this case?


I see it now

@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 Jan 12, 2020
@zeripath
Copy link
Contributor

You need to update the test fixtures

@zeripath
Copy link
Contributor

zeripath commented Jan 12, 2020

@lunny lunny force-pushed the lunny/save_owner_name_repo branch from 5ea84c6 to fba2bb7 Compare January 12, 2020 05:16
@zeripath
Copy link
Contributor

Ok so it looks like there are a few places where owner is expected to be present because it would have been loaded in order to get the owner name. I'm not sure what's best here, it might be that it's easiest to just stick LoadOwner s before every call to repo. Owner but that could easily cause some deadlocks - and that still doesn't necessarily catch the templates.

There's also an issue with a unit test.

@lunny lunny force-pushed the lunny/save_owner_name_repo branch from 759da38 to 7b62e9a Compare January 12, 2020 07:12
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@7b62e9a). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #9717   +/-   ##
=========================================
  Coverage          ?   42.23%           
=========================================
  Files             ?      593           
  Lines             ?    78207           
  Branches          ?        0           
=========================================
  Hits              ?    33028           
  Misses            ?    41128           
  Partials          ?     4051
Impacted Files Coverage Δ
modules/setting/git.go 65.38% <ø> (ø)
cmd/hook.go 0% <0%> (ø)

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 7b62e9a...3fe901b. Read the comment docs.

@lunny lunny merged commit 5765212 into go-gitea:master Jan 12, 2020
@lunny lunny deleted the lunny/save_owner_name_repo branch January 12, 2020 09:36
@zeripath
Copy link
Contributor

We should expect to have to fix several NPEs in the coming weeks regarding this - however, I still think this is worthwhile.

We can also begin the process of removing all of the unnecessary loadOwner calls that this will free us from making.

@zeripath zeripath added this to the 1.12.0 milestone Jun 20, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants