Alexey Shumkin <zapped@xxxxxxx> writes: > diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh > ... > @@ -48,28 +53,25 @@ head2=$(add_file sm1 foo3) > > test_expect_success 'modified submodule(forward)' " As this is [PATCH 1/2], doesn't this patch make this test fail, calling for test_expect_failure here (and then later in 2/2 to be flipped back to test_expect_success)? > git diff-index -p --submodule=log HEAD >actual && > - cat >expected <<-EOF && > -Submodule sm1 $head1..$head2: > - > Add foo3 > -EOF > + printf \"Submodule sm1 $head1..$head2:\n\ > + > Add foo3 ($added foo3)\n\ > +\" > expected && > test_cmp expected actual > " Hmmm... why printf? Is it worth to force reviewers wonder what would happen if any of these $variables happened to have "%" in them? Are you benefiting from any printf features that you cannot achieve with the original cat? This hunk and others throughout your patch that change cat with here doc into printf seem to make the tests less legible, at least to me. Perhaps like this instead, if the "flushed left" of the original looked ugly to your eyes? @@ -49,27 +54,25 @@ head2=$(add_file sm1 foo3) test_expect_success 'modified submodule(forward)' " git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && -Submodule sm1 $head1..$head2: - > Add foo3 -EOF + Submodule sm1 $head1..$head2: + > Add foo3 ($added foo3) + EOF test_cmp expected actual " -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html