| Message ID | 20200812192737.13971-3-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t7401: modernize, cleanup and more | expand |
Shourya Shukla <shouryashukla.oo@gmail.com> writes: > Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to > 'test_i18ncmp expected actual' to align it with the convention followed > by other tests in the test script. > ... > @@ -285,7 +285,7 @@ EOF > > test_expect_success '--for-status' " > git submodule summary --for-status HEAD^ >actual && > - test_i18ncmp actual - <<EOF > + test_i18ncmp - actual <<-EOF > * sm1 $head6...0000000: > > * sm2 0000000...$head7 (2): This one does more than what the proposed log message explains, but it does not do enough at the same time. If "actual vs expected order" is what this commit wants to fix, then "<<EOF" vs "<<-EOF" is outside the scope of it. Personally, I think it is a good idea to update the end-of-heredoc marker to "<<-EOF", but the patch makes its readers wonder if the author of the patch understands why it is a good idea to begin with, because the end-of-heredoc marker is the only thing the patch changes, and it does not change the indentation of heredoc itself. The whole point of using "<<-EOF" instead of "<<EOF" is so that the result becomes easier to read with indentation, e.g. test_i18ncmp - actual <<-EOF * sm1 $head6...0000000: * sm2 0000000...$head7 (2): ... EOF Compared to the original: test_i18ncmp - actual <<EOF * sm1 $head6...0000000: * sm2 0000000...$head7 (2): ... EOF it would be easier to read.
Junio C Hamano <gitster@pobox.com> writes: > Shourya Shukla <shouryashukla.oo@gmail.com> writes: > >> Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to >> 'test_i18ncmp expected actual' to align it with the convention followed >> by other tests in the test script. >> ... >> @@ -285,7 +285,7 @@ EOF >> >> test_expect_success '--for-status' " >> git submodule summary --for-status HEAD^ >actual && >> - test_i18ncmp actual - <<EOF >> + test_i18ncmp - actual <<-EOF >> * sm1 $head6...0000000: >> >> * sm2 0000000...$head7 (2): > > This one does more than what the proposed log message explains, but > it does not do enough at the same time. > > If "actual vs expected order" is what this commit wants to fix, then > "<<EOF" vs "<<-EOF" is outside the scope of it. > > Personally, I think it is a good idea to update the end-of-heredoc > marker to "<<-EOF", ... It seems that the theme of your [3/4] is exactly about fixing the "end-of-heredoc marker is prefixed with dash, but the heredoc is not indented for readability", so perhaps you'd want to stop this step at turning the line to >> - test_i18ncmp actual - <<EOF >> + test_i18ncmp - actual <<EOF i.e. "compare expected vs actual in this order", and then in the next patch [3/4], edit the line further to test_i18ncmp - actual <<-EOF *and* indent the here-doc at the same time?
On 12/08 02:02, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Shourya Shukla <shouryashukla.oo@gmail.com> writes: > > > >> Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to > >> 'test_i18ncmp expected actual' to align it with the convention followed > >> by other tests in the test script. > >> ... > >> @@ -285,7 +285,7 @@ EOF > >> > >> test_expect_success '--for-status' " > >> git submodule summary --for-status HEAD^ >actual && > >> - test_i18ncmp actual - <<EOF > >> + test_i18ncmp - actual <<-EOF > >> * sm1 $head6...0000000: > >> > >> * sm2 0000000...$head7 (2): > > > > This one does more than what the proposed log message explains, but > > it does not do enough at the same time. > > > > If "actual vs expected order" is what this commit wants to fix, then > > "<<EOF" vs "<<-EOF" is outside the scope of it. > > > > Personally, I think it is a good idea to update the end-of-heredoc > > marker to "<<-EOF", ... > > It seems that the theme of your [3/4] is exactly about fixing the > "end-of-heredoc marker is prefixed with dash, but the heredoc is not > indented for readability", so perhaps you'd want to stop this step > at turning the line to > > >> - test_i18ncmp actual - <<EOF > >> + test_i18ncmp - actual <<EOF > > i.e. "compare expected vs actual in this order", and then in the > next patch [3/4], edit the line further to > > test_i18ncmp - actual <<-EOF > > *and* indent the here-doc at the same time? Ohh okay okay. I understand what you are saying. I wanted to fix the heredoc markers and indent the tests for better readibility but actually I fixed the heredoc marker in [2/4]. Therefore, the change in [2/4] should in fact be: - test_i18ncmp actual - <<EOF + test_i18ncmp - actual <<EOF And in this patch [3/4], it should become: - test_i18ncmp - actual <<EOF + test_i18ncmp - actual <<-EOF As well as the indentation fixes I did in the patch already. Now I understand the exact use and significance of the heredoc. Thank you.
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh index 8ee78bcb69..53943c9cc9 100755 --- a/t/t7401-submodule-summary.sh +++ b/t/t7401-submodule-summary.sh @@ -183,7 +183,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --cached' " < Add foo5 EOF - test_i18ncmp actual expected + test_i18ncmp expected actual " test_expect_success 'typechanged submodule(submodule->blob), --files' " @@ -193,7 +193,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --files' " > Add foo5 EOF - test_i18ncmp actual expected + test_i18ncmp expected actual " rm -rf sm1 && @@ -204,7 +204,7 @@ test_expect_success 'typechanged submodule(submodule->blob)' " * sm1 $head4(submodule)->$head5(blob): EOF - test_i18ncmp actual expected + test_i18ncmp expected actual " rm -f sm1 && @@ -217,7 +217,7 @@ test_expect_success 'nonexistent commit' " Warn: sm1 doesn't contain commit $head4_full EOF - test_i18ncmp actual expected + test_i18ncmp expected actual " commit_file @@ -285,7 +285,7 @@ EOF test_expect_success '--for-status' " git submodule summary --for-status HEAD^ >actual && - test_i18ncmp actual - <<EOF + test_i18ncmp - actual <<-EOF * sm1 $head6...0000000: * sm2 0000000...$head7 (2):
Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to 'test_i18ncmp expected actual' to align it with the convention followed by other tests in the test script. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t7401-submodule-summary.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)