Quantcast

[PATCH] add a 'pre-push' hook

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] add a 'pre-push' hook

Scott Chacon
This commit adds support for a 'pre-push' hook that can be called before
a `git push` command.

It takes no arguments currently, but if the .git/hooks/pre-push script
exists and is executable, it will be called before the 'git push' command
and will stop the push process if it does not exit with a 0 status.

This hook can be overridden by passing in the --no-verify or -n option to
git push.  Documentation and tests have been updated to reflect the change.

Signed-off-by: Scott Chacon <[hidden email]>
---
 Documentation/git-push.txt |   11 +++-
 builtin-push.c             |   27 +++++++++-
 t/t5550-pre-push-hook.sh   |  132 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 168 insertions(+), 2 deletions(-)
 create mode 100644 t/t5550-pre-push-hook.sh

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 45c9643..2b504b3 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git push' [--all] [--dry-run] [--tags] [--receive-pack=<git-receive-pack>]
-          [--repo=all] [-f | --force] [-v | --verbose]
+          [--repo=all] [-f | --force] [-v | --verbose] [-n | --no-verify]
          [<repository> <refspec>...]

 DESCRIPTION
@@ -111,6 +111,10 @@ nor in any Push line of the corresponding remotes
file---see below).
       transfer spends extra cycles to minimize the number of
       objects to be sent and meant to be used on slower connection.

+--no-verify::
+       This option bypasses the pre-push hook.
+       See also linkgit:githooks[5].
+
 -v::
 --verbose::
       Run verbosely.
@@ -193,6 +197,11 @@ git push origin master:refs/heads/experimental::
       needed to create a new branch or tag in the remote repository when
       the local name and the remote name are different; otherwise,
       the ref name on its own will work.
+
+HOOKS
+-----
+This command can run the `pre-push` hook.
+See linkgit:githooks[5] for more information.

 Author
 ------
diff --git a/builtin-push.c b/builtin-push.c
index c1ed68d..f63de9f 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -10,11 +10,12 @@
 #include "parse-options.h"

 static const char * const push_usage[] = {
-       "git push [--all | --mirror] [--dry-run] [--tags]
[--receive-pack=<git-receive-pack>] [--repo=all] [-f | --force] [-v]
[<repository> <refspec>...]",
+       "git push [--all | --mirror] [--dry-run] [--tags]
[--receive-pack=<git-receive-pack>] [--repo=all] [-f | --force] [-n |
--no-verify] [-v] [<repository> <refspec>...]",
       NULL,
 };

 static int thin;
+static int skiphook;
 static const char *receivepack;

 static const char **refspec;
@@ -98,6 +99,24 @@ static int do_push(const char *repo, int flags)
       return !!errs;
 }

+static int run_hook(const char *name)
+{
+       struct child_process hook;
+       const char *argv[1];
+
+       argv[0] = git_path("hooks/%s", name);
+
+       if (access(argv[0], X_OK) < 0)
+               return 0;
+
+       memset(&hook, 0, sizeof(hook));
+       hook.argv = argv;
+       hook.no_stdin = 1;
+       hook.stdout_to_stderr = 1;
+
+       return run_command(&hook);
+}
+
 int cmd_push(int argc, const char **argv, const char *prefix)
 {
       int flags = 0;
@@ -115,6 +134,7 @@ int cmd_push(int argc, const char **argv, const
char *prefix)
               OPT_BIT( 0 , "dry-run", &flags, "dry run",
TRANSPORT_PUSH_DRY_RUN),
               OPT_BIT('f', "force", &flags, "force updates",
TRANSPORT_PUSH_FORCE),
               OPT_BOOLEAN( 0 , "thin", &thin, "use thin pack"),
+               OPT_BOOLEAN('n', "no-verify", &skiphook, "skip pre-push hook"),
               OPT_STRING( 0 , "receive-pack", &receivepack,
"receive-pack", "receive pack program"),
               OPT_STRING( 0 , "exec", &receivepack, "receive-pack",
"receive pack program"),
               OPT_END()
@@ -130,6 +150,11 @@ int cmd_push(int argc, const char **argv, const
char *prefix)
               set_refspecs(argv + 1, argc - 1);
       }

