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

no git hook executed during "gitea squash and merge" #6003

Closed
2 of 7 tasks
uwebartels opened this issue Feb 8, 2019 · 18 comments · Fixed by #6961
Closed
2 of 7 tasks

no git hook executed during "gitea squash and merge" #6003

uwebartels opened this issue Feb 8, 2019 · 18 comments · Fixed by #6961
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/bug
Milestone

Comments

@uwebartels
Copy link

  • Gitea version (or commit ref): dfad569, 1.7.1 (docker image)
  • Git version: git version 2.18.1
  • Operating system: Alpine Linux 3.8.2
  • Database (use [x]):
    • PostgreSQL
    • MySQL
    • MSSQL
    • SQLite
  • Can you reproduce the bug at https://try.gitea.io:
    • Yes (provide example URL)
    • No
    • Not relevant
  • Log gist:

Description

running a "squash and merge" in a pull request after an approval does not trigger a git hook post-receive. The git hook script is a bash script provided with "set -x". No corresponding output is found in any logfile.
But the merge is done.
Doing a git merge without gitea+pull request works like a charm.

Screenshots

none.

@apricote
Copy link
Contributor

apricote commented Feb 8, 2019

From the official documentation on Git Hooks:

[Server-Side Hooks] run before and after pushes to the server.

Emphasis mine.

This explains why it works when you manually do a git merge and git push.

When the Gitea Server performs a merge, it checks out your repository into a temporary location, runs git merge and then git pushes the new commit back into the main repository:

