[PATCH] Add support for a 'pre-push' hook

[PATCH] Add support for a 'pre-push' hook

[Aske Olsson]

If the script .git/hooks/pre-push exists and is executable it will be
called before a `git push` command, and when the script exits with a
non-zero status the push will be aborted.
The hook can be overridden by passing the '--no-verify' option to
`git push`.

The pre-push hook is usefull to run tests etc. before push. Or to make
sure that if a binary solution like git-media, git-annex or git-bin is
used the binaries are uploaded before the push, so when others do a
fetch the binaries will be available already. This also reduces the
need for introducing extra (git) commands to e.g. sync binaries.

Signed-off-by: Aske Olsson <askeolsson@gmail.com>
---
 Documentation/git-push.txt |  11 +++-
 Documentation/githooks.txt |  12 +++++
 builtin/push.c             |   6 +++
 t/t5542-pre-push-hook.sh   | 132 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 160 insertions(+), 1 deletion(-)
 create mode 100644 t/t5542-pre-push-hook.sh

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index fe46c42..5807b6a 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git push' [--all | --mirror | --tags] [-n | --dry-run]
[--receive-pack=<git-receive-pack>]
    [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose]
[-u | --set-upstream]
-   [<repository> [<refspec>...]]
+   [ --no-verify] [<repository> [<refspec>...]]

 DESCRIPTION
 -----------
@@ -157,6 +157,10 @@ useful if you write an alias or script around 'git push'.
  receiver share many of the same objects in common. The default is
  \--thin.

+--no-verify::
+ This option bypasses the pre-push hook.
+ See also linkgit:githooks[5].
+
 -q::
 --quiet::
  Suppress all output, including the listing of updated refs,
@@ -430,6 +434,11 @@ Commits A and B would no longer belong to a
branch with a symbolic name,
 and so would be unreachable.  As such, these commits would be removed by
 a `git gc` command on the origin repository.

+HOOKS
+-----
+This command can run the `pre-push` hook.  See linkgit:githooks[5] for
+more information.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index b9003fe..847e0f8 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -176,6 +176,18 @@ save and restore any form of metadata associated
with the working tree
 (eg: permissions/ownership, ACLS, etc).  See contrib/hooks/setgitperms.perl
 for an example of how to do this.

+[[pre-push]]
+pre-push
+~~~~~~~~
+
+This hook is invoked by 'git push' and can be bypassed with the
+`--no-verify` option. It takes no parameter, and is invoked before
+the push happens.
+Exiting with a non-zero status from this script causes 'git push'
+to abort.
+
+
+
 [[pre-receive]]
 pre-receive
 ~~~~~~~~~~~
diff --git a/builtin/push.c b/builtin/push.c
index db9ba30..9c4d2ec 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -16,6 +16,7 @@ static const char * const push_usage[] = {
 };

 static int thin;
+static int no_verify;
 static int deleterefs;
 static const char *receivepack;
 static int verbosity;
@@ -392,6 +393,7 @@ int cmd_push(int argc, const char **argv, const
char *prefix)
  N_("control recursive pushing of submodules"),
  PARSE_OPT_OPTARG, option_parse_recurse_submodules },
  OPT_BOOLEAN( 0 , "thin", &thin, N_("use thin pack")),
+ OPT_BOOLEAN('0', "no-verify", &no_verify, "bypass pre-push hook"),
  OPT_STRING( 0 , "receive-pack", &receivepack, "receive-pack",
N_("receive pack program")),
  OPT_STRING( 0 , "exec", &receivepack, "receive-pack", N_("receive
pack program")),
  OPT_BIT('u', "set-upstream", &flags, N_("set upstream for git pull/status"),
@@ -419,6 +421,10 @@ int cmd_push(int argc, const char **argv, const
char *prefix)
  set_refspecs(argv + 1, argc - 1);
  }

+ if (!no_verify && run_hook(NULL, "pre-push")) {
+ die(_("pre-push hook failed: exiting\n"));
+ }
+
  rc = do_push(repo, flags);
  if (rc == -1)
  usage_with_options(push_usage, options);
diff --git a/t/t5542-pre-push-hook.sh b/t/t5542-pre-push-hook.sh
new file mode 100644
index 0000000..842aa23
--- /dev/null
+++ b/t/t5542-pre-push-hook.sh
@@ -0,0 +1,132 @@
+#!/bin/sh
+
+test_description='pre-push hook'
+
+. ./test-lib.sh
+
+D=`pwd`
+HOOK="master/.git/hooks/pre-push"
+
+# Repo pair
+mk_repo_pair () {
+ rm -rf master mirror &&
+ mkdir mirror &&
+ (
+ cd mirror &&
+ git init &&
+ git config receive.denyCurrentBranch warn
+ )
+ mkdir master &&
+ (
+ cd master &&
+ git init &&
+ git remote add $1 up ../mirror
+ )
+}
+
+# hook that always succeeds
+mk_hook_exec () {
+cat > "$HOOK" <<EOF
+#!/bin/sh
+exit 0
+EOF
+chmod +x "$HOOK"
+}
+
+# hook that fails
+mk_hook_fail () {
+cat > "$HOOK" <<EOF
+#!/bin/sh
+exit 1
+EOF
+chmod +x "$HOOK"
+}
+
+# nonexecutable hook
+mk_hook_no_exec () {
+rm -f "$HOOK"
+cat > "$HOOK" <<EOF
+#!/bin/sh
+echo 'test run'
+exit 0
+EOF
+}
+
+test_expect_success 'push with no pre-push hook' '
+ mk_repo_pair &&
+ (
+ cd master &&
+ echo one >foo && git add foo && git commit -m one &&
+ git push --mirror up
+ )
+'
+
+test_expect_success 'push --no-verify with no pre-push hook' '
+ mk_repo_pair &&
+ (
+ cd master &&
+ echo one >foo && git add foo && git commit -m one &&
+ git push --no-verify --mirror up
+ )
+'
+
+test_expect_success 'push with succeeding pre-push hook' '
+ mk_repo_pair &&
+ (
+ mk_hook_exec &&
+ cd master &&
+ echo one >foo && git add foo && git commit -m one &&
+ git push --mirror up
+ )
+'
+
+test_expect_success 'push --no-verify with succeeding pre-push hook' '
+ mk_repo_pair &&
+ (
+ mk_hook_exec &&
+ cd master &&
+ echo one >foo && git add foo && git commit -m one &&
+ git push --no-verify --mirror up
+ )
+'
+
+test_expect_success 'push with failing pre-push hook' '
+ mk_repo_pair &&
+ (
+ mk_hook_fail &&
+ cd master &&
+ echo one >foo && git add foo && git commit -m one &&
+ test_must_fail git push --mirror up
+ )
+'
+
+test_expect_success 'push --no-verify with failing hook' '
+ mk_repo_pair &&
+ (
+ mk_hook_fail &&
+ cd master &&
+ echo one >foo && git add foo && git commit -m one &&
+ git push --no-verify --mirror up
+ )
+'
+
+test_expect_success 'push with non-executable pre-push hook' '
+ mk_repo_pair &&
+ (
+ mk_hook_no_exec &&
+ cd master &&
+ echo one >foo && git add foo && git commit -m one &&
+ git push --mirror up
+ )
+'
+
+test_expect_success 'push --no-verify with non-executable pre-push hook' '
+ mk_repo_pair &&
+ (
+ mk_hook_no_exec &&
+ cd master &&
+ echo one >foo && git add foo && git commit -m one &&
+ git push --no-verify --mirror up
+ )
+'
+test_done
--
1.8.0

Re: [PATCH] Add support for a 'pre-push' hook

[Matthieu Moy]

Aske Olsson <askeolsson@gmail.com> writes:

> If the script .git/hooks/pre-push exists and is executable it will be
> called before a `git push` command, and when the script exits with a
> non-zero status the push will be aborted.

That sounds like a sane thing to do.

>  Documentation/git-push.txt |  11 +++-
>  Documentation/githooks.txt |  12 +++++
>  builtin/push.c             |   6 +++
>  t/t5542-pre-push-hook.sh   | 132 +++++++++++++++++++++++++++++++++++++++++++++

It would be nice to provide a sample hook in the default template. See
the template/ directory in Git's source code.

> +--no-verify::
> + This option bypasses the pre-push hook.
> + See also linkgit:githooks[5].
> +
>  -q::
>  --quiet::
>   Suppress all output, including the listing of updated refs,

Here, and below: you seem to have whitespace damage. Somebody replaced
tabs with spaces I guess. Using git send-email helps avoiding this.

> +D=`pwd`

Unused variable, left from previous hacking I guess.

> +# hook that always succeeds
> +mk_hook_exec () {
> +cat > "$HOOK" <<EOF
> +#!/bin/sh
> +exit 0
> +EOF
> +chmod +x "$HOOK"
> +}
> +
> +# hook that fails
> +mk_hook_fail () {
> +cat > "$HOOK" <<EOF
> +#!/bin/sh
> +exit 1
> +EOF
> +chmod +x "$HOOK"
> +}

I'd add a "touch hook-ran" in the script, a "rm -f hook-ran" before
launching git-push, and test the file existance after the hook to make
sure it was ran.

> +test_expect_success 'push with no pre-push hook' '
> + mk_repo_pair &&
> + (
> + cd master &&
> + echo one >foo && git add foo && git commit -m one &&
> + git push --mirror up
> + )
> +'
> +
> +test_expect_success 'push --no-verify with no pre-push hook' '
> + mk_repo_pair &&

I don't think you need to re-create the repos for each tests. Tests are
good, but they're better when they're fast so avoiding useless
operations is good.

We try to write tests so that one test failure does not imply failures
of the next tests (i.e. stopping in the middle should not not leave
garbage that would prevent the next test from running), but do not
necessarily write 100% self-contained tests.

> + echo one >foo && git add foo && git commit -m one &&

test_commit ?

> +test_expect_success 'push with failing pre-push hook' '
> + mk_repo_pair &&
> + (
> + mk_hook_fail &&
> + cd master &&
> + echo one >foo && git add foo && git commit -m one &&
> + test_must_fail git push --mirror up
> + )
> +'

It would be cool to actually check that the push was not performed
(verify that the target repo is still pointing to the old revision).
Otherwise, an implementation that would call run_hook after pushing
would pass the tests.

> +test_expect_success 'push with non-executable pre-push hook' '
> + mk_repo_pair &&
> + (
> + mk_hook_no_exec &&
> + cd master &&
> + echo one >foo && git add foo && git commit -m one &&
> + git push --mirror up
> + )
> +'
> +
> +test_expect_success 'push --no-verify with non-executable pre-push hook' '
> + mk_repo_pair &&
> + (
> + mk_hook_no_exec &&
> + cd master &&
> + echo one >foo && git add foo && git commit -m one &&
> + git push --no-verify --mirror up
> + )
> +'

These two tests are not testing much. The hook is not executable, so it
shouldn't be ran, but you do not check whether the hook is ran or not.
At least make the "exit 0" an "exit 1" in the hook.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] Add support for a 'pre-push' hook

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>> +# hook that fails
>> +mk_hook_fail () {
>> +cat > "$HOOK" <<EOF
>> +#!/bin/sh
>> +exit 1
>> +EOF
>> +chmod +x "$HOOK"
>> +}
>
> I'd add a "touch hook-ran" in the script, a "rm -f hook-ran" before
> launching git-push, and test the file existance after the hook to make
> sure it was ran.

And if you create that "evidence that it did ran" file without using
"touch", it would be perfect.  Unless you are updating the timestamp
of an existing file while preserving the contents of it, it is
misleading to use "touch".

All other points in your review are good.  Thanks.

Re: [PATCH] Add support for a 'pre-push' hook

[Aske Olsson]

On Fri, Nov 16, 2012 at 9:25 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Aske Olsson <askeolsson@gmail.com> writes:
>
>> +--no-verify::
>> + This option bypasses the pre-push hook.
>> + See also linkgit:githooks[5].
>> +
>>  -q::
>>  --quiet::
>>   Suppress all output, including the listing of updated refs,
>
> Here, and below: you seem to have whitespace damage. Somebody replaced
> tabs with spaces I guess. Using git send-email helps avoiding this.

I had some firewall problems at work, so ended up sending from gmail.
Will fix ;)

>> +D=`pwd`
>
> Unused variable, left from previous hacking I guess.

Yep!

> I'd add a "touch hook-ran" in the script, a "rm -f hook-ran" before
> launching git-push, and test the file existance after the hook to make
> sure it was ran.

Yea' that would probably be a good idea!

> I don't think you need to re-create the repos for each tests. Tests are
> good, but they're better when they're fast so avoiding useless
> operations is good.
>
> We try to write tests so that one test failure does not imply failures
> of the next tests (i.e. stopping in the middle should not not leave
> garbage that would prevent the next test from running), but do not
> necessarily write 100% self-contained tests.

Ok, I'll speed it up.

>> + echo one >foo && git add foo && git commit -m one &&
>
> test_commit ?

Cool, I'll use that!

> It would be cool to actually check that the push was not performed
> (verify that the target repo is still pointing to the old revision).
> Otherwise, an implementation that would call run_hook after pushing
> would pass the tests.
[...]
>
> These two tests are not testing much. The hook is not executable, so it
> shouldn't be ran, but you do not check whether the hook is ran or not.
> At least make the "exit 0" an "exit 1" in the hook.

All very good points, thanks. I'll get back to hacking.

-Aske

Re: [PATCH] Add support for a 'pre-push' hook

[Junio C Hamano]

Aske Olsson <askeolsson@gmail.com> writes:

> If the script .git/hooks/pre-push exists and is executable it will be
> called before a `git push` command, and when the script exits with a
> non-zero status the push will be aborted.
> The hook can be overridden by passing the '--no-verify' option to
> `git push`.
>
> The pre-push hook is usefull to run tests etc. before push. Or to make
> sure that if a binary solution like git-media, git-annex or git-bin is
> used the binaries are uploaded before the push, so when others do a
> fetch the binaries will be available already. This also reduces the
> need for introducing extra (git) commands to e.g. sync binaries.
>
> Signed-off-by: Aske Olsson <askeolsson@gmail.com>
> ---
> ...
> +[[pre-push]]
> +pre-push
> +~~~~~~~~
> +
> +This hook is invoked by 'git push' and can be bypassed with the
> +`--no-verify` option. It takes no parameter, and is invoked before
> +the push happens.
> +Exiting with a non-zero status from this script causes 'git push'
> +to abort.
> ...
> + if (!no_verify && run_hook(NULL, "pre-push")) {
> + die(_("pre-push hook failed: exiting\n"));
> + }

NAK, at least in the current form.  At least the system should tell
the hook where it is pushing and what is being pushed.

After working on my notebook, I may want to push to my desktop
machine first to test it, without having to test locally on an
underpowered CPU, but when I am publishing the end result to the
wider world, I do want to test the result beforehand.  Without
"where am I pushing", the hook would not help me to enforce testing
only for the latter.

A lazy "git push" without any other argument may be configured in my
repository to push its "maint", "master", "next" and "pu" branches
to the public repository.  I may say "git push origin +pu", while on
one of the topic branches, to push only the "pu" branches out before
I am confident that the other branches I'll eventually publish are
ready (it is more likely that I may even *know* they are broken and
do not pass the test in such a case, and that is why I am pushing
only "pu" out).  I'd want to run tests only on 'pu' in such a case.
Without "what am I pushing", the hook would not be able to help me
doing that, either.

Besides, there are five valid reasons to add a new hook to the
system, but your version of pre-push does not satisfy any of them:

     http://thread.gmane.org/gmane.comp.version-control.git/94111/focus=71069

It is more like "to always cause an action before running a git
operation locally," so you can even write

    cat >$HOME/bin/git-mypush <<\EOF
    #!/bin/sh
    run test || exit
    exec git push "$@"
    EOF
    chmod +x $HOME/bin/git-mypush