+       if (!skiphook && run_hook("pre-push")) {
+               fprintf(stderr, "pre-push script failed: exiting\n");
+               return 128;
+       }
+
       rc = do_push(repo, flags);
       if (rc == -1)
               usage_with_options(push_usage, options);
diff --git a/t/t5550-pre-push-hook.sh b/t/t5550-pre-push-hook.sh
new file mode 100644
index 0000000..f3c9cce
--- /dev/null
+++ b/t/t5550-pre-push-hook.sh
@@ -0,0 +1,132 @@
+#!/bin/sh
+
+test_description='pre-push hook'
+
+. ./test-lib.sh
+
+D=`pwd`
+
+mk_repo_pair () {
+       rm -rf master mirror &&
+       mkdir mirror &&
+       (
+               cd mirror &&
+               git init
+       ) &&
+       mkdir master &&
+       (
+               cd master &&
+               git init &&
+               git remote add $1 up ../mirror
+       )
+}
+
+test_expect_success 'with no hook' '
+       mk_repo_pair &&
+       (
+               cd master &&
+               echo one >foo && git add foo && git commit -m one &&
+               git push --mirror up
+       )
+'
+
+test_expect_success '--no-verify with no hook' '
+       mk_repo_pair &&
+       (
+               cd master &&
+               echo one >foo && git add foo && git commit -m one &&
+               git push --no-verify --mirror up
+       )
+'
+
+# now install hook that always succeeds
+HOOKDIR="master/.git/hooks"
+HOOK="$HOOKDIR/pre-push"
+mk_hook_exec () {
+       mkdir -p "$HOOKDIR"
+cat > "$HOOK" <<EOF
+#!/bin/sh
+exit 0
+EOF
+       chmod +x "$HOOK"
+}
+
+test_expect_success 'with succeeding 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 '--no-verify with succeeding 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
+       )
+'
+
+# now a hook that fails
+mk_hook_fail () {
+cat > "$HOOK" <<EOF
+#!/bin/sh
+echo 'test run'
+exit 1
+EOF
+       chmod +x "$HOOK"
+}
+
+test_expect_success 'with failing 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 '--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
+       )
+'
+
+chmod -x "$HOOK"
+mk_hook_no_exec () {
+cat > "$HOOK" <<EOF
+#!/bin/sh
+echo 'test run'
+exit 0
+EOF
+}
+
+test_expect_success 'with non-executable 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 '--no-verify with non-executable 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.6.0.GIT
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Jeff King
On Tue, Aug 19, 2008 at 11:55:27AM -0700, Scott Chacon wrote:

> This commit adds support for a 'pre-push' hook that can be called before
> a `git push` command.
>
> It takes no arguments currently, but if the .git/hooks/pre-push script
> exists and is executable, it will be called before the 'git push' command
> and will stop the push process if it does not exit with a 0 status.
>
> This hook can be overridden by passing in the --no-verify or -n option to
> git push.  Documentation and tests have been updated to reflect the change.

Would you care to describe what this is useful for (either in
documentation, to help potential users, or at least in the commit log,
so we know there is a need that is not otherwise fulfilled)?

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Scott Chacon
If the patch is acceptable, I will update the githooks doc with more
information, but we would like this so that you could add a hook that
runs your automated tests before a push would go through.

Scott

On Tue, Aug 19, 2008 at 11:58 AM, Jeff King <[hidden email]> wrote:

> On Tue, Aug 19, 2008 at 11:55:27AM -0700, Scott Chacon wrote:
>
>> This commit adds support for a 'pre-push' hook that can be called before
>> a `git push` command.
>>
>> It takes no arguments currently, but if the .git/hooks/pre-push script
>> exists and is executable, it will be called before the 'git push' command
>> and will stop the push process if it does not exit with a 0 status.
>>
>> This hook can be overridden by passing in the --no-verify or -n option to
>> git push.  Documentation and tests have been updated to reflect the change.
>
> Would you care to describe what this is useful for (either in
> documentation, to help potential users, or at least in the commit log,
> so we know there is a need that is not otherwise fulfilled)?
>
> -Peff
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Jeff King
On Tue, Aug 19, 2008 at 12:00:38PM -0700, Scott Chacon wrote:

> If the patch is acceptable, I will update the githooks doc with more
> information, but we would like this so that you could add a hook that
> runs your automated tests before a push would go through.

I think the common wisdom has been that such tests should be done on the
_receiving_ end, since that makes a more trustworthy enforcement point.
E.g., I know that crap can't get into my central repo because a hook
checks everything coming in. But if a developer has turned off his
pre-push hook (or accidentally failed to enable it), he can still send
crap.

One other argument I have seen is that, to prevent the proliferation of
hooks, the rule is not to add a hook that could just as easily be done
as a sequence of commands. IOW, what's wrong with

  run_my_automated_tests && git push

?

Off the top of my head, I guess the response to those two arguments
would be:

 - sometimes the receiving end isn't set up to run tests, which means it
   is more reasonable to do it on the sending side

 - it's more convenient to just type "git push" than to remember "tests
   && git push", so this reduces the chances of contributors
   accidentally pushing crap

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Junio C Hamano
In reply to this post by Scott Chacon
"Scott Chacon" <[hidden email]> writes:

> If the patch is acceptable, I will update the githooks doc with more
> information, but we would like this so that you could add a hook that
> runs your automated tests before a push would go through.

I've said this number of times on this list but I guess it has been while
since the last time I said it.

"If the patch is acceptable, I'll document it" is the last thing we as
reviewers would want to hear from the submitter, *unless* there is an
ongoing discussion that already have established what is wanted and a
patch came as "ok, here is a possible solution, what do you guys think?".

If the original submitter does not care enough to defend why it is needed,
why should reviewers spend their precious brain cycles to decipher what it
does, guess what situation the change would help, and determine if the
change actually would help the situation it might be trying to help (and
risk wasting all this work because they guessed the motivation wrong)?
And what assurance would we have that the change will be maintained and
supported?

Having said that, I would agree "validate and potentially stop before
pushing" is a very good thing to have.

It is still unclear at this point what kind of input that validation would
want to base its decision on.  At least we would want what branch is being
pushed (so that a validation failure on a branch that is not being pushed
would not interfere), and possibly where you are pushing to (so that you
can still push a change you would want to verify and potentially polish on
a different test/dev box without getting interfered).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Scott Chacon
On Tue, Aug 19, 2008 at 12:39 PM, Junio C Hamano <[hidden email]> wrote:

> "Scott Chacon" <[hidden email]> writes:
>
>> If the patch is acceptable, I will update the githooks doc with more
>> information, but we would like this so that you could add a hook that
>> runs your automated tests before a push would go through.
>
> I've said this number of times on this list but I guess it has been while
> since the last time I said it.
>
> "If the patch is acceptable, I'll document it" is the last thing we as
> reviewers would want to hear from the submitter, *unless* there is an
> ongoing discussion that already have established what is wanted and a
> patch came as "ok, here is a possible solution, what do you guys think?".
>
> If the original submitter does not care enough to defend why it is needed,
> why should reviewers spend their precious brain cycles to decipher what it
> does, guess what situation the change would help, and determine if the
> change actually would help the situation it might be trying to help (and
> risk wasting all this work because they guessed the motivation wrong)?
> And what assurance would we have that the change will be maintained and
> supported?

I didn't say, "if it's acceptable, I'll document it", I said I would add more
information into the githooks doc. I updated the git-push.txt doc and explained
what it did with the commit message and sent test cases.  There should have
been more than enough information on what it did in the message.

Then I sent a patch to githooks with use cases like 5 minutes later.  I'm more
than happy to defend it and replied to Jeffs email almost immediately.

> Having said that, I would agree "validate and potentially stop before
> pushing" is a very good thing to have.
>
> It is still unclear at this point what kind of input that validation would
> want to base its decision on.  At least we would want what branch is being
> pushed (so that a validation failure on a branch that is not being pushed
> would not interfere), and possibly where you are pushing to (so that you
> can still push a change you would want to verify and potentially polish on
> a different test/dev box without getting interfered).

I would be happy to add the name of the branch being updated and the remote
we're trying to push to.  Is there interest then, in the patch?
Should I spend my
precious brain cycles on adding that functionality?

Scott
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Shawn Pearce
In reply to this post by Jeff King
Jeff King <[hidden email]> wrote:

> I think the common wisdom has been that such tests should be done on the
> _receiving_ end, since that makes a more trustworthy enforcement point.
> E.g., I know that crap can't get into my central repo because a hook
> checks everything coming in. But if a developer has turned off his
> pre-push hook (or accidentally failed to enable it), he can still send
> crap.
>
> One other argument I have seen is that, to prevent the proliferation of
> hooks, the rule is not to add a hook that could just as easily be done
> as a sequence of commands. IOW, what's wrong with
>
>   run_my_automated_tests && git push

Yup, I agree completely.

Why not just setup an alias:

        git config alias.send '! run_my_tests && git push "$@"'

and retrain your fingers to use "git send ..."?
 
--
Shawn.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Scott Chacon
On Tue, Aug 19, 2008 at 12:59 PM, Shawn O. Pearce <[hidden email]> wrote:

> Jeff King <[hidden email]> wrote:
>> I think the common wisdom has been that such tests should be done on the
>> _receiving_ end, since that makes a more trustworthy enforcement point.
>> E.g., I know that crap can't get into my central repo because a hook
>> checks everything coming in. But if a developer has turned off his
>> pre-push hook (or accidentally failed to enable it), he can still send
>> crap.
>>
>> One other argument I have seen is that, to prevent the proliferation of
>> hooks, the rule is not to add a hook that could just as easily be done
>> as a sequence of commands. IOW, what's wrong with
>>
>>   run_my_automated_tests && git push
>
> Yup, I agree completely.
>
> Why not just setup an alias:
>
>        git config alias.send '! run_my_tests && git push "$@"'
>
> and retrain your fingers to use "git send ..."?
>
> --
> Shawn.

Sorry, but couldn't this argument be made about any of the hooks run
after manual operations?  ie: pre-commit, pre-applypatch, commit-msg,
post-commit, post-applypatch?  I mean, couldn't you do :

git config alias.docommit '! do_pre_commit && git commit ...' ?

I thought the point of these kind of hooks was to make stuff like this
automatic and easy to standardize for a project, so people working on
a dozen git repos don't have to remember all the aliases they set up
in each one.

Scott
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Nanako Shiraishi
Quoting Scott Chacon <[hidden email]>:

> On Tue, Aug 19, 2008 at 12:59 PM, Shawn O. Pearce <[hidden email]> wrote:
>> Jeff King <[hidden email]> wrote:
>>>
>>> One other argument I have seen is that, to prevent the proliferation of
>>> hooks, the rule is not to add a hook that could just as easily be done
>>> as a sequence of commands. IOW, what's wrong with
>>>
>>>   run_my_automated_tests && git push
>>
>> Yup, I agree completely.
>>
>> Why not just setup an alias:
>>
>>        git config alias.send '! run_my_tests && git push "$@"'
>>
>> and retrain your fingers to use "git send ..."?
>
> Sorry, but couldn't this argument be made about any of the hooks run
> after manual operations?  ie: pre-commit, pre-applypatch, commit-msg,
> post-commit, post-applypatch?  I mean, couldn't you do :
>
> git config alias.docommit '! do_pre_commit && git commit ...' ?
>
> I thought the point of these kind of hooks was to make stuff like this
> automatic and easy to standardize for a project, so people working on
> a dozen git repos don't have to remember all the aliases they set up
> in each one.

This topic seems to come up every once in a while.

 http://thread.gmane.org/gmane.comp.version-control.git/70781/focus=71069
 http://thread.gmane.org/gmane.comp.version-control.git/79306/focus=79321

Somebody needs to describe the general rules in SubmittingPatches, perhaps?

I do not understand why Junio said he thinks this pre-push hook is a good idea.  This clearly is "you always would want to do before running a git command" case.

--
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Scott Chacon
On Tue, Aug 19, 2008 at 2:26 PM, しらいしななこ <[hidden email]> wrote:

> Quoting Scott Chacon <[hidden email]>:
>
>> On Tue, Aug 19, 2008 at 12:59 PM, Shawn O. Pearce <[hidden email]> wrote:
>>> Jeff King <[hidden email]> wrote:
>>>>
>>>> One other argument I have seen is that, to prevent the proliferation of
>>>> hooks, the rule is not to add a hook that could just as easily be done
>>>> as a sequence of commands. IOW, what's wrong with
>>>>
>>>>   run_my_automated_tests && git push
>>>
>>> Yup, I agree completely.
>>>
>>> Why not just setup an alias:
>>>
>>>        git config alias.send '! run_my_tests && git push "$@"'
>>>
>>> and retrain your fingers to use "git send ..."?
>>
>> Sorry, but couldn't this argument be made about any of the hooks run
>> after manual operations?  ie: pre-commit, pre-applypatch, commit-msg,
>> post-commit, post-applypatch?  I mean, couldn't you do :
>>
>> git config alias.docommit '! do_pre_commit && git commit ...' ?
>>
>> I thought the point of these kind of hooks was to make stuff like this
>> automatic and easy to standardize for a project, so people working on
>> a dozen git repos don't have to remember all the aliases they set up
>> in each one.
>
> This topic seems to come up every once in a while.
>
>  http://thread.gmane.org/gmane.comp.version-control.git/70781/focus=71069
>  http://thread.gmane.org/gmane.comp.version-control.git/79306/focus=79321
>
> Somebody needs to describe the general rules in SubmittingPatches, perhaps?
>
> I do not understand why Junio said he thinks this pre-push hook is a good idea.  This clearly is "you always would want to do before running a git command" case.
>
> --
> Nanako Shiraishi
> http://ivory.ap.teacup.com/nanako3/
>

