Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
> 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.
--
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