"git", "push", baseGitRepo.Path, pr.BaseBranch); err != nil {

So it looks like there is an actual bug here.

I am not sure if we want to support this though, as the error from post-receive would be displayed as a 500 error page and the error message probably only includes the exit code and not your stderr output.


References:

@uwebartels
Copy link
Author

Thanks for your response.
There is no 500 error page and no log entry which shows a hint of the post-receive hook.
What I'm wondering is, why the push in pull.go:570 is not triggering the post-receive hook. Cause when I merge the branch via cmd I also merge locally and then push the commited result. At least it would be nice to add some debug output.

Best...
Uwe

@apricote
Copy link
Contributor

apricote commented Feb 8, 2019

  1. Yes, the git push in line 570 should trigger the post-receive hook. Currently I can not confirm that this is or is not happening.

  2. If it would trigger the post-receive hook, you would still not get any useful output, per the reasons described in my first comment.


Can you maybe add some HTTP call to your post-receive script to a server that you control (local nginx on the machine?) so you can verify that the post-receive is not executed.

@zeripath
Copy link
Contributor

zeripath commented Feb 8, 2019

The prereceive and post-receive hooks won't run because the environment variables aren't being set properly.

Well aren't being set at all.

@zeripath
Copy link
Contributor

zeripath commented Feb 8, 2019

Even in my make upload not clone branch (#5702) they won't run because they don't run in the editor currently.

Personally I think they probably should run.

See my pr for refactoring the editor to use LFS. That goes almost all the way but doesn't set the final key variable to make them run SSH_ORIGINAL_COMMAND. (Which can be set to anything it just has to be set.)

@lunny
Copy link
Member

lunny commented Feb 9, 2019

@zeripath did you mean your PR will fix this bug?

@zeripath
Copy link
Contributor

zeripath commented Feb 9, 2019

Nope, it won't. I meant that as it currently stands it won't because that's current behaviour but I set almost all of the necessary environment variables to make them run.

I do think we should probably run the hooks but it looks like they were deliberately set up so that they wouldn't run for internal pushes etc.

So, there are two ways of fixing this:

  • Add the appropriate environment variables to internal pushes thus granting them hooks by default. We then have to account for the possibility that the git push might fail and be prepared to send that error back to the user.
  • Replicate the functionality of the hooks in our internal code.

I suspect that they original idea was that we would end up replicating the functionality of the hooks in the internal code, but of course that's code duplication so it gets missed/forgotten.

Therefore we should set it so that the hooks are run.

@lunny lunny added the type/bug label Feb 9, 2019
@zeripath
Copy link
Contributor

zeripath commented Feb 9, 2019

For clarity; in order for the hooks to be run you need to set the following environment variables:

SSH_ORIGINAL_COMMAND
GITEA_REPO_ID (models.ProtectedBranchRepoID)
GITEA_REPO_NAME (models.EnvRepoName)
GITEA_REPO_USER_NAME (models.EnvRepoUsername)
GITEA_REPO_IS_WIKI (models.EnvRepoIsWiki)
GITEA_PUSHER_NAME (models.EnvPusherName)
GITEA_PUSHER_ID (models.EnvPusherID)

That means that if you call a git command to commit or push you should probably set:

isWiki := "false"
if strings.HasSuffix(repo.Name, ".wiki") {
	isWiki = "true"
}
sig := doer.NewGitSig()

env := append(os.Environ(),
		"SSH_ORIGINAL_COMMAND=internal",
		"GIT_AUTHOR_NAME="+sig.Name,
		"GIT_AUTHOR_EMAIL="+sig.Email,
		"GIT_COMMITTER_NAME="+sig.Name,
		"GIT_COMMITTER_EMAIL="+sig.Email,
		models.EnvRepoName+"="+t.repo.Name,
		models.EnvRepoUsername+"="+repo.OwnerName,
		models.EnvRepoIsWiki+"="+isWiki,
		models.EnvPusherName+"="+doer.Name,
		models.EnvPusherID+"="+fmt.Sprintf("%d", doer.ID),
		models.ProtectedBranchRepoID+"="+fmt.Sprintf("%d", t.repo.ID),
)

@zeripath
Copy link
Contributor

zeripath commented Feb 9, 2019

The issue is that our integration tests fail if you set SSH_ORIGINAL_COMMAND, presumably because the environment isn't completely set up. I think if we migrate the affected tests to TestGit style it might work though.

@uwebartels
Copy link
Author

Hiho,

any update? Do you think somebody could fix this?

I'm heavily dependent on git hooks and the code review process.
webhooks unfortunately are not an option as they are asynchronous.

Muchas...
Uwe

@theAkito
Copy link

Does post-receive git hook work with other commands or does it not work at all?

@uwebartels
Copy link
Author

yes. the current workaround for us to work directly with git commands like

git merge --squash <branch>
git commit -m '<message>'
git push

I just cannot enforce merging via gitea gui, thus not using the branch protection.

Best Regards
Uwe

@theAkito
Copy link

theAkito commented Mar 22, 2019

Yes, but do Git hooks over Gitea GUI not work at all or does only merging over Gitea GUI not work?

@uwebartels
Copy link
Author

git hooks over gitea gui are not triggered at all. merging works well.

@theAkito
Copy link

git hooks over gitea gui are not triggered at all. merging works well.

Well, so apparently that's why my hook doesn't work either. I think it should be clarified in the OP that this issue regards Git hooks in general, which is a big deal.

@zeripath
Copy link
Contributor

I think I'd better take another look at this.

The issue was that when I implemented the final environment variable SSH_ORIGINAL_COMMAND the integration test suites completely failed.

That's likely due to the integration testing environment being too basic and not completely set up.

The required variables are listed above if anyone wants to try.

@theAkito
Copy link

I think I'd better take another look at this.

The issue was that when I implemented the final environment variable SSH_ORIGINAL_COMMAND the integration test suites completely failed.

That's likely due to the integration testing environment being too basic and not completely set up.

The required variables are listed above if anyone wants to try.

Does your example from above contain the finalized additions or do parts of those need to be customized for each git hook with specific variables?

@stale
Copy link

stale bot commented May 23, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 weeks. Thank you for your contributions.

@stale stale bot added the issue/stale label May 23, 2019
@lafriks lafriks added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label May 24, 2019
@stale stale bot removed the issue/stale label May 24, 2019
@lafriks lafriks added this to the 1.9.0 milestone May 24, 2019
@techknowlogick techknowlogick modified the milestones: 1.9.0, 1.10.0 Jun 25, 2019
@lafriks lafriks modified the milestones: 1.10.0, 1.9.0 Jul 1, 2019
@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
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants