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

rewrite pre-commit, post-commit and options hooks (fixes #1250) #1257

Merged
merged 7 commits into from
Mar 23, 2017
Merged

rewrite pre-commit, post-commit and options hooks (fixes #1250) #1257

merged 7 commits into from
Mar 23, 2017

Conversation

philfry
Copy link
Contributor

@philfry philfry commented Mar 14, 2017

see #1250

…e shell script that does not require custom hooks to be a sh-script
@lunny lunny added this to the 1.2.0 milestone Mar 15, 2017
@lunny lunny added the type/bug label Mar 15, 2017
Copy link
Member

@strk strk left a comment

Choose a reason for hiding this comment

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

I see you're feeding a bash script to a user-configurable scriptType, sounds fragile to me. Can you us a POSIX syntax please ?

models/repo.go Outdated
fmt.Sprintf("#!/usr/bin/env %s\nORI_DIR=`pwd`\nSHELL_FOLDER=$(cd \"$(dirname \"$0\")\";pwd)\ncd \"$ORI_DIR\"\nfor i in `ls \"$SHELL_FOLDER/update.d\"`; do\n sh \"$SHELL_FOLDER/update.d/$i\" $1 $2 $3\ndone", setting.ScriptType),
fmt.Sprintf("#!/usr/bin/env %s\nORI_DIR=`pwd`\nSHELL_FOLDER=$(cd \"$(dirname \"$0\")\";pwd)\ncd \"$ORI_DIR\"\nfor i in `ls \"$SHELL_FOLDER/post-receive.d\"`; do\n sh \"$SHELL_FOLDER/post-receive.d/$i\"\ndone", setting.ScriptType),
}
hookTpl = fmt.Sprintf("#!/usr/bin/env %s\ndata=$(cat)\nexitcodes=()\nhookname=$(basename $0)\nGIT_DIR=${GIT_DIR:-$(dirname $0)}\n\nfor hook in ${GIT_DIR}/hooks/${hookname}.d/*; do\ntest -x \"${hook}\" || continue\necho \"${data}\" | \"${hook}\"\nexitcodes+=($?)\ndone\n\nfor i in \"${exitcodes[@]}\"; do\n[ \"${i}\" == 0 ] || exit ${i}\ndone\n", setting.ScriptType)
Copy link
Member

Choose a reason for hiding this comment

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

bash-specific syntax ($(cat)) in what could be running with something else (setting.ScriptType).
Not that I understand how can Gitea possibly contains code to be run by unknown scripts, anyway the app.ini Cheat Sheet says ScriptType is usually bash or sh...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

data=$(cat) is not bash-specific.

bash can be run in posix mode when either invoking the script either using sh (sh scriptname, sh in shebang) or by using the switch --posix (or using set -o posix in the script itself). Tried both of it, script worked fine. Even though myarray=() is a bashism.

Some Debian systems might symlink /bin/sh to /bin/dash (the Debian Almquist Shell), so we might want to force using bash in this case.

Or, to make it more posix-compliant, give up on arrays:

#!/bin/sh

data=$(cat)
exitcodes=""
hookname=$(basename $0)
GIT_DIR=${GIT_DIR:-$(dirname $0)}

for hook in ${GIT_DIR}/hooks/${hookname}.d/*; do
	test -x "${hook}" || continue
	echo "${data}" | "${hook}"
	exitcodes="${exitcodes} $?"
done

for i in ${exitcodes}; do
	[ $i -eq 0 ] || exit $i
done

(tested with dash)

@vtolstov
Copy link

vtolstov commented Mar 15, 2017 via email

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 15, 2017
@strk
Copy link
Member

strk commented Mar 20, 2017

I dunno why are you talking about symlinks, but yes I think we could just invoke the commands, if executables. But please make sure you touch all files containing that bash invocation, I see 3 places.
Also it would be great to have unit tests for this

@philfry
Copy link
Contributor Author

philfry commented Mar 20, 2017

Am I missing something? The migrations?

$ find . -name '*.go' -print0 | xargs -r0 grep -P '^(?!//).*\bsh\b'
./models/migrations/v22.go:			fmt.Sprintf("#!/usr/bin/env %s\nORI_DIR=`pwd`\nSHELL_FOLDER=$(cd \"$(dirname \"$0\")\";pwd)\ncd \"$ORI_DIR\"\nfor i in `ls \"$SHELL_FOLDER/pre-receive.d\"`; do\n    sh \"$SHELL_FOLDER/pre-receive.d/$i\"\ndone", setting.ScriptType),
./models/migrations/v22.go:			fmt.Sprintf("#!/usr/bin/env %s\nORI_DIR=`pwd`\nSHELL_FOLDER=$(cd \"$(dirname \"$0\")\";pwd)\ncd \"$ORI_DIR\"\nfor i in `ls \"$SHELL_FOLDER/update.d\"`; do\n    sh \"$SHELL_FOLDER/update.d/$i\" $1 $2 $3\ndone", setting.ScriptType),
./models/migrations/v22.go:			fmt.Sprintf("#!/usr/bin/env %s\nORI_DIR=`pwd`\nSHELL_FOLDER=$(cd \"$(dirname \"$0\")\";pwd)\ncd \"$ORI_DIR\"\nfor i in `ls \"$SHELL_FOLDER/post-receive.d\"`; do\n    sh \"$SHELL_FOLDER/post-receive.d/$i\"\ndone", setting.ScriptType),
./models/migrations/v19.go:			fmt.Sprintf("#!/usr/bin/env %s\nORI_DIR=`pwd`\nSHELL_FOLDER=$(cd \"$(dirname \"$0\")\";pwd)\ncd \"$ORI_DIR\"\nfor i in `ls \"$SHELL_FOLDER/pre-receive.d\"`; do\n    sh \"$SHELL_FOLDER/pre-receive.d/$i\"\ndone", setting.ScriptType),
./models/migrations/v19.go:			fmt.Sprintf("#!/usr/bin/env %s\nORI_DIR=`pwd`\nSHELL_FOLDER=$(cd \"$(dirname \"$0\")\";pwd)\ncd \"$ORI_DIR\"\nfor i in `ls \"$SHELL_FOLDER/update.d\"`; do\n    sh \"$SHELL_FOLDER/update.d/$i\" $1 $2 $3\ndone", setting.ScriptType),
./models/migrations/v19.go:			fmt.Sprintf("#!/usr/bin/env %s\nORI_DIR=`pwd`\nSHELL_FOLDER=$(cd \"$(dirname \"$0\")\";pwd)\ncd \"$ORI_DIR\"\nfor i in `ls \"$SHELL_FOLDER/post-receive.d\"`; do\n    sh \"$SHELL_FOLDER/post-receive.d/$i\"\ndone", setting.ScriptType),
./vendor/golang.org/x/text/language/maketables.go:	"ro", "ru", "sh", "si", "sk", "sl", "sq", "sr", "sv", "sw", "ta", "te", "th",
./vendor/golang.org/x/text/language/maketables.go:	lang.updateLater("sh", "hbs")
./vendor/golang.org/x/sys/unix/types_openbsd.go:Input to cgo -godefs.  See also mkerrors.sh and mkall.sh
./vendor/golang.org/x/sys/unix/types_darwin.go:Input to cgo -godefs.  See also mkerrors.sh and mkall.sh
./vendor/golang.org/x/sys/unix/types_netbsd.go:Input to cgo -godefs.  See also mkerrors.sh and mkall.sh
./vendor/golang.org/x/sys/unix/types_freebsd.go:Input to cgo -godefs.  See also mkerrors.sh and mkall.sh
./vendor/golang.org/x/sys/unix/types_linux.go:Input to cgo -godefs.  See also mkerrors.sh and mkall.sh
./vendor/golang.org/x/sys/unix/types_solaris.go:Input to cgo -godefs.  See also mkerrors.sh and mkall.sh
./vendor/golang.org/x/sys/unix/types_dragonfly.go:Input to cgo -godefs.  See also mkerrors.sh and mkall.sh
./modules/base/tool.go:	sh := sha1.New()
./modules/base/tool.go:	sh.Write([]byte(data + setting.SecretKey + startStr + endStr + com.ToStr(minutes)))
./modules/base/tool.go:	encoded := hex.EncodeToString(sh.Sum(nil))
./modules/highlight/highlight.go:		".sh":     true,

@strk
Copy link
Member

strk commented Mar 20, 2017

Yes, the migrations. I'm not very sure what are them for, and how to effectively test them, will be tricky.
You can always trigger them by setting version.version in the database to the lowest version (19, in this case) and starting Gitea again. I guess it takes installing some hooks in the old format to test the migrations.

@lunny
Copy link
Member

lunny commented Mar 21, 2017

The migrations will be simple to rewrite all the scripts. But don't forget wiki repo's hooks.

@philfry
Copy link
Contributor Author

philfry commented Mar 21, 2017

afaics the wiki pages use the same hooks as the repos do. Which leads to funny side-effects[1] but that's not relevant here.
So, nothing to do for the wikis here.

I'm a little concerned about the migration scripts. In my understanding the scripts should update database schemas, scripts (i.e. hooks) and other stuff that might have changed between gitea versions.
I can change v19.go and v22.go to match the new hook scripts. So when updating from gitea .. say 0.9 .. the hooks will be rewritten. But when updating from gitea 1.1.0 to gitea > 1.1.0 will the changes be applied? Or do we need a completely new migration script v23.go (or whatever it will be named) that replaces the hooks once again?

edit
I think now I understand how these migration scripts work. Every migration script gets called on every update, regardless of its "version" number. So I'll only have to change some conditionals in v19.go and v22.go like if com.IsExist(customHooksDir).

[1] I have git-multimail.py in my git templates so on every wiki page change I get a mail with the diff :)

@strk
Copy link
Member

strk commented Mar 21, 2017

Good question about migrations. I think first step is understanding what those migrations did in v19 and v22. If they installed broken scripts, now we'd need a new migration to fix those scripts (if we can ensure those scripts are "owned" by gitea)

@philfry
Copy link
Contributor Author

philfry commented Mar 21, 2017

the migration works for me.
Tested:

  • freshly installed gitea 1.0.0 → 1.1.0 → 1.1.0fix_hooks
  • freshly installed gitea 1.1.0 → 1.1.0fix_hooks
  • gitea 1.1.0 w/ custom hooks → 1.1.0fix_hooks

@bkcsoft
Copy link
Member

bkcsoft commented Mar 21, 2017

As @strk pointed out, don't touch existing migrations, it will break things even worse. Create a new migration that updates all existing hooks.

@philfry
Copy link
Contributor Author

philfry commented Mar 21, 2017

@bkcsoft I did not touch existing migrations.

@@ -0,0 +1,83 @@
package migrations
Copy link
Member

Choose a reason for hiding this comment

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

comment header

@@ -100,6 +100,8 @@ var migrations = []Migration{
NewMigration("change the key_id and primary_key_id type", changeGPGKeysColumns),
// v25 -> v26
NewMigration("add show field in user openid table", addUserOpenIDShow),
// v26 -> v27
NewMigration("generate and migrate repo and wiki Git hooks", generateAndMigrateGitHookChains),
Copy link
Member

Choose a reason for hiding this comment

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

Format

Copy link
Member

Choose a reason for hiding this comment

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

Run make fmt should be enough (and we need our bot to guard after this, I may give that a try)

@lunny
Copy link
Member

lunny commented Mar 22, 2017

And maybe you can also fix another issue

SyncRepositoryHooks

This function didn't sync wiki repository's hooks.

@lunny
Copy link
Member

lunny commented Mar 22, 2017

otherwise LGTM

@tboerger tboerger 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 Mar 22, 2017
@strk
Copy link
Member

strk commented Mar 22, 2017

So doesn't it take any migration to fix the existing hooks ? How/when do those wrapper scripts get installed in git repos ?

@philfry
Copy link
Contributor Author

philfry commented Mar 22, 2017

@strk I'm not sure what you're talking about. Actually there is a migration script (v26.go) to install the new hooks.

@lunny I'll try to fix it. Do you want it in this PR or shall I open a new PR when done? (edit: the change is tiny enough to have it in this PR)
btw - the "comment header" change is done but this PRs status is still on "Changes requested".

@strk
Copy link
Member

strk commented Mar 22, 2017

@philfry sorry I think I was fooled by github as when I wrote that comment I really didn't see the migration file !

LGTM (trusted, didn't really test it)

@tboerger tboerger 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 Mar 22, 2017
@lunny lunny merged commit fe94032 into go-gitea:master Mar 23, 2017
@lunny
Copy link
Member

lunny commented Mar 23, 2017

@philfry, could you send a backport PR to v1.1? But maybe the migrations is difficult to handle.

lunny pushed a commit that referenced this pull request Mar 28, 2017
…t and options hooks (#1376)

* issue #1250, replace {pre,post}-receive and update hooks with a single shell script that does not require custom hooks to be a sh-script

* issue #1250, make script posix compilant

* v23, add migration script to update {pre,post}-receive and update hooks

* migration: use a more common name and rename v23 to v26 to avoid conflicts

* gofmt'ed and added copyright header

* fix SyncRepositoryHooks to also sync wiki repos

* no migration for you.
@bkcsoft bkcsoft added the backport/done All backports for this PR have been created label Jul 10, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants