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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



From: "Junio C Hamano" <gitster@xxxxxxxxx>
David Cowden <dcow90@xxxxxxxxx> 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@xxxxxxxxx>
---

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]

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]