Documentation of post-receive hook

Documentation of post-receive hook

[Christoph Michelbach]

Hi,

I think the documentation of the post-receive hook is misleading. When reading
it, it appears as though the post-receive hook is executed even when no commits
are transferred by a git push because it isn't mentioned anywhere that this is
necessary for its execution.

This can easily be fixed by changing

    This hook is invoked by 'git-receive-pack' on the remote repository,
    which happens when a 'git push' is done on a local repository.

to:

    This hook is invoked by 'git-receive-pack' on the remote repository,
    which happens when a 'git push' is done on a local repository and
    successfully transfers at least 1 commit.

Alternatively,

    This hook executes once for the receive operation.

can be changed to

    This hook executes once for the receive operation, but only if at least
    1 commit was successfully transferred.

Imho, the first option should be chosen as it informs the reader about this
behavior at the first convenient opportunity.

-- 
Christoph Michelbach <michelbach94@gmail.com>

Re: Documentation of post-receive hook

[Junio C Hamano]

Christoph Michelbach <michelbach94@gmail.com> writes:

> I think the documentation of the post-receive hook is misleading. When reading
> it, it appears as though the post-receive hook is executed even when no commits
> are transferred by a git push because it isn't mentioned anywhere that this is
> necessary for its execution.

In other words, post-receive hook triggers only after it receives
objects.  A mere action of running receive-pack command does not.

> This can easily be fixed by changing
>
>     This hook is invoked by 'git-receive-pack' on the remote repository,
>     which happens when a 'git push' is done on a local repository.

So the existing description is technically correct (i.e. it does
correctly identify who invokes it) but lacks a more interesting and
relevant information (i.e. receive-pack invokes only after receiving
data).

>     This hook is invoked by 'git-receive-pack' on the remote repository,
>     which happens when a 'git push' is done on a local repository and
>     successfully transfers at least 1 commit.

I am not sure "at least 1 commit" is a good phrase to use here.
There are transfer that sends objects but no commit object, and the
above makes it sound as if such a transfer will not trigger the
hook.  Would

	This hook is run by 'git receive-pack' on the remote
	repository, after it receives objects sent by 'git push'.

be clear enough to teach readers that a no-op push that recieve-pack
does not receive any object does not trigger the hook?

Re: Documentation of post-receive hook

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>     This hook is invoked by 'git-receive-pack' on the remote repository,
>>     which happens when a 'git push' is done on a local repository and
>>     successfully transfers at least 1 commit.
>
> I am not sure "at least 1 commit" is a good phrase to use here.
> There are transfer that sends objects but no commit object, and the
> above makes it sound as if such a transfer will not trigger the
> hook.  Would
>
> 	This hook is run by 'git receive-pack' on the remote
> 	repository, after it receives objects sent by 'git push'.
>
> be clear enough to teach readers that a no-op push that recieve-pack
> does not receive any object does not trigger the hook?

Actually I take this back.  Your original observation "only when at
least one commit is transferred" is not even correct.

You can try what I just tried to make sure:

        $ git clone --no-local . ../victim
        $ cat >../victim/.git/hooks/post-receive <<\EOF
        #!/bin/sh
        (
            echo "post receive was here at $(date)"
            cat 
        ) >>../STAMP
        EOF
        $ chmod +x ../victim/.git/hooks/post-receive
        $ git push ../victim master:foo

The last "push" does not transfer any object (and obviously does not
satisfy your "at least 1 commit" requirement), but it does update
the STAMP file.  This is because it updates a ref and that is what
post-receive wants to react to, even if there is no new objects
placed in the receiving repository.

So an updated suggestion for the text would be:

     This hook is invoked by 'git-receive-pack' on the remote repository,
     which happens when a 'git push' is done on a local repository.

Oh, wait.  That is what we already have ;-).

Having said all that, there is one case that running 'git push' does
*NOT* cause 'receive-pack' to be invoked at the other end, and in
that scenario, obviously the hook cannot be run, simply because the
command that would run the hook is not run in the first place.

After the above sequence against the "victim" test repository, you
could try:

        $ git push ../victim master:foo
	Everything up-to-date

and observe that the STAMP file is not updated.

What is happening is that "git push" notices that there is nothing
gained by invoking receive-pack on the other side, because the
branch 'foo' already points at the commit at the tip of our
'master'.

So it might technically be an improvement to update the text to
mention that 'git push' does not necessarily always lead to
invocation of receive-pack, something like:

     This hook is invoked by 'git-receive-pack' on the remote
     repository, which may happen when a 'git push' is done on a
     local repository.

but then that introduces the need to make the reader understand what
"may happen" is trying to say, iow, when does a user 'push' and it
does not trigger receive-pack?

