Junio C Hamano venit, vidit, dixit 19.05.2011 08:50: > Michael J Gruber <git@xxxxxxxxxxxxxxxxxxxx> writes: > > [jc: cull the part you did not comment on, thanks] > >>> +test_expect_success 'removing more than one' ' >>> + before=$(git rev-parse --verify refs/notes/commits) && >>> + test_when_finished "git update-ref refs/notes/commits $before" && >>> + git notes remove HEAD^^ HEAD^^^ && >>> + git diff --name-only refs/notes/commits^ refs/notes/commits >actual && >>> + test 2 = $(wc -l <actual) && >>> + git ls-tree -r --name-only refs/notes/commits >actual && >>> + >empty && >>> + test_cmp empty actual >>> +' >> >> You're not really testing that this removes the correct notes. Actually, >> you're not even testing that this removes only 2 notes when there are >> more than 2, are you? > > In the test vector, there are only two notes, and I am testing removal of > multiple. What do you want me to do? Test removing one and make sure the > other one survives ;-)? The test has two notes because it was created when remove would remove one note at a time only, and the test made sure it did not remove the other one (!). If you want a meaningful test which tests that "git notes remove A B" removes exactly 2 notes (not more) you need to add another note first. Otherwise it could be that your new code simply nukes the notes tree. (I know it does not, but only by looking at the source; this test does not check it.) Checking that it actually removes *those* 2 notes would give extra credit but may not be necessary (since we know it removes the correct one in single mode). > Patches on top of it is very welcome, but I wouldn't bother. Is that the kind of response you accept from submitters to your (valid) requests that they provide negative tests in addition to positive ones? I don't mind helping out with a test patch (later this UTC day) but I'm concerned about the attitude someone may read into this and may want to follow as an example. In terms of code-with-test habits, I would claim that git.git is really an exemplary standard to follow (and to keep). Cheers, Michael -- 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