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

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

[Stefan Tauner]

Hi,

I noticed that without an additional --force the pre-push hook does not
get any input on stdin if a push would result in non-fast-forward
uploads. This is not a problem per se (although I don't get the
rationale) but it is undocumented and the latter left me puzzled.

(Please keep me in CC since I am not subscribed, thanks)

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

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

[Junio C Hamano]

Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at> writes:

> I noticed that without an additional --force the pre-push hook does not
> get any input on stdin if a push would result in non-fast-forward
> uploads. This is not a problem per se (although I don't get the
> rationale) but it is undocumented and the latter left me puzzled.
>
> (Please keep me in CC since I am not subscribed, thanks)

Thanks (no need to say "please cc me", as that is the norm here).

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.

So I dunno.

 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

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

[Stefan Beller]

On Mon, Mar 28, 2016 at 2:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at> writes:
>
>> I noticed that without an additional --force the pre-push hook does not
>> get any input on stdin if a push would result in non-fast-forward
>> uploads. This is not a problem per se (although I don't get the
>> rationale) but it is undocumented and the latter left me puzzled.
>>
>> (Please keep me in CC since I am not subscribed, thanks)
>
> Thanks (no need to say "please cc me", as that is the norm here).
>
> 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.
>
> So I dunno.
>
>  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 we don't want to give promises, s/does not/ may not/ ?

Although that sounds like it is non-deterministic (some occur, some don't,
maybe it depends on $random_reason)

>
>  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 unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stefan Tauner]

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