Documentation of post-receive hook

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> writes:

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

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).

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?

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

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...

On Fri, 2017-11-17 at 12:24 +0900, Junio C Hamano wrote:

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"?

On Sun, 19 Nov 2017, Christoph Michelbach wrote:

  "referenced"?

rday

On Sun, 2017-11-19 at 12:42 -0500, Robert P. J. Day wrote:

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

Christoph Michelbach <michelbach94@gmail.com> writes:

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.

On November 20, 2017 2:17:58 AM GMT+01:00, 

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. 

Christoph Michelbach <michelbach94@gmail.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?

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).

On Wed, 2017-11-22 at 10:14 +0900, Junio C Hamano wrote:

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

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

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(-)