[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH 08/16] t3310: extract common no_notes_merge_left()
From:       Jakub Narebski <jnareb () gmail ! com>
Date:       2019-12-30 20:38:04
Message-ID: 86eewl35xv.fsf () gmail ! com
[Download RAW message or body]

Eric Sunshine <sunshine@sunshineco.com> writes:
> 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.

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
[prev in list] [next in list] [prev in thread] [next in thread]