But I do not think teaching that (i.e. when does receive-pack run?)
is the job of this paragraph, whose primary objective is to teach
about this hook that is run when receive-pack is run.  So...

Re: Documentation of post-receive hook

[Christoph Michelbach]

On Fri, 2017-11-17 at 12:24 +0900, Junio C Hamano wrote:
> So an updated suggestion for the text would be:
> 
>      This hook is invoked by 'git-receive-pack' on the remote repository,
>      which happens when a 'git push' is done on a local repository.
> 
> Oh, wait.  That is what we already have ;-).

But this text suggests that the hook is always invoked when a git push to that
repo is done, which is not the case. How about adding "and updates are reference
in the remote repository"?

Re: Documentation of post-receive hook

[Robert P. J. Day]

[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]

On Sun, 19 Nov 2017, Christoph Michelbach wrote:

> On Fri, 2017-11-17 at 12:24 +0900, Junio C Hamano wrote:
> > So an updated suggestion for the text would be:
> > 
> >      This hook is invoked by 'git-receive-pack' on the remote repository,
> >      which happens when a 'git push' is done on a local repository.
> > 
> > Oh, wait.  That is what we already have ;-).
>
> But this text suggests that the hook is always invoked when a git
> push to that repo is done, which is not the case. How about adding
> "and updates are reference in the remote repository"?

  "referenced"?

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

Re: Documentation of post-receive hook

[Christoph Michelbach]

On Sun, 2017-11-19 at 12:42 -0500, Robert P. J. Day wrote:
> On Sun, 19 Nov 2017, Christoph Michelbach wrote:
> 
> > 
> > On Fri, 2017-11-17 at 12:24 +0900, Junio C Hamano wrote:
> > > 
> > > So an updated suggestion for the text would be:
> > >  
> > >       This hook is invoked by 'git-receive-pack' on the remote repository,
> > >       which happens when a 'git push' is done on a local repository.
> > >  
> > > Oh, wait.  That is what we already have ;-).
> > But this text suggests that the hook is always invoked when a git
> > push to that repo is done, which is not the case. How about adding
> > "and updates are reference in the remote repository"?
>   "referenced"?
> 
> rday
> 

Sorry, meant to write "a" instead of "are".

Re: Documentation of post-receive hook

[Junio C Hamano]

Christoph Michelbach <michelbach94@gmail.com> writes:

> On Fri, 2017-11-17 at 12:24 +0900, Junio C Hamano wrote:
>> So an updated suggestion for the text would be:
>> 
>>      This hook is invoked by 'git-receive-pack' on the remote repository,
>>      which happens when a 'git push' is done on a local repository.
>> 
>> Oh, wait.  That is what we already have ;-).
>
> But this text suggests that the hook is always invoked when a git push to that
> repo is done, which is not the case. How about adding "and updates are reference
> in the remote repository"?

Yeah, I think we are moving in the right direction.  Let's step back
a bit and try not to say "remote repository" twice.  

For that matter, I am not sure if local/remote is adding much value,
to be honest.  It has a certain value to clarify that the receive is
happening on the other end of the push (i.e. not in the repository
you push from), but we do not have to use local/remote for that---
and it is better to avoid 'local', that is relative: if you are in
the mindset of thinking about what happens during a receive, the
repository the receive-pack and the hook operates in is 'local' and
not 'remote' (it is only 'remote' if you think you are the one who
is pushing).

How about this rewrite?  Would it consider all the points raised and
make it easier to understand?

    This hook is invoked by 'git-receive-pack' when it reacts to
    'git push' and updates reference(s) in its repository.

Re: Documentation of post-receive hook

[Christoph Michelbach]

On November 20, 2017 2:17:58 AM GMT+01:00, 
>How about this rewrite?  Would it consider all the points raised and
>make it easier to understand?
>
>    This hook is invoked by 'git-receive-pack' when it reacts to
>    'git push' and updates reference(s) in its repository.

I think it's much more intelligible but a hint as to when this happens wouldn't hurt. E.g.: "This does not happen if the user receives the message 'Already up-to-date'." That is if this is correct, of course.

Re: Documentation of post-receive hook

[Junio C Hamano]

Christoph Michelbach <michelbach94@gmail.com> writes:

> On November 20, 2017 2:17:58 AM GMT+01:00, 
>>How about this rewrite?  Would it consider all the points raised and
>>make it easier to understand?
>>
>>    This hook is invoked by 'git-receive-pack' when it reacts to
>>    'git push' and updates reference(s) in its repository.
>
> I think it's much more intelligible but a hint as to when this
> happens wouldn't hurt. E.g.: "This does not happen if the user
> receives the message 'Already up-to-date'." That is if this is
> correct, of course.

Your suggesting to mention that particular message hints at me that
you feel that the users may not necessarily understand that push did
not result in any update of references on the other side when they
see it.  If the message was clear enough to them, "when it reacts to
push and updates" ought to be clear enough description, too.

