On Mon, Feb 09, 2009 at 01:09:21AM -0800, Junio C Hamano wrote:
> Each test chdir'ed around and ended up in a random place if any of the
> test in the sequence failed but the entire test script was allowed to
> run. This wrapps each in a subshell as necessary.
Certainly a good cleanup, but...
> -test_expect_success \
> - 'push can be used to delete a ref' '
> +test_expect_success 'push can be used to delete a ref' '
> + (
> cd victim &&
> git branch extra master &&
> cd .. &&
> test -f victim/.git/refs/heads/extra &&
> git send-pack ./victim/.git/ :extra master &&
> ! test -f victim/.git/refs/heads/extra
> + )
> '
Wouldn't this be cleaner as:
(
cd victim &&
git branch extra master
) &&
...
That is, it is not only safer but (IMHO) a bit easier to see which parts
are happening in which directory.
> +test_expect_success 'pushing a delete should be denied with denyDeletes' '
> + (
> cd victim &&
> git config receive.denyDeletes true &&
> git branch extra master &&
> cd .. &&
> test -f victim/.git/refs/heads/extra &&
> test_must_fail git send-pack ./victim/.git/ :extra master
> + )
Ditto (and there are more, but I won't quote each one).
> +test_expect_success 'pushing with --force should be denied with denyNonFastforwards' '
> + (
> cd victim &&
> git config receive.denyNonFastforwards true &&
> cd .. &&
> git update-ref refs/heads/master master^ || return 1
> git send-pack --force ./victim/.git/ master && return 1
> ! test_cmp .git/refs/heads/master victim/.git/refs/heads/master
> + )
And here I don't know what in the world is going on with those "return
1" lines. Shouldn't this be a chain of &&'s with a test_must_fail?
I.e.,:
( cd victim && git config receive.denyNonFastforwards true ) &&
git update-ref refs/heads/master master^ &&
test_must_fail git send-pack --force ./victim/.git/ master &&
! test_cmp .git/refs/heads/master victim/.git/refs/heads/master
Not to mention that the final test_cmp would be more robust if written
to make sure the victim's master ref stayed the same (instead of just
making sure we didn't screw it up in one particular way). And it should
probably use a git command rather than looking at the refs files (to be
future-proof against any automatic ref-packing), but that is just
nit-picking.
All minor things, of course, but while we're cleaning up... :)
-Peff
--
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