and then can run "git mypush" instead of adding this hook.

Re: [PATCH] Add support for a 'pre-push' hook

[Michael Haggerty]

On 11/16/2012 09:30 PM, Junio C Hamano wrote:
> Aske Olsson <askeolsson@gmail.com> writes:
> 
>> If the script .git/hooks/pre-push exists and is executable it will be
>> called before a `git push` command, and when the script exits with a
>> non-zero status the push will be aborted.
>> The hook can be overridden by passing the '--no-verify' option to
>> `git push`.
>>
>> The pre-push hook is usefull to run tests etc. before push. Or to make
>> sure that if a binary solution like git-media, git-annex or git-bin is
>> used the binaries are uploaded before the push, so when others do a
>> fetch the binaries will be available already. This also reduces the
>> need for introducing extra (git) commands to e.g. sync binaries.
>>
>> Signed-off-by: Aske Olsson <askeolsson@gmail.com>
>> ---
>> ...
>> +[[pre-push]]
>> +pre-push
>> +~~~~~~~~
>> +
>> +This hook is invoked by 'git push' and can be bypassed with the
>> +`--no-verify` option. It takes no parameter, and is invoked before
>> +the push happens.
>> +Exiting with a non-zero status from this script causes 'git push'
>> +to abort.
>> ...
>> + if (!no_verify && run_hook(NULL, "pre-push")) {
>> + die(_("pre-push hook failed: exiting\n"));
>> + }
> 
> NAK, at least in the current form.  At least the system should tell
> the hook where it is pushing and what is being pushed.

I agree.

> Besides, there are five valid reasons to add a new hook to the
> system, but your version of pre-push does not satisfy any of them:
> 
>      http://thread.gmane.org/gmane.comp.version-control.git/94111/focus=71069

Here I disagree.  I think it satisfies (1):

>  (1) A hook that countermands the normal decision made by the
>      underlying command.  Examples of this class are the update
>      hook and the pre-commit hook.

pre-push would be very similar in spirit to pre-commit: pre-commit is a
filter that can prevent a "bad" commit from getting into the local
repository; pre-push is similarly a filter between the local repo and
remote repositories.

I also think it satisfies (2) and/or (5b):

>  (2) A hook that operates on data generated after the command
>      starts to run.  [...]

>  (5) [...]  Another example is the post-checkout
>      hook that gets information that is otherwise harder to get
>      (namely, if it was a branch checkout or file checkout --
>      you can figure it out by examining the command line but
>      that already is part of the processing git-checkout does
>      anyway, so no need to force duplicating that code in the
>      userland).

It would not be trivial for a wrapper script to figure out what branches
and commits are about to be pushed.  But "git push" could tell the hook
script what branches are to be pushed.  And if the pre-push hook is run
after negotiation between client and server of what commits need to be
transfered, the hook could also be provided that information and use it
to figure out which commits it should vet.


Although a pre-receive script on the remote repository could do most of
the same things as a pre-push script, the latter would sometimes have
advantages because it is run "client-side":

* When the user doesn't have the ability to change the pre-receive
script on the server (think public git hosting services).

* For user-specific actions that are not wanted by all users pushing to
the same server (for example, maybe I add the string "WIP" to commit
messages for commits that are not ready to be pushed; my pre-push script
could verify that I don't push such a commit by accident).

* Preventing "secret" info (password files, proprietary branches) from
being pushed.  Even if the remote repo were taught to reject them, they
would have already traversed the internet.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH] Add support for a 'pre-push' hook

[Aske Olsson]

