Skip to content
This repository has been archived by the owner on Jun 8, 2019. It is now read-only.

Add API for manipulating Git hooks #156

Merged
merged 1 commit into from
Apr 16, 2019

Conversation

segevfiner
Copy link

@segevfiner segevfiner commented Mar 26, 2019

This adds API support for manipulating Git hooks. Accompanying PR for a PR against Gitea: go-gitea/gitea#6436

Signed-off-by: Segev Finer <segev@codeocean.com>
gitea/git_hook.go Show resolved Hide resolved
gitea/git_hook.go Show resolved Hide resolved
gitea/git_hook.go Show resolved Hide resolved
gitea/git_hook.go Show resolved Hide resolved
@segevfiner
Copy link
Author

@zeripath I based this on the existing API for web hooks, which doesn't seem to escape. Actually, I'm not seeing any reference to PathEscape in the entire SDK. So if I add escaping here, that will be the only place in the SDK that this would be done. So are you sure you want me to do this in this PR?

I couldn't find a function named PathSegmentsEscape, but should we add escaping here, we only need the normal PathEscape anyhow, as it's just the name of the hook which is a simple string without slashes.

P.S. If you are already looking at escaping URL components. An alternative way to build URLs with correct escaping is to use a library/package that implements RFC 6570 - URI Template level 4 to build the URLs which already handles escaping and should interface quite nicely with a project that uses Swagger (Swagger/OpenAPI v3 was actually designed with it in mind).

@zeripath
Copy link

We have to start somewhere. But I guess we'll have to do a refactor to actually escape all this stuff properly.

In terms of using templates to build URIs I suspect that any code that uses this will be bulky and verbose. Go doesn't have named function arguments, so you're then suggesting passing in either structs or maps to get the named arguments. Of course you can just use the go html template but even then you're probably going to end up with bulky code and you still have to pass in named arguments as a map or use numeric arguments.

Now there is an alternative and that is to get swagger or something else to generate this for us - this would clearly be the best idea but again that's a massive refactor. (I'm perfectly happy with bulky code so long as I never need look at it.)

Ok if you don't want to do the escaping I will approve but it really does need fixing - it doesn't exactly engender confidence in our code to see obvious escaping errors.

@techknowlogick techknowlogick merged commit 7d954d7 into go-gitea:master Apr 16, 2019
@segevfiner segevfiner deleted the git-hook branch April 16, 2019 17:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants