Re: [PATCH v2 2/3] push: Add support for pre-push hooks

2013-01-14 Thread Junio C Hamano
Junio C Hamano  writes:

> At least the attached patch is necessary.

Sorry, but the last hunk (see below) is not.  It breaks the hook.

> In the longer term, we may want to discuss what should happen when
> the hook exited without even reading what we fed.  My gut feeling is
> that we can still trust its exit status (a hook that was badly coded
> so it wanted to read from us and use that information to decide but
> somehow died before fully reading from us is not likely to exit with
> zero status, so we wouldn't diagnosing breakage as a success), but
> there may be downsides for being that lax.
>
> If we decide we want to be lax, then the call site of this hook and
> the pre-receive hook (is there any other "take info from the
> standard input" hook?) need to be modified so that they ignore
> sigpipe, I think.
>
> There was a related discussion around this issue about a year ago.
>
> http://thread.gmane.org/gmane.comp.version-control.git/180346/focus=186291
> ...
>

> @@ -38,6 +40,7 @@ COMMIT2="$(git rev-parse HEAD)"
>  export COMMIT2
>  
>  write_script "$HOOK" <<'EOF'
> +cat >/dev/null
>  echo "$1" >actual
>  echo "$2" >>actual
>  cat >>actual

As this one wants to keep the incoming data to "actual", we do not
want the extra "cat" to slurp everything in.  Sorry for not being
careful.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 2/3] push: Add support for pre-push hooks

2013-01-14 Thread Junio C Hamano
Aaron Schrab  writes:

>  t/t5571-pre-push-hook.sh   | 129 
> +
> diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
> new file mode 100755
> index 000..d68fed7
> --- /dev/null
> +++ b/t/t5571-pre-push-hook.sh
> @@ -0,0 +1,129 @@
> +#!/bin/sh
> +
> +test_description='check pre-push hooks'
> +. ./test-lib.sh
> +
> +# Setup hook that always succeeds
> +HOOKDIR="$(git rev-parse --git-dir)/hooks"
> +HOOK="$HOOKDIR/pre-push"
> +mkdir -p "$HOOKDIR"
> +write_script "$HOOK" < +exit 0
> +EOF

As this script is expected to read from the pipe, if this exits
before the parent has a chance to write to the pipe, the parent can
be killed with sigpipe.

At least the attached patch is necessary.

In the longer term, we may want to discuss what should happen when
the hook exited without even reading what we fed.  My gut feeling is
that we can still trust its exit status (a hook that was badly coded
so it wanted to read from us and use that information to decide but
somehow died before fully reading from us is not likely to exit with
zero status, so we wouldn't diagnosing breakage as a success), but
there may be downsides for being that lax.

If we decide we want to be lax, then the call site of this hook and
the pre-receive hook (is there any other "take info from the
standard input" hook?) need to be modified so that they ignore
sigpipe, I think.

There was a related discussion around this issue about a year ago.

http://thread.gmane.org/gmane.comp.version-control.git/180346/focus=186291


 t/t5571-pre-push-hook.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
index d68fed7..577d252 100755
--- a/t/t5571-pre-push-hook.sh
+++ b/t/t5571-pre-push-hook.sh
@@ -8,6 +8,7 @@ HOOKDIR="$(git rev-parse --git-dir)/hooks"
 HOOK="$HOOKDIR/pre-push"
 mkdir -p "$HOOKDIR"
 write_script "$HOOK" /dev/null
 echo "$1" >actual
 echo "$2" >>actual
 cat >>actual



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 2/3] push: Add support for pre-push hooks

2013-01-14 Thread Junio C Hamano
Aaron Schrab  writes:

> Add support for a pre-push hook which can be used to determine if the
> set of refs to be pushed is suitable for the target repository.  The
> hook is run with two arguments specifying the name and location of the
> destination repository.
>
> Information about what is to be pushed is provided by sending lines of
> the following form to the hook's standard input:
>
>SP  SP  SP  LF
>
> If the hook exits with a non-zero status, the push will be aborted.
>
> This will allow the script to determine if the push is acceptable based
> on the target repository and branch(es), the commits which are to be
> pushed, and even the source branches in some cases.
>
> Signed-off-by: Aaron Schrab 
> ---
>  Documentation/githooks.txt |  29 ++
>  builtin/push.c |   1 +
>  t/t5571-pre-push-hook.sh   | 129 
> +
>  transport.c|  60 +
>  transport.h|   1 +
>  5 files changed, 220 insertions(+)
>  create mode 100755 t/t5571-pre-push-hook.sh
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index b9003fe..d839233 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -176,6 +176,35 @@ 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
> +
> +
> +This hook is called by 'git push' and can be used to prevent a push from 
> taking
> +place.  The hook is called with two parameters which provide the name and
> +location of the destination remote, if a named remote is not being used both
> +values will be the same.
> +
> +Information about what is to be pushed is provided on the hook's standard
> +input with lines of the form:
> +
> +   SP  SP  SP  LF
> +
> +For instance, if the command +git push origin master:foreign+ were run the

Just being curious, but why use +monospace text+ here?  Most of the
new text use `monospace text literally` instead in this patch.

> +hook would receive a line like the following:
> +
> +  refs/heads/master 67890 refs/heads/foreign 12345
> +
> +although the full, 40-character SHA1s would be supplied.

Perhaps ellipses are called for here?

refs/heads/master 67890... refs/heads/foreign 12345...

 (the above abbreviates full 40-hexdigits for illustration purposes only)

> +If the foreign ref
> +does not yet exist the `` will be 40 `0`.  If a ref is to be
> +deleted, the `` will be supplied as `(delete)` and the ` +SHA1>` will be 40 `0`.  If the local commit was specified by something other
> +than a name which could be expanded (such as `HEAD~`, or a SHA1) it will be
> +supplied as it was originally given.
> +
> +If this hook exits with a non-zero status, 'git push' will abort without
> +pushing anything.  Information about why the push is rejected may be sent
> +to the user by writing to standard error.

s/standard error/& of the hook/; perhaps?  It is unclear who does
the writing and it can be misunderstood that git-push will write to
standard error upon seeing your hook that silently exits.

> diff --git a/builtin/push.c b/builtin/push.c
> index 8491e43..b158028 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -407,6 +407,7 @@ int cmd_push(int argc, const char **argv, const char 
> *prefix)
>   OPT_BOOL(0, "progress", &progress, N_("force progress 
> reporting")),
>   OPT_BIT(0, "prune", &flags, N_("prune locally removed refs"),
>   TRANSPORT_PUSH_PRUNE),
> + OPT_BIT(0, "no-verify", &flags, N_("bypass pre-push hook"), 
> TRANSPORT_PUSH_NO_HOOK),
>   OPT_END()
>   };

So to countermand this, you have to say --no-no-verify?  Wouldn't it
be more natural to introduce a --verify option that turns the bit
on, which automatically gives you --no-verify to turn it off?  A
bit in a flag word can be initialized to true before the flag word
is given to the parse_options() machinery to make the field default
to true, no?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html