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.
