Re: [PATCH 08/16] t3310: extract common no_notes_merge_left()
From: Eric Sunshine <hidden>
Date: 2019-12-28 07:20:26
On Fri, Dec 27, 2019 at 8:48 AM Denton Liu [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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.