On Sat, Nov 17, 2012 at 7:39 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>
> On 11/16/2012 09:30 PM, Junio C Hamano wrote:
> > Aske Olsson <askeolsson@gmail.com> writes:
> >
> >> If the script .git/hooks/pre-push exists and is executable it will be
> >> called before a `git push` command, and when the script exits with a
> >> non-zero status the push will be aborted.
> >> The hook can be overridden by passing the '--no-verify' option to
> >> `git push`.
> >>
> >> The pre-push hook is usefull to run tests etc. before push. Or to make
> >> sure that if a binary solution like git-media, git-annex or git-bin is
> >> used the binaries are uploaded before the push, so when others do a
> >> fetch the binaries will be available already. This also reduces the
> >> need for introducing extra (git) commands to e.g. sync binaries.
> >>
> >> Signed-off-by: Aske Olsson <askeolsson@gmail.com>
> >> ---
> >> ...
> >> +[[pre-push]]
> >> +pre-push
> >> +~~~~~~~~
> >> +
> >> +This hook is invoked by 'git push' and can be bypassed with the
> >> +`--no-verify` option. It takes no parameter, and is invoked before
> >> +the push happens.
> >> +Exiting with a non-zero status from this script causes 'git push'
> >> +to abort.
> >> ...
> >> + if (!no_verify && run_hook(NULL, "pre-push")) {
> >> + die(_("pre-push hook failed: exiting\n"));
> >> + }
> >
> > NAK, at least in the current form.  At least the system should tell
> > the hook where it is pushing and what is being pushed.
>
> I agree.

Yes, I also agree that is a nice piece of information to pass to the hook.
Will work on that.


> > Besides, there are five valid reasons to add a new hook to the
> > system, but your version of pre-push does not satisfy any of them:
> >
> >      http://thread.gmane.org/gmane.comp.version-control.git/94111/focus=71069
>
> Here I disagree.  I think it satisfies (1):
>
> >  (1) A hook that countermands the normal decision made by the
> >      underlying command.  Examples of this class are the update
> >      hook and the pre-commit hook.
>
> pre-push would be very similar in spirit to pre-commit: pre-commit is a
> filter that can prevent a "bad" commit from getting into the local
> repository; pre-push is similarly a filter between the local repo and
> remote repositories.

Yes, I was thinking of a pre-push hook in the same way, preventing bad
commits from leaving the repo, but I guess you could argue that a bad
commit shouldn't happen in the first place due to the pre-commit hook.

> I also think it satisfies (2) and/or (5b):
>
> >  (2) A hook that operates on data generated after the command
> >      starts to run.  [...]
>
> >  (5) [...]  Another example is the post-checkout
> >      hook that gets information that is otherwise harder to get
> >      (namely, if it was a branch checkout or file checkout --
> >      you can figure it out by examining the command line but
> >      that already is part of the processing git-checkout does
> >      anyway, so no need to force duplicating that code in the
> >      userland).
>
> It would not be trivial for a wrapper script to figure out what branches
> and commits are about to be pushed.  But "git push" could tell the hook
> script what branches are to be pushed.  And if the pre-push hook is run
> after negotiation between client and server of what commits need to be
> transfered, the hook could also be provided that information and use it
> to figure out which commits it should vet.
>
>
> Although a pre-receive script on the remote repository could do most of
> the same things as a pre-push script, the latter would sometimes have
> advantages because it is run "client-side":
>
> * When the user doesn't have the ability to change the pre-receive
> script on the server (think public git hosting services).

Yeah, and for my case I would like to sync some binaries to a remote
storage before pushing the commits that contains references to these.
I know a wrapper script could easily do this, but I'm not to sure that it will
work in an organization with a lot of developers. Sooner or later one of them
will have some git problem and search the net, find an answer which (of
course) doesn't include the commands in the wrapper script. Then he might
end up pushing something that none of the other devs can get hold of and
that might cause builds to break etc.
And we can't currently run pre-receive hooks to deny the commit.

> * For user-specific actions that are not wanted by all users pushing to
> the same server (for example, maybe I add the string "WIP" to commit
> messages for commits that are not ready to be pushed; my pre-push script
> could verify that I don't push such a commit by accident).
>
> * Preventing "secret" info (password files, proprietary branches) from
> being pushed.  Even if the remote repo were taught to reject them, they
> would have already traversed the internet.

Yes we have also seen a couple of these, would be nice to have a way
do deny them.

-Aske