Re: [PATCH 2/2] receive-pack: support push-to-checkout hook

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 2, 2014, 16:39 UTC
Message-ID: <xmqqtx1em2fu.fsf@gitster.dls.corp.google.com>
In-Reply-To: <alpine.DEB.1.00.1412020929420.13845@s15462909.onlinehome-server.info>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 21 quoted lines

> In this particular case, I think that we really, really *just* need to
> verify that the presence of the hook switches off the default behavior of
> updateInstead. *Nothing* else is needed to verify that this particular
> functionality hasn't regressed. I.e. something like:
>
> +test_expect_success 'updateInstead with push-to-checkout hook' '
> +	rm -fr testrepo &&
> +	git clone . testrepo &&
> +	(
> +		cd testrepo &&
> +		echo unclean > path1 &&
> +		git config receive.denyCurrentBranch updateInstead &&
> +		echo 'touch yep' | write_script .git/hooks/push-to-checkout
> +	) &&
> +	git push testrepo HEAD^:refs/heads/master &&
> +	test unclean = $(cat testrepo/path1) &&
> +	test -f testrepo/yep
> +'
>
> would be more appropriate (although it probably has one or three bugs,
> given that I wrote this in the mailer).

Not really. You need to remember that we write tests not to show off the new shiny, but to protect essential invariants from being broken by careless others attempting to rewrite the implementation in the future.

What needs to be tested for the feature are at the minimum:

 * The hook is not triggered when denycurrent is set to
   updateInstead; the posted version does not even test this, but it
   should.

 * The hook is run with the right settings, so that git commands it
   runs will operate on the right repository and its working tree,
   but without overspecifying how that "right settings" happens [*1*].

 * Non-zero exit from the hook will fail "git push", and zero exit
   from the hook will allow "git push" to pass.

 * Whether the hook returns a success or a failure, the working tree
   and the index is in the state the hook expects to give the user
   (i.e. nobody else further munges the working tree and the index
   after hook returns) [*2*].

The patch I posted is minimum to do so. Compared to that, the "yep" test is not checking anything of the importance, and only insists on a single immaterial detail (i.e. hook must be run after cd'ing to the top of the working tree).

I fail to see why it could be more appropriate.

[Footnote]

*1* Your version above assumes that the hook must run in the working
    tree, but it does not have to, if GIT_DIR and GIT_WORK_TREE are
    both exported; your test is overspecifying what should happen,
    and will reject a legitimate implementation of the feature.

*2* A hook may muck with the index or with the working tree and then
    return a failure.  I do not offhand see why a failing push
    should be allowed to contaminate the working tree that way, but
    whatever the hook does is what the user wishes, and we should
    not lose data coming from that wish.  The hook the test uses
    refrains from touching the working tree or the index when it
    fails, so the test checks that the working tree and the index
    are left as-is when it happens.