And if that indeed is the case (and I would not be surprised if it
is, but I suspect that most users are clueful enough), it is not the
documentation, but the "Already up-to-date" message, that needs to
be clarified, no?

Besides, we'd rather not cast the end-user facing message in stone
in the documentation like that (especially when the message has
known room for improvement and will change).

Re: Documentation of post-receive hook

[Christoph Michelbach]

On Wed, 2017-11-22 at 10:14 +0900, Junio C Hamano wrote:
> Your suggesting to mention that particular message hints at me that
> you feel that the users may not necessarily understand that push did
> not result in any update of references on the other side when they
> see it.  If the message was clear enough to them, "when it reacts to
> push and updates" ought to be clear enough description, too.
> 
> And if that indeed is the case (and I would not be surprised if it
> is, but I suspect that most users are clueful enough), it is not the
> documentation, but the "Already up-to-date" message, that needs to
> be clarified, no?
> 
> Besides, we'd rather not cast the end-user facing message in stone
> in the documentation like that (especially when the message has
> known room for improvement and will change).

You're right (with everything you wrote). Your proposed text sounds good.

Re*: Documentation of post-receive hook

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Your suggesting to mention that particular message hints at me that
> you feel that the users may not necessarily understand that push did
> not result in any update of references on the other side when they
> see it.  If the message was clear enough to them, "when it reacts to
> push and updates" ought to be clear enough description, too.
>
> And if that indeed is the case (and I would not be surprised if it
> is, but I suspect that most users are clueful enough), it is not the
> documentation, but the "Already up-to-date" message, that needs to
> be clarified, no?

I do not know if it helps to _also_ clarify the message, but at
least, let's tie the loose end by updating the documentation.

-- >8 --
Subject: hooks doc: clarify when receive-pack invokes its hooks

The text meant to say that receive-pack runs these hooks, and only
because receive-pack is not a command the end users use every day
(ever), as an explanation also meantioned that it is run in response
to 'git push', which is an end-user facing command readers hopefully
know about.

This unfortunately gave an incorrect impression that 'git push'
always result in the hook to run.  If the refs push wanted to update
all already had the desired value, these hooks are not run.  

Explicitly mention "... and updates reference(s)" as a precondition
to avoid this ambiguity.

Helped-by: Christoph Michelbach <michelbach94@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Documentation/githooks.txt | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 9565dc3fda..8f6a3cd63e 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -222,8 +222,8 @@ to the user by writing to standard error.
 pre-receive
 ~~~~~~~~~~~
 
-This hook is invoked by 'git-receive-pack' on the remote repository,
-which happens when a 'git push' is done on a local repository.
+This hook is invoked by 'git-receive-pack' when it reacts to
+'git push' and updates reference(s) in its repository.
 Just before starting to update refs on the remote repository, the
 pre-receive hook is invoked.  Its exit status determines the success
 or failure of the update.
@@ -260,8 +260,8 @@ will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.
 update
 ~~~~~~
 
-This hook is invoked by 'git-receive-pack' on the remote repository,
-which happens when a 'git push' is done on a local repository.
+This hook is invoked by 'git-receive-pack' when it reacts to
+'git push' and updates reference(s) in its repository.
 Just before updating the ref on the remote repository, the update hook
 is invoked.  Its exit status determines the success or failure of
 the ref update.
@@ -305,8 +305,8 @@ unannotated tags to be pushed.
 post-receive
 ~~~~~~~~~~~~
 
-This hook is invoked by 'git-receive-pack' on the remote repository,
-which happens when a 'git push' is done on a local repository.
+This hook is invoked by 'git-receive-pack' when it reacts to
+'git push' and updates reference(s) in its repository.
 It executes on the remote repository once after all the refs have
 been updated.
 
@@ -344,8 +344,8 @@ will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.
 post-update
 ~~~~~~~~~~~
 
-This hook is invoked by 'git-receive-pack' on the remote repository,
-which happens when a 'git push' is done on a local repository.
+This hook is invoked by 'git-receive-pack' when it reacts to
+'git push' and updates reference(s) in its repository.
 It executes on the remote repository once after all the refs have
 been updated.
 
@@ -375,8 +375,8 @@ for the user.
 push-to-checkout
 ~~~~~~~~~~~~~~~~
 
-This hook is invoked by 'git-receive-pack' on the remote repository,
-which happens when a 'git push' is done on a local repository, when
+This hook is invoked by 'git-receive-pack' when it reacts to
+'git push' and updates reference(s) in its repository, and when
 the push tries to update the branch that is currently checked out
 and the `receive.denyCurrentBranch` configuration variable is set to
 `updateInstead`.  Such a push by default is refused if the working