Re: [PATCH 08/16] t3310: extract common no_notes_merge_left()

> On Fri, Dec 27, 2019 at 8:48 AM Denton Liu <liu.denton@gmail.com> wrote:
>> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
>> @@ -32,6 +32,11 @@ verify_notes () {
>> +no_notes_merge_left () {
>> +       { ls .git/NOTES_MERGE_* >output || :; } &&
>> +       test_must_be_empty output
>> +}
>
> This function name leaves me thinking that it's talking about
> directionality (left vs. right) and gives insufficient clue that it's
> talking about a .git/NOTES_MERGE_* file. A name such as
> assert_no_notes_merge_files() or notes_merge_files_gone() would make
> the intention more obvious.
>
>> -       # No .git/NOTES_MERGE_* files left
>> -       test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
>> -       test_must_be_empty output &&
>
> On the other hand, the original in-code comment was not confusing,
> probably because it was obvious it was talking about an actual file,
> due to spelling out .git/NOTES_MERGE_* explicitly and due to actually
> using the literal word "file", plus the code following the comment
> made it very obvious what was happening.
>
> These observations may not be actionable since someone actually
> working on this script will know that it's dealing with
> .git/NOTES_MERGES_*, but as a reviewer not familiar with this
> particular script, reading the patch from top to bottom, I found the
> function name confusing.