Re: [PATCHv2 2/3] t3312-notes-empty: Test that 'git notes' removes empty notes by default

> Johan Herland <johan@herland.net> writes:
>> +verify_missing() {
>> +     git log -1 > actual &&
>
> Hmph, it was unclear what exactly you are trying to check with this
> one and the other "git log -1 >expect_missing".
>
> Perhaps a comment that says "We are interested in the trailing
> 'Notes: ...' in the output" is necessary here, or (probably even
> better) use the --format='%N' to make it crystal clear?

>> +     test_cmp expect_missing actual &&
>> +     ! git notes list HEAD
>
> Isn't this test_must_fail (i.e. if somebody screws up to make "git
> notes list" segfault, the test should fail, not taking the dying
> with SEGV as a sign of success)?

>> +}
>> +
>> +for cmd in \
>> +     'add' \
>> +     'add -F /dev/null' \
>> +     'add -m ""' \
>> +     'add -c "$empty_blob"' \
>> +     'add -C "$empty_blob"' \
>> +     'append' \
>> +     'append -F /dev/null' \
>> +     'append -m ""' \
>> +     'append -c "$empty_blob"' \
>> +     'append -C "$empty_blob"' \
>> +     'edit'
>> +do
>> +     test_expect_success "'git notes $cmd' removes empty note" "
>> +             cleanup_notes &&
>> +             MSG= git notes $cmd &&
>> +             verify_missing
>> +     "
>> +done
>
> Perhaps just a taste issue, but I would think
>
>         while read cmd
>         do
>                 ... that test eval with $cmd interpolation ...
>         done <<-\EOF
>         add
>         add -F /dev/null
>         ...
>         EOF
>
> would be easier to maintain and to read, without having to worry
> about quoting and backslashing.

> By definition, an empty note is empty ;-) and devoid of useful
> information other than a single bit, its existence.  I would
> understand a handful of tests that check "oh by the way, we should
> also handle empty ones sensibly", but are you sure that a _new_
> separate test script, not addition to existing test script, is worth
> to check _only_ empty notes?