I don't think I understand how this is different than 'pre-commit'
(or, alternatively, how this does not fall under #1 in that list).  If
the script exits non-0, it stops the push, isn't that exactly what
pre-commit does, but with 'push' instead of 'commit'?

Scott
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Junio C Hamano
"Scott Chacon" <[hidden email]> writes:

> On Tue, Aug 19, 2008 at 2:26 PM, しらいしななこ <[hidden email]> wrote:
>> Quoting Scott Chacon <[hidden email]>:
>>
>>> I thought the point of these kind of hooks was to make stuff like this
>>> automatic and easy to standardize for a project, so people working on
>>> a dozen git repos don't have to remember all the aliases they set up
>>> in each one.
>>
>> This topic seems to come up every once in a while.
>>
>>  http://thread.gmane.org/gmane.comp.version-control.git/70781/focus=71069
>>  http://thread.gmane.org/gmane.comp.version-control.git/79306/focus=79321
>>
>> Somebody needs to describe the general rules in SubmittingPatches, perhaps?
>>
>> I do not understand why Junio said he thinks this pre-push hook is a good idea.  This clearly is "you always would want to do before running a git command" case.
>
> I don't think I understand how this is different than 'pre-commit'
> (or, alternatively, how this does not fall under #1 in that list).  If
> the script exits non-0, it stops the push, isn't that exactly what
> pre-commit does, but with 'push' instead of 'commit'?

[jc: trimmed excessive quotes --- please don't quote e-mail sigs]

The primary reason I said it would be a good thing to have is that it
could be common enough.

On one hand, the fact that this pre-push proposal came very late after
everybody used "git push" for eternity might mean that this is not common
requirement at all, and the wrapper approach Shawn and Jeff suggested may
be the right thing to do for minorities who want it.

The pre-commit hook has a good reason behind its existence than merely
being a "pre-something" hook that interferes.  If you only think about
"git commit -a" run by the end user, yes, the whole working tree can be
validated by your wrapper script before making the commit without any need
for a hook, but the user can also say "git commit this-path-only" and give
other options, and at that point, a wrapper approach would not fly well
unless your wrapper simulates what the underlying "git commit" would do
given the set of parameters.

Similarly, a pre-push hook, if done correctly, needs to see what is about
to be pushed (e.g. the user may only say "git push" without saying where
to push to and what ref to update with which commit) to base its
validation decision on, but that cannot be easily checked without actually
simulating the push.  IOW, it has criteria (2) component in it as well,
just like pre-commit hook does.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Sam Vilain
In reply to this post by Jeff King
Jeff King wrote:
>> If the patch is acceptable, I will update the githooks doc with more
>> information, but we would like this so that you could add a hook that
>> runs your automated tests before a push would go through.
>
> I think the common wisdom has been that such tests should be done on the
> _receiving_ end, since that makes a more trustworthy enforcement point.

Probably true, but if someone wants to arrange it the other way around,
what harm is there in that?

Sam
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Junio C Hamano
In reply to this post by Scott Chacon
"Scott Chacon" <[hidden email]> writes:

> I would be happy to add the name of the branch being updated and the remote
> we're trying to push to.  Is there interest then, in the patch?

I do not see a fundamental reason not to have a pre-push hook that
inspects what is going to be pushed where and declines it.  We already do
"don't push non-fast-forward" on the sending end, and we can think of this
as an exhancement of that.

IOW, I am Ok with the goal.  I haven't looked at the implementation,
though. I seem to recall all of tabstops in the patch were lost somewhere
between your editor and my MUA to make the patch unappliable, and stopped
reading there.

> Should I spend my
> precious brain cycles on adding that functionality?

Well, submitters pushing changes to scratch their own itch don't say "my
precious brain cycles".  Reviewers working with submitter to polish and
accept the change to help the submitter, even when it is not their own
itch, do ;-).

In any case, I think passing the information necessary for the validation
to the hook is essential to make this patch worthwhile.  Without it, I
have to agree with Jeff and Shawn that it is no better than a wrapper.

I merely raised destination URL and the tip that is sent as an example,
but I suspect we may also want to know the name of branch on the remote
side that is being updated, and the old tip commit on that branch as well.

The real issue about "that functionality" is what kind of information is
often needed and/or is enough, and we may need to look at what kind of
validation are useful in expected use scenarios.

For example, if your goal is "to enforce that the tip of the tree is
always without compilation errors", then you only need the commit that you
are sending.  If however that policy applies only to some branches but not
other, you would need to know which branch you are sending things to.  If
you are scanning what is sent to the remote side so that your published
history does not leak confidential information, you further need to know
the old tip and run "rev-list --objects old..new" and inspect all the new
objects you are sending, not just the tip.

I am just listing these off the top of my head; you as the original
submitter of the patch must have thought about the issues and use cases
much deeper than I did in the past 7 minutes while I am typing this
message, so you are in a better position to come up with various use cases
and the set of parameters the hook would need to do its validation job.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Shawn Pearce
Junio C Hamano <[hidden email]> wrote:
> "Scott Chacon" <[hidden email]> writes:
>
> > I would be happy to add the name of the branch being updated and the remote
> > we're trying to push to.  Is there interest then, in the patch?
>
> [...] so you are in a better position to come up with various use cases
> and the set of parameters the hook would need to do its validation job.

And please be careful with the term "parameters" here.

We not too long ago changed receive-pack to use stdin into a hook
so we don't have to worry about command line length limitations.
Once we start talking about something like push or fetch which can
operate on a huge batch of refs at one shot we really should avoid
the command line limits if we can.

--
Shawn.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] add a 'pre-push' hook

Jeff King
In reply to this post by Sam Vilain
On Wed, Aug 20, 2008 at 10:26:48AM +1200, Sam Vilain wrote:

> > I think the common wisdom has been that such tests should be done on the
> > _receiving_ end, since that makes a more trustworthy enforcement point.
>
> Probably true, but if someone wants to arrange it the other way around,
> what harm is there in that?

Read the rest of the message you are quoting where I say basically that.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Loading...