On Wed, Nov 5, 2014 at 8:00 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Johan Herland <johan@xxxxxxxxxxx> 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?
I'm rewriting the tests not to use 'git log' at all. We already have
other tests that check the formatting of notes, so we really only need
to use "git notes list HEAD" in these tests.
>> + 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)?
Yes, will fix.
>> +}
>> +
>> +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.
Thanks. That's much easier to read. Will fix.
On Wed, Nov 5, 2014 at 7:36 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> 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?
Will move the tests into t3301 in the re-roll.
...Johan
--
Johan Herland, <johan@xxxxxxxxxxx>
www.herland.net
--
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