Re: pre-push hook does not get input on non-fast-forward pushes

[Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at>]

On Mon, 28 Mar 2016 14:44:20 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at> writes:
> 
> The pre-push hook is not the only thing that may prevent you from
> pushing a ref update.  As you noticed, non-fast-forward check may
> trigger and decide that a ref is not going to be pushed, and that
> may happen before we call the pre-push hook.
> 
>     Information about what is to be pushed is provided on the hook's
>     standard input with lines of the form ...
> 
> So when the pre-push is called, the refs that will not fast-forward
> may not be among the "what is to be pushed", hence not reported.
> 
> We _could_ add something like this to the documentation, but we do
> not necessarily want to promise that the order of checks to stay
> "internal checks like non-ff check first before pre-push hook", so
> this update may not be an improvement.  The current text conveys
> enough information without making such a promise, but you need to
> read it carefully.

I understand but that behavior is quite surprising and the wording not
that clear (if one is not aware of the behavior already) IMHO. If the
fast forward check is done beforehand and no refs remain, why is the
script called at all? Or put otherwise, why isn't git aborting before
that? That would seem way more logical... consistency is a good
argument to call the hook with no refs anyway... but the abort/filter
order is remarkable and should be documented in some way IMHO.

> 
>  Documentation/githooks.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 9ef2469..605ba4d 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -201,7 +201,9 @@ does not yet exist the `<remote SHA-1>` will be 40 `0`.  If a ref is to be
>  deleted, the `<local ref>` will be supplied as `(delete)` and the `<local
>  SHA-1>` 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 SHA-1) it will be
> -supplied as it was originally given.
> +supplied as it was originally given.  A request to update remote ref that has
> +already been rejected for other reasons (e.g. failing to pass a fast-forward
> +test) does not appear in the input.
>  
>  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

LGTM... if you don't want to promise anything then maybe something like
"may not appear" instead of "does not appear" is better. As long as the
reason for no input is stated more explicitly than currently I am happy.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner