Re: [PATCH v3] Clarify pre-push hook documentation

[David Cowden <dcow90@gmail.com>]

Hey guys, I was on vacation for a little over a week, I'll be back on
this this coming week (haven't forgotten).

David

On Wed, Mar 26, 2014 at 4:21 PM, Philip Oakley <philipoakley@iee.org> wrote:
> From: "Junio C Hamano" <gitster@pobox.com>
>>
>> David Cowden <dcow90@gmail.com> writes:
>>
>>
>>> The documentation as-is does not mention that the pre-push hook is
>>> executed even when there is nothing to push.  This can lead a new
>>> reader to believe there will always be lines fed to the script's
>>> standard input and cause minor confusion as to what is happening
>>> when there are no lines provided to the pre-push script.
>>>
>>> Signed-off-by: David Cowden <dcow90@gmail.com>
>>> ---
>>>
>>> Notes:
>>>     I'm not sure if I've covered every case here.  If there are more
>>> cases to
>>>     consider, please let me know and I can update to include them.
>>
>>
>> I do not think of any offhand, but a more important point that I was
>> trying to get at was that we should not give an incorrect impression
>> to the readers that the scenario that is described is the only case
>> they need to be worried about by pretending to be exhaustive.
>>
>> The "may" in your wording "This may happen when" may be good enough
>> to hint that these may not be the only cases.
>>
>>>     c.f.
>>>
>>> http://stackoverflow.com/questions/22585091/git-hooks-pre-push-script-does-not-receive-input-via-stdin
>>>
>>>  Documentation/githooks.txt | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
>>> index d954bf6..1fd6da9 100644
>>> --- a/Documentation/githooks.txt
>>> +++ b/Documentation/githooks.txt
>>> @@ -203,6 +203,15 @@ 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.
>>>
>>> +The hook is executed regardless of whether changes will actually be
>>> pushed or
>>> +not.  This may happen when 'git push' is called and:
>>> +
>>> + - the remote ref is already up to date, or
>>> + - pushing to the remote ref cannot be handled by a simple
>>> fast-forward
>>> +
>>> +In other words, the script is called for every push.  In the event
>>> that nothing
>>> +is to be pushed, no data will be provided on the script's standard
>>> input.
>
>
> Doesn't an 'in other words' indicate it could be further tightened?
> Maybe
>    "If there is nothing to push, the hook will still run, but the input
>    line will be empty.
>
>    Likewise the hook will still run for other cases such as:
>
>    - the remote ref is already up to date,
>    - pushing to the remote ref cannot be handled by a simple
>      fast-forward,
>    - etc."
>
>
>>
>> When two things are to be pushed, the script will see the two
>> things.  When one thing is to be pushed, the script will see the one
>> thing.  When no thing is to be pushed, the script will see no thing
>> on its standard input.
>>
>> But isn't that obvious?  I still wonder if we really need to single
>> out that "nothing" case.  The more important thing is that it is
>> invoked even in the "0-thing pushed" case, and "the list of things
>> pushed that is given to the hook happens to be empty" is an obvious
>> natural fallout.
>
>
> Personally I think it should be mentioned in that paragraph, which is
> covering all the various special cases. The 'nothing' case often causes
> confusion when it's not specified in documentation.
>>
>>
>>>  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.
>>
>> --
>
>
> It may be that the documentation should include the caveat
>
>    "Hooks, when enabled, are executed unconditionally by their calling
>    functions.
>     Script writers should ensure they handle all conditions."
>
> somewhere near the top of the page to cover all hooks, which IIRC
> started David's journey. That would allow my second paragraph
> "Likewise.." to be dropped.
>
> Philip
> --
> [apologies for any whitespace damage]
>