Feature Request: provide cmdline args to git hooks

Feature Request: provide cmdline args to git hooks

[Chris O'Kelly]

Hello,
Just a brief note about a feature I would find incredibly useful, were
it available.

A brief background of my use case:
I am wanting to write a pre-push hook to prevent tags being pushed to
our production servers. The production servers in our case are --bare
endpoints, and when we push a tag at them, they always checkout the
commit that tag is attached to via some post-receive magic (WPEngine,
FWIW). This behavior is even present when the tag was already pushed
to the repo previously, if for instance a normal push is made with the
--tags argument. In the past we have had problems when a developer is
using a GUI like SourceTree and accidentally leaves the 'push all
tags' option checked, pushing 100s of tags to the server, which then
dutifully begins checking out each in turn.

Currently I check for tag refs being pushed with:
#...SNIP...
while read local_ref local_sha remote_ref remote_sha
do
local_type=$(echo "$local_ref" | awk -F/ '{print $2}')
remote_type=$(echo "$remote_ref" | awk -F/ '{print $2}')
if [[ "$no_tags" -eq 1 && ($local_type = "tags" || $remote_type = "tags")]]
then
echo "Detected attempt to push tags to a no_tags repo! Exiting without push..."
exit 1
fi
#...SNIP...

which works fine the first time tags are pushed, but when they are
already up to date as of the last fetch, they are not passed to the
stdin for pre-push, so I cannot detect them.

in a linux environment we can inspect /proc/$PPID/cmdline or ps
-ocommand= -p $PPID to find the --tags argument (or any manually
specified tag refs, etc), however commonly developers are using
Windows with SourceTree, and the pseudo-nix environment it provides
lacks a /proc directory and uses the cut down cygwin version of ps. I
have considered going down the parsing route with general ps output,
reading line by line to find the appropriate ppid, then echoing the
output, but varying output and column order of ps between SourceTree
and various linux versions looks to make that infeasible as a portable
solution.

If we could access the original git push commandline inside the hook,
either through a parameter passed directly to the script or possibly a
GIT_* environment variable, it would make this possible. My specific
use case may not be incredibly common, but this could also be used to
check if, for example, a push is being forced in a portable fashion -
something I can see being useful for a pre-push hook in a variety of
circumstances.

Alternatively - am I missing the super easy (and probably super
obvious) way to do this with the existing tools?

Re: Feature Request: provide cmdline args to git hooks

[Junio C Hamano]

"Chris O'Kelly" <chris@mapcreative.com.au> writes:

> A brief background of my use case:
> I am wanting to write a pre-push hook to prevent tags being pushed to
> our production servers. The production servers in our case are --bare
> endpoints, and when we push a tag at them, they always checkout the
> commit that tag is attached to via some post-receive magic (WPEngine,
> FWIW). This behavior is even present when the tag was already pushed
> to the repo previously, if for instance a normal push is made with the
> --tags argument.

Do you mean that you want to forbid some people from pushing tags
into that repository while allowing others (i.e. those who manage
production deployment)?  If that is the goal, it sounds like that
the right place to do this is at the receiving end via pre-receive,
not at the sending end via pre-push.

Re: Feature Request: provide cmdline args to git hooks

[Chris O'Kelly]

Unfortunately in this case we don't have control over the hooks at the
receiving end - we want to prevent tags from being pushed by all users
to these repo's.

On Wed, Apr 15, 2015 at 1:08 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Chris O'Kelly" <chris@mapcreative.com.au> writes:
>
>> A brief background of my use case:
>> I am wanting to write a pre-push hook to prevent tags being pushed to
>> our production servers. The production servers in our case are --bare
>> endpoints, and when we push a tag at them, they always checkout the
>> commit that tag is attached to via some post-receive magic (WPEngine,
>> FWIW). This behavior is even present when the tag was already pushed
>> to the repo previously, if for instance a normal push is made with the
>> --tags argument.
>
> Do you mean that you want to forbid some people from pushing tags
> into that repository while allowing others (i.e. those who manage
> production deployment)?  If that is the goal, it sounds like that
> the right place to do this is at the receiving end via pre-receive,
> not at the sending end via pre-push.
>

Re: Feature Request: provide cmdline args to git hooks

[Junio C Hamano]

"Chris O'Kelly" <chris@mapcreative.com.au> writes:

[administrivia: people read from top to bottom; please do not top
post]

> On Wed, Apr 15, 2015 at 1:08 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> "Chris O'Kelly" <chris@mapcreative.com.au> writes:
>>
>>> A brief background of my use case:
>>> I am wanting to write a pre-push hook to prevent tags being pushed to
>>> our production servers. The production servers in our case are --bare
>>> endpoints, and when we push a tag at them, they always checkout the
>>> commit that tag is attached to via some post-receive magic (WPEngine,
>>> FWIW). This behavior is even present when the tag was already pushed
>>> to the repo previously, if for instance a normal push is made with the
>>> --tags argument.
>>
>> Do you mean that you want to forbid some people from pushing tags
>> into that repository while allowing others (i.e. those who manage
>> production deployment)?  If that is the goal, it sounds like that
>> the right place to do this is at the receiving end via pre-receive,
>> not at the sending end via pre-push.
>>

> Unfortunately in this case we don't have control over the hooks at the
> receiving end - we want to prevent tags from being pushed by all users
> to these repo's.

Well, if you refuse to or are unable to use the mechanism that was
specifically designed to solve your problem, then there is no way we
can offer you a good solution.

Let's set the baseline of the discussion first.

We agree that any client-side mechanism (like the hook) is *not* a
good way to enforce policy or ensure security. It is merely a way to
avoid mistakes---prevent the users to avoid doing something they are
allowed to do (at the mechanical level) that is not desirable to the
project.  After all, the user's hook can be misconfigured (or
disabled) by mistake, or the user's workstation may have an older
version of Git that does not know about the hook, thereby bypassing
whategver you try to do on the client side.  The user can even say
"git push --no-verify".  That is why a true policy-enforcement and
security must be done on the receiving end.

So our conversation must start from the shared understanding that
you are seeking a client-side "convention" that helps users avoid
mistakes.  You do not have anything more than "convention" to force
certain behaviour on users.  Are we in agreement?

Now, if a "convention," is an acceptaible solution, you do not even
necessarily need a hook. You can give "git mypush" to the users,
tell them to use it instead of "git push" and do whatever check in
"git mypush" and you are done.  If your users can be told not to run
"git push" with --no-verify, they can certainly taught not to use
"git push" but to use "git mypush". Cf. 5 valid reasons to have hook
(http://thread.gmane.org/gmane.comp.version-control.git/71069).

The reason why we added pre-push is primarily because parsing the
command line is too brittle a way to guess what will be pushed.
When the user says "git push" and your "hook" sees "ah, there is no
argument", that does not mean the user did not try to push any
tags.  The user may have "remote.origin.push" refspec to always push
things without typing from the command line, for example.  The
approach to inspect the command line, whether it is done in "hook"
or "git mypush", is unworkable, and that is why pre-push is told
what "git push" decided to update on the remote end.

Having said all that, I am not sure if ec55559f (push: Add support
for pre-push hooks, 2013-01-13) implemented the pre-push hook
correctly.

I do not offhand know (I am on a bus with terrible connection so I
won't bother checking the source now) if we send "this ref has to
point at that object" even for STATUS_UPTODATE cases, to cause your
remote to trigger the receive hook in the frist place, but if that
is the case, then the code in run_pre_push_hook() that omits such
refs from the hook input looks just *wrong*.  On the other hand, I
do not offhand think of a valid reason for the push codepath (with
or without the pre-push hook) to send "this ref has to point at that
object" when we know the ref is already pointing at that object, so
I am not sure if your claim (i.e. "when ... was already pushed
previously") is true for a correctly built receiving side of Git.

On the other side, if we are telling the other end "I know your
refs/tags/v1.0 is pointing at this object, but I am telling you to
point at it again anyway", then we should be feeding pre-push that,
too.  The code here in transport.c omits UPTODATE ones, and it may
need to be disabled to be consistent.

	for (r = remote_refs; r; r = r->next) {
		if (!r->peer_ref) continue;
		if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue;
		if (r->status == REF_STATUS_REJECT_STALE) continue;
		if (r->status == REF_STATUS_UPTODATE) continue;

		strbuf_reset(&buf);
		strbuf_addf( &buf, "%s %s %s %s\n",
			 r->peer_ref->name, sha1_to_hex(r->new_sha1),
			 r->name, sha1_to_hex(r->old_sha1));

		if (write_in_full(proc.in, buf.buf, buf.len) != buf.len) {
			ret = -1;
			break;
		}

Re: Feature Request: provide cmdline args to git hooks

[Chris O'Kelly]

> I do not offhand know (I am on a bus with terrible connection so I
> won't bother checking the source now) if we send "this ref has to
> point at that object" even for STATUS_UPTODATE cases, to cause your
> remote to trigger the receive hook in the frist place, but if that
> is the case, then the code in run_pre_push_hook() that omits such
> refs from the hook input looks just *wrong*.  On the other hand, I
> do not offhand think of a valid reason for the push codepath (with
> or without the pre-push hook) to send "this ref has to point at that
> object" when we know the ref is already pointing at that object, so
> I am not sure if your claim (i.e. "when ... was already pushed
> previously") is true for a correctly built receiving side of Git.

No I totally understand that the pre-receive is the ideal place to do
it, and I can see that it's not feasible to rework how pre-push was
designed if it was specifically made not to handle this kind of
situation. In a perfect world I would just remove/modify the
post-receive hook causing the undesirable behavior, but I work within
the restrictions of my environment. To reiterate and clarify I'm not
saying the undesirable behavior is a built in part of git, just a
feature of our hosting platform's Git deployment mechanisms, although
if what you mean is that the post-receive hook on the receiving end
shouldn't be getting passed pushed tag refs that the local git
believed to be already up to date on the remote (as of most recent
fetch), then yeah... that is weird because it seems to be the behavior
in this case.

Anyway it sounds like the answer here is that it really isn't a
feasible task in a client side hook, and we should stick with the
current solution of "Don't use the GUI to push to Live" for now, which
is fine; I should probably stop trying to script around every single
problem anyway (https://xkcd.com/1319/).

Re: Feature Request: provide cmdline args to git hooks

[Junio C Hamano]

"Chris O'Kelly" <chris@mapcreative.com.au> writes:

> To reiterate and clarify I'm not saying the undesirable behavior
> is a built in part of git, just a feature of our hosting
> platform's Git deployment mechanisms, although if what you mean is
> that the post-receive hook on the receiving end shouldn't be
> getting passed pushed tag refs that the local git believed to be
> already up to date on the remote (as of most recent fetch), then
> yeah... that is weird because it seems to be the behavior in this
> case.

I just checked.

    $ rm -fr new && mkdir new && cd new
    $ git init src && git init --bare dst
    $ cd src
    $ echo "(date;cat) >pushlog" >.git/hooks/pre-push
    $ chmod +x .git/hooks/pre-push
    $ git commit -m 'initial' --allow-empty
    $ git tag -m 'initial' initial

Push only the branch:

    $ GIT_TRACE_PACKET=1 git push ../dst master 2>&1 | grep 'push>'
    11:07:26.... packet: push> 0000... 66ba... refs/heads/master\0report-st...
    11:07:26.... packet: push> 0000
    $ cat pushlog
    Wed Apr 15 11:07:26 PDT 2015
    refs/heads/master 66ba... refs/heads/master 0000...

In the packet trace, we can see that we told the remote to update 'master',
and the pre-push logger also records the same.

Then push with --follow-tags:

    $ GIT_TRACE_PACKET=1 git push --follow-tags ../dst master 2>&1 | grep 'push>'
    11:09:53.... packet: push> 0000... 30fa... refs/tags/initial\0report-st...
    11:09:53.... packet: push> 0000
    $ cat pushlog
    Wed Apr 15 11:09:53 PDT 2015
    refs/tags/initial 30fa... refs/tags/initial 0000...

We can see that we told the remote to store the tag, which matches
what the pre-push saw.

And then an empty push:

    $ GIT_TRACE_PACKET=1 git push --follow-tags ../dst master 2>&1 | grep 'push>'
    11:11:23.... packet: push> 0000
    $ cat pushlog
    Wed Apr 15 11:11:23 PDT 2015

We tell them to do nothing, and pre-push saw nothing.

For a good measure, let's advance the branch and push it out:

    $ git commit --allow-empty -m second
    $ GIT_TRACE_PACKET=1 git push --follow-tags ../dst master 2>&1 | grep 'push>'
    11:13:43.... packet: push> 66ba... e711... refs/heads/master\0report-st...
    11:13:43.... packet: push> 0000
    $ cat pushlog
    Wed Apr 15 11:13:43 PDT 2015
    refs/heads/master e711... refs/heads/master 66ba...

We notice that the tag is up to date and do not tell them to do
anything to it, and pre-push did not see the tag either.

As far as I can see so far, the behaviour of the underlying push
(i.e. what we decide to tell the remote to update) is sensible,
and what pre-push is told we are doing by the command is consistent
with what is really pushed.

So....

> Anyway it sounds like the answer here is that it really isn't a
> feasible task in a client side hook, and we should stick with the
> current solution of "Don't use the GUI to push to Live" for now, which
> is fine; I should probably stop trying to script around every single
> problem anyway (https://xkcd.com/1319/).

It appears that your "GUI" is a broken implementation of Git, that
tells the other side to update what it did not even send, and that
is what is causing the trouble, perhaps?