Jeff King <peff@xxxxxxxx> writes: > 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... :) Sure. This was made as a quick-fix to a mess others created, so I did not study them very deeply. Will reroll if I have the time but it is likely that I may be tending other topics first. -- 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
