I'd like to ask for @puiterwijk +1 on this but this seems simple and efficient to me, thanks @bkabrda

:thumbsup:

I would say that this breaks part of the "promise" of the hooks that they get run for every push: even forking is pushing to the forked repo.
If we do go ahead with this, please add a note to the documentation for hooks.

Though my personal preference would be instead to make the imports faster: the main contributor to the import slowness per my measurements is the libgit2 import.
If we can make loading that lazy until we need it, any hook runs that don't need info from the git repo (or that use other means to get that info) should run significantly faster.

If we think the entries in the pagure_logs table are wrong, those hooks should instead be short-circuiting themselves if internal=True.

That all being said, it's up to @pingou.

I would say that this breaks part of the "promise" of the hooks that they get run for every push: even forking is pushing to the forked repo.
If we do go ahead with this, please add a note to the documentation for hooks.

The fact that forking is pushing to the forked repo is an implementation detail though. Forking is an event that creates new repo with some content, but Pagure doesn't provide any guarantee on how the content gets there.

Though my personal preference would be instead to make the imports faster: the main contributor to the import slowness per my measurements is the libgit2 import.
If we can make loading that lazy until we need it, any hook runs that don't need info from the git repo (or that use other means to get that info) should run significantly faster.

Yes, that is the main contributor, but not the only one. Please consider that there are deployments that have repos with hundreds of branches, so any action that runs in dozens of miliseconds will be multiplied by that and thus will result in seconds of slowdown for end user.

If we think the entries in the pagure_logs table are wrong, those hooks should instead be short-circuiting themselves if internal=True.

I do agree that we could do that, but IMO this would make the code much less readable, because we'd pretty much need to add conditionals everywhere, handle the lazy imports, make sure we only connect to DB and message bus when absolutely necessary etc. While I do agree that technically this might be a clearer solution, I don't think it's worth it practically.

That all being said, it's up to @pingou.

Thank you for your comments!

So as an update, in my staging environment with this patch, forking a repo with 84 branches took about a minute. Previously it took less than 10 seconds, so the slowdown is still significant (an order of magnitude). Without this patch, the fork takes about 7 minutes. Since the pushes are mostly about running subprocesses, I think we could improve this further by running several pushes simultaneously using threads and get another significant performance increase. When/if this gets merged, I'll try seeing how much that would improve the situation.

Are all the changes pinpointed to us using subprocess for "git push" or are there other changes that are impacting this?

Yeah, I think it's just because of doing the sub-process thing branch by branch. Previously we just did pygit2.clone_repository, which is in-process and it doesn't invoke any hooks => very fast. Right now we create a subprocess for push of every branch and each one of these pushes still invokes hookrunner multiple times, even though that exits immediately (using this fix).

I'm leaning towards merging this PR, we were not running hooks on the forking operation before and the time it takes now is really not something we want.
Logging again commits on forks actually is not something we want, we only want to log commits from the main project I believe.

Let's get this in :)

:thumbsup: