Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> On Fri, Dec 27, 2019 at 8:48 AM Denton Liu <liu.denton@xxxxxxxxx> 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.
The problem with clarity of meaning is enhanced by the fact that during
refactoring the "# No .git/NOTES_MERGE_* files left" comment got lost.
It could have been added in the new function, as first line; or
rephrased as this new function description.
Best,
--
Jakub Narębski
