[GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script

[GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script

[Shuqi Liang]

I cleaned up some old style in test script.

for example :

* old style:

    test_expect_success \
        'title' \
        'body line 1 &&
        body line 2'

  should become:

    test_expect_success 'title' '
        body line 1 &&
        body line 2
    '




Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
Hi,I'm Shuqi Liang.a junior student majors in Computer Science at University of Western Ontario. 

This patch is the microproject I try to getting involved with the Git project.

I have read 'MyFirstContribution.txt', 'Hacking Git' and the book 《pro git》 ,and I know more about objects, references, packfile format, etc.
Over the coming period, I will delve into the source code and gain a deeper understanding and try to  contribute more meaningful patch to the community.

 t/t4113-apply-ending.sh | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index 66fa51591e..aa57895b22 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -24,13 +24,14 @@ echo 'a' >file
 echo 'b' >>file
 echo 'c' >>file
 
-test_expect_success setup \
-    'git update-index --add file'
-
+test_expect_success setup '
+    git update-index --add file
+'
 # test
 
-test_expect_success 'apply at the end' \
-    'test_must_fail git apply --index test-patch'
+test_expect_success 'apply at the end' '
+    test_must_fail git apply --index test-patch
+'
 
 cat >test-patch <<\EOF
 diff a/file b/file
@@ -47,7 +48,8 @@ b
 c'
 git update-index file
 
-test_expect_success 'apply at the beginning' \
-	'test_must_fail git apply --index test-patch'
+test_expect_success 'apply at the beginning' '
+    test_must_fail git apply --index test-patch
+'
 
 test_done
-- 
2.39.0

Re: [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script

[Andrei Rybak]

Hi Shuqi Liang,

> Subject: [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script

For patches that change a single test, the subject line can include just
the "t" and the number.  The part after the colon should start with a
lowercase letter.  Something like

     t4113: modernize test style

On 31/01/2023 23:49, Shuqi Liang wrote:
> 
> I cleaned up some old style in test script.

Commit message should start with description of the existing problem
in present tense, something like:

     Test scripts in file t4113-apply-ending.sh are written in old style,
     where the test_expect_success command and test title are written on
     separate lines ...

Then changes should be described using imperative mood, as if you are
giving commands to the codebase.  See section "[[describe-changes]]"
in "Documentation/SubmittingPatches" for details.

You can also find examples of existing commit messages for similar
changes:

     $ git log --no-merges --grep='modernize' -- t

> 
> for example :
> 
> * old style:
> 
>      test_expect_success \
>          'title' \
>          'body line 1 &&
>          body line 2'
> 
>    should become:
> 
>      test_expect_success 'title' '
>          body line 1 &&
>          body line 2
>      '
> 
> 
> 
> 
> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
 > Hi,I'm Shuqi Liang.a junior student majors in Computer Science at
 > University of Western Ontario.

Welcome!

>   t/t4113-apply-ending.sh | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
> index 66fa51591e..aa57895b22 100755
> --- a/t/t4113-apply-ending.sh
> +++ b/t/t4113-apply-ending.sh
> @@ -24,13 +24,14 @@ echo 'a' >file
>   echo 'b' >>file
>   echo 'c' >>file

A "modern" test could also do such preparation for test files as
part of its "setup" step.  This could its own patch in the same
series, separate from style changes.

In case of t4113, files "test-patch" and "file" are created twice.
The second creation of the files could be either its own step
'setup for apply at the beginning', or incorporated into the step
'apply at the beginning'.

Section "Recommended style" in t/README also has some notes about
how heredocs should be indented.

>   
> -test_expect_success setup \
> -    'git update-index --add file'
> -
> +test_expect_success setup '
> +    git update-index --add file
> +'

While changing the quoting around test tiles and commands, the
indentation with spaces could also be changed to TABs.

>   # test

If the setup code on top level of the file is moved into test
steps, this comment and the "# setup" comment at line 11 will
become unnecessary.

>   
> -test_expect_success 'apply at the end' \
> -    'test_must_fail git apply --index test-patch'
> +test_expect_success 'apply at the end' '
> +    test_must_fail git apply --index test-patch
> +'
>   
>   cat >test-patch <<\EOF
>   diff a/file b/file
> @@ -47,7 +48,8 @@ b
>   c'
>   git update-index file
>   
> -test_expect_success 'apply at the beginning' \
> -	'test_must_fail git apply --index test-patch'
> +test_expect_success 'apply at the beginning' '
> +    test_must_fail git apply --index test-patch
> +'
>   
>   test_done

Thanks.

[PATCH v2 0/4] t4113: modernize test style

[Shuqi Liang]

Modernize the test script t4113.

Comparing to v1:

1.Test scripts in file in this script are written in old style,
     where the test_expect_success command and test title are written on
     separate lines.Change the old style '\'  to new style "'".
for example :
-test_expect_success setup \
-    'git update-index --add file'
-
+test_expect_success setup '
+    git update-index --add file
+'    

2.Files "test-patch" and "file" are created twice.
put the second creation of the files to its own step
'setup for apply at the beginning'

3.This script still use the old style "<<".
Change  "<<-" instead of "<<"
for exmaple:
-	cat >test-patch <<\EOF
+	cat >test-patch <<-\EOF

4.The test bodies in this script are written in old style .which indented with space, but not TAB.replace indentation spaces with tabs.
for example :

 test_expect_success setup '
-    git update-index --add file
+	git update-index --add file
 '

Shuqi Liang (4):
  t/t4113-apply-ending.sh: Modernize a test script
  Test scripts in file t4113-apply-ending.sh, files "test-patch" and
    "file" are created twice.
  use "<<-" instead of "<<"
  t4113-apply-ending.sh used 4-column indent with space,fix it in use
    tabs for indentation.

 t/t4113-apply-ending.sh | 51 +++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

Range-diff against v1:
1:  d7d52f1f79 = 1:  d7d52f1f79 t/t4113-apply-ending.sh: Modernize a test script
-:  ---------- > 2:  d9e5a54e32 Test scripts in file t4113-apply-ending.sh, files "test-patch" and "file" are created twice.
-:  ---------- > 3:  01a5c3285e use "<<-" instead of "<<"
-:  ---------- > 4:  cf2b2ca5a0 t4113-apply-ending.sh used 4-column indent with space,fix it in use tabs for indentation.
-- 
2.39.0

[PATCH v2 1/4]t4113: replace backslash with single quote

[Shuqi Liang]

Change the old style '\'  to new style "'"

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index 66fa51591e..aa57895b22 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -24,13 +24,14 @@ echo 'a' >file
 echo 'b' >>file
 echo 'c' >>file
 
-test_expect_success setup \
-    'git update-index --add file'
-
+test_expect_success setup '
+    git update-index --add file
+'
 # test
 
-test_expect_success 'apply at the end' \
-    'test_must_fail git apply --index test-patch'
+test_expect_success 'apply at the end' '
+    test_must_fail git apply --index test-patch
+'
 
 cat >test-patch <<\EOF
 diff a/file b/file
@@ -47,7 +48,8 @@ b
 c'
 git update-index file
 
-test_expect_success 'apply at the beginning' \
-	'test_must_fail git apply --index test-patch'
+test_expect_success 'apply at the beginning' '
+    test_must_fail git apply --index test-patch
+'
 
 test_done
-- 
2.39.0

[PATCH v2 2/4] t4113:put second creation in own step

[Shuqi Liang]

files "test-patch" and "file" are created twice.put the second creation 
of the files to its own step'setup for apply at the beginning'.

Make the second creation of the files its own step
'setup for apply at the beginning'.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index aa57895b22..d84f632bc3 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -8,8 +8,6 @@ test_description='git apply trying to add an ending line.
 '
 . ./test-lib.sh
 
-# setup
-
 cat >test-patch <<\EOF
 diff --git a/file b/file
 --- a/file
@@ -27,26 +25,27 @@ echo 'c' >>file
 test_expect_success setup '
     git update-index --add file
 '
-# test
 
 test_expect_success 'apply at the end' '
     test_must_fail git apply --index test-patch
 '
 
-cat >test-patch <<\EOF
-diff a/file b/file
---- a/file
-+++ b/file
-@@ -1,2 +1,3 @@
-+a
- b
- c
-EOF
-
-echo >file 'a
-b
-c'
-git update-index file
+test_expect_success 'setup for apply at the beginning' '
+	cat >test-patch <<\EOF
+	diff a/file b/file
+	--- a/file
+	+++ b/file
+	@@ -1,2 +1,3 @@
+	+a
+	b
+	c
+	EOF
+
+	echo >file 'a
+	b
+	c'
+	git update-index file
+'
 
 test_expect_success 'apply at the beginning' '
     test_must_fail git apply --index test-patch
-- 
2.39.0

[PATCH v2 3/4] t4113: use "<<-" instead of "<<"

[Shuqi Liang]

use "<<-" instead of "<<"

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index d84f632bc3..d5b15e97d3 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -8,7 +8,7 @@ test_description='git apply trying to add an ending line.
 '
 . ./test-lib.sh
 
-cat >test-patch <<\EOF
+cat >test-patch <<-\EOF
 diff --git a/file b/file
 --- a/file
 +++ b/file
@@ -31,7 +31,7 @@ test_expect_success 'apply at the end' '
 '
 
 test_expect_success 'setup for apply at the beginning' '
-	cat >test-patch <<\EOF
+	cat >test-patch <<-\EOF
 	diff a/file b/file
 	--- a/file
 	+++ b/file
-- 
2.39.0

[PATCH v2 4/4] t4113: indent with tabs

[Shuqi Liang]

t4113-apply-ending.sh used 4-column indent with
space,fix it in use tabs for indentation.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index d5b15e97d3..9e28c72355 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -23,11 +23,11 @@ echo 'b' >>file
 echo 'c' >>file
 
 test_expect_success setup '
-    git update-index --add file
+	git update-index --add file
 '
 
 test_expect_success 'apply at the end' '
-    test_must_fail git apply --index test-patch
+	test_must_fail git apply --index test-patch
 '
 
 test_expect_success 'setup for apply at the beginning' '
@@ -48,7 +48,7 @@ test_expect_success 'setup for apply at the beginning' '
 '
 
 test_expect_success 'apply at the beginning' '
-    test_must_fail git apply --index test-patch
+	test_must_fail git apply --index test-patch
 '
 
 test_done
-- 
2.39.0

Re: [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script

[cheska fran]

Hi Andrei,


Andrei Rybak <rybak.a.v@gmail.com> On 31/01/2023 21:21：
> For patches that change a single test, the subject line can include just
> the "t" and the number.  The part after the colon should start with a
> lowercase letter.  Something like
>
>      t4113: modernize test style
>

Thanks, this tip is really helpful. I will change it.


> Commit message should start with description of the existing problem
> in present tense, something like:
>
>      Test scripts in file t4113-apply-ending.sh are written in old style,
>      where the test_expect_success command and test title are written on
>      separate lines ...
>
> Then changes should be described using imperative mood, as if you are
> giving commands to the codebase.  See section "[[describe-changes]]"
> in "Documentation/SubmittingPatches" for details.

> You can also find examples of existing commit messages for similar
> changes:
>
>      $ git log --no-merges --grep='modernize' -- t
>
Thanks,that is cool! I tried it and I saw a lot of examples and their
descriptions were very clear and I learned a lot


> In case of t4113, files "test-patch" and "file" are created twice.
> The second creation of the files could be either its own step
> 'setup for apply at the beginning', or incorporated into the step
> 'apply at the beginning'.
yeah,once before the first instance of test-patch and then again
before the second instance of test-patch.
I will move the second creation of the files to its own step in setup
for apply at the beginning.'

> Section "Recommended style" in t/README also has some notes about
> how heredocs should be indented.

Sure, I did not realize this.I will use "<<-" instead of "<<".

> While changing the quoting around test tiles and commands, the
> indentation with spaces could also be changed to TABs.
will do.

> If the setup code on top level of the file is moved into test
> steps, this comment and the "# setup" comment at line 11 will
> become unnecessary.
Thanks. It's easy to miss.The purpose of these tags is to distinguish
the setup and test parts of the script,
but if the file creation has been moved to a separate step, then these
tags are no longer needed.



Thanks for the reply and it is really helpful!

--
Thanks,
Shuqi

Re: [PATCH v2 1/4]t4113: replace backslash with single quote

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

The example Andrei gave you, i.e.

    Test scripts in file t4113-apply-ending.sh are written in old style,
    where the test_expect_success command and test title are written on
    separate lines ...

was quite readable, but this

> Change the old style '\'  to new style "'"

is almost impossible to understand without knowing that this wanted
to say what Andrei gave in a different way.  The title is worse.
It's not replacing a backslash with a single quote, which would
result in

    -test_expect_success setup \
    +test_expect_success setup '
        'git update-index --add file'

and obviously that is not what you did (or wanted to do).

> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
>  t/t4113-apply-ending.sh | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)

The patch text looks OK.

Re: [PATCH v2 3/4] t4113: use "<<-" instead of "<<"

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> use "<<-" instead of "<<"

You forgot to explain "Why?".  What you did, anybody can see in the
patch text.  Why you did so is what you are expected to explain in
your proposed log message.

> -cat >test-patch <<\EOF
> +cat >test-patch <<-\EOF
>  diff --git a/file b/file
>  --- a/file
>  +++ b/file

There is no need to do this, as the body of the here-doc is not
indented/prefixed with HT at all. 

> @@ -31,7 +31,7 @@ test_expect_success 'apply at the end' '
>  '
>  
>  test_expect_success 'setup for apply at the beginning' '
> -	cat >test-patch <<\EOF
> +	cat >test-patch <<-\EOF
>  	diff a/file b/file
>  	--- a/file
>  	+++ b/file

This is necessary but that is only because [PATCH v2 2/4] broke it.
In general, we frown upon a series where a bug is introduced in an
earlier step, with another patch fixing that bug.  

Let's not introduce such a bug that we need to fix later from the
beginning instead.

Thanks.

Re: [PATCH v2 4/4] t4113: indent with tabs

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> t4113-apply-ending.sh used 4-column indent with
> space,fix it in use tabs for indentation.

Good, but end the sentence with a full-top with a space after it,
and start the next sentence with a capital letter.


> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
>  t/t4113-apply-ending.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
> index d5b15e97d3..9e28c72355 100755
> --- a/t/t4113-apply-ending.sh
> +++ b/t/t4113-apply-ending.sh
> @@ -23,11 +23,11 @@ echo 'b' >>file
>  echo 'c' >>file
>  
>  test_expect_success setup '
> -    git update-index --add file
> +	git update-index --add file
>  '

This is not wrong per se, but the modern style is to avoid having
any executable lines outside test_expect_foo.  I'd expect that the
resulting script begins more like the attached.  [PATCH 4/4] stops
the conversion in the middle, which leaves funny taste in our mouth.

Thanks.

diff --git i/t/t4113-apply-ending.sh w/t/t4113-apply-ending.sh
index 66fa51591e..9746f45898 100755
--- i/t/t4113-apply-ending.sh
+++ w/t/t4113-apply-ending.sh
@@ -8,24 +8,20 @@ test_description='git apply trying to add an ending line.
 '
 . ./test-lib.sh
 
-# setup
-
-cat >test-patch <<\EOF
-diff --git a/file b/file
---- a/file
-+++ b/file
-@@ -1,2 +1,3 @@
- a
- b
-+c
-EOF
-
-echo 'a' >file
-echo 'b' >>file
-echo 'c' >>file
-
-test_expect_success setup \
-    'git update-index --add file'
+test_expect_success setup '
+	cat >test-patch <<-\EOF
+	diff --git a/file b/file
+	--- a/file
+	+++ b/file
+	@@ -1,2 +1,3 @@
+	 a
+	 b
+	+c
+	EOF
+
+	test_write_lines a b c >file
+	git update-index --add file
+'
 
 # test
 

Re: [PATCH v2 1/4]t4113: replace backslash with single quote

[Shuqi Liang]

Hi Junio,

On Thu, Feb 2, 2023 at 4:00 PM Junio C Hamano <gitster@pobox.com> wrote:

> is almost impossible to understand without knowing that this wanted
> to say what Andrei gave in a different way.  The title is worse.
> It's not replacing a backslash with a single quote, which would
> result in
>
>     -test_expect_success setup \
>     +test_expect_success setup '
>         'git update-index --add file'
>
> and obviously that is not what you did (or wanted to do).

Thanks, I will modify it to make it clear about my motivation and the
real changes to my patch.

Re: [PATCH v2 3/4] t4113: use "<<-" instead of "<<"

[Shuqi Liang]

On Thu, Feb 2, 2023 at 4:05 PM Junio C Hamano <gitster@pobox.com> wrote:

> You forgot to explain "Why?".  What you did, anybody can see in the
> patch text.  Why you did so is what you are expected to explain in
> your proposed log message.
Thanks. I did make a lot of mistakes in writing a good commit message.
I will modify it.

> > -cat >test-patch <<\EOF
> > +cat >test-patch <<-\EOF
> >  diff --git a/file b/file
> >  --- a/file
> >  +++ b/file
>
> There is no need to do this, as the body of the here-doc is not
> indented/prefixed with HT at all.

yeah, I did not notice that , According to t/README says, Indent the
body of here-document, and use "<<-" instead of "<<"
to strip leading TABs used for indentation. But here do not have the
leading TABS in front of it.

> > @@ -31,7 +31,7 @@ test_expect_success 'apply at the end' '
> >  '
> >
> >  test_expect_success 'setup for apply at the beginning' '
> > -     cat >test-patch <<\EOF
> > +     cat >test-patch <<-\EOF
> >       diff a/file b/file
> >       --- a/file
> >       +++ b/file
>
> This is necessary but that is only because [PATCH v2 2/4] broke it.
> In general, we frown upon a series where a bug is introduced in an
> earlier step, with another patch fixing that bug.
>
> Let's not introduce such a bug that we need to fix later from the
> beginning instead.

Thanks, I will introduce the new bug caused by the current patch in
the beginning.

--------
Thanks,
shuqi

Re: [PATCH v2 4/4] t4113: indent with tabs

[Shuqi Liang]

On Thu, Feb 2, 2023 at 4:10 PM Junio C Hamano <gitster@pobox.com> wrote:

> Good, but end the sentence with a full-top with a space after it,
> and start the next sentence with a capital letter.

Sure, think I need to change it to " ...space. Fix..". I will pay more
attention to my English written style.


> This is not wrong per se, but the modern style is to avoid having
> any executable lines outside test_expect_foo.  I'd expect that the
> resulting script begins more like the attached.  [PATCH 4/4] stops
> the conversion in the middle, which leaves funny taste in our mouth.

Thanks, Will avoid having any executable lines outside test_expect_foo.


Overall, thanks for the reply and it is really helpful!
--------------------------------
Shuqi

[PATCH v3 0/3] modernize style

[Shuqi Liang]

different between V2:

1.change the commit massage in t4113: Modernize test script and t4113: indent with space.

2. Put the executable lines inside the test_expect_success.Mention the new style problem cause 
by this change,which is change the "<<" to "<<-" to strip leading TABs used for indentation.

Shuqi Liang (3):
  t/t4113-apply-ending.sh: Modernize test script
  t4113: put executable lines to test_expect_success
  t4113: indent with space

 t/t4113-apply-ending.sh | 79 ++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 41 deletions(-)

Range-diff against v2:
1:  d7d52f1f79 ! 1:  3d40bcce13 t/t4113-apply-ending.sh: Modernize a test script
    @@ Metadata
     Author: Shuqi Liang <cheskaqiqi@gmail.com>
     
      ## Commit message ##
    -    t/t4113-apply-ending.sh: Modernize a test script
    +    t/t4113-apply-ending.sh: Modernize test script
     
    +    Test scripts in file in this script are written in old style,
    +    where the test_expect_success command and test title are written on
    +    separate lines. Change the old style to modern style.
    +
    +    for example :
    +    -test_expect_success setup \
    +    -    'git update-index --add file'
    +    -
    +    +test_expect_success setup '
    +    +    git update-index --add file
    +    +'
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
     
      ## t/t4113-apply-ending.sh ##
2:  d9e5a54e32 ! 2:  5c55b208a8 Test scripts in file t4113-apply-ending.sh, files "test-patch" and "file" are created twice.
    @@ Metadata
     Author: Shuqi Liang <cheskaqiqi@gmail.com>
     
      ## Commit message ##
    -    Test scripts in file t4113-apply-ending.sh, files
    -    "test-patch" and "file" are created twice.
    +    t4113: put executable lines to test_expect_success
     
    -    Make the second creation of the files its own step
    -    'setup for apply at the beginning'.
    +    This script is written in old style,where there are
    +    some executable lines outside test_expect_success. Put the executable
    +    lines inside the test_expect_success.
    +
    +    As t/README says,use "<<-" instead of "<<"
    +    to strip leading TABs used for indentation. change the "<<" to "<<-"
    +
    +    for example:
    +    -cat >test-patch <<\EOF
    +    -diff a/file b/file
    +
    +     test_expect_success 'apply at the beginning' '
    +    +       cat >test-patch <<-\EOF
    +    +       diff a/file b/file
    +    +       --- a/file
     
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
     
    @@ t/t4113-apply-ending.sh: test_description='git apply trying to add an ending lin
      
     -# setup
     -
    - cat >test-patch <<\EOF
    - diff --git a/file b/file
    - --- a/file
    -@@ t/t4113-apply-ending.sh: echo 'c' >>file
    +-cat >test-patch <<\EOF
    +-diff --git a/file b/file
    +---- a/file
    +-+++ b/file
    +-@@ -1,2 +1,3 @@
    +- a
    +- b
    +-+c
    +-EOF
    +-
    +-echo 'a' >file
    +-echo 'b' >>file
    +-echo 'c' >>file
    +-
      test_expect_success setup '
    ++	cat >test-patch <<-\EOF
    ++	diff --git a/file b/file
    ++	--- a/file
    ++	+++ b/file
    ++	@@ -1,2 +1,3 @@
    ++	a
    ++	b
    ++	+c
    ++	EOF
    ++
    ++	echo 'a' >file
    ++	echo 'b' >>file
    ++	echo 'c' >>file
          git update-index --add file
      '
     -# test
    @@ -1,2 +1,3 @@
     -b
     -c'
     -git update-index file
    -+test_expect_success 'setup for apply at the beginning' '
    -+	cat >test-patch <<\EOF
    +-
    + test_expect_success 'apply at the beginning' '
    ++	cat >test-patch <<-\EOF
     +	diff a/file b/file
     +	--- a/file
     +	+++ b/file
    @@ -1,2 +1,3 @@
     +	b
     +	c'
     +	git update-index file
    -+'
    - 
    - test_expect_success 'apply at the beginning' '
          test_must_fail git apply --index test-patch
    + '
    + 
3:  01a5c3285e < -:  ---------- use "<<-" instead of "<<"
4:  cf2b2ca5a0 < -:  ---------- t4113-apply-ending.sh used 4-column indent with space,fix it in use tabs for indentation.
-:  ---------- > 3:  02b661279f t4113: indent with space
-- 
2.39.0

[PATCH v3 1/3]t4113: modernize a test script

[Shuqi Liang]

Test scripts in file in this script are written in old style,
where the test_expect_success command and test title are written on
separate lines. Change the old style to modern style.

for example :
-test_expect_success setup \
-    'git update-index --add file'
-
+test_expect_success setup '
+    git update-index --add file
+'
Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index 66fa51591e..aa57895b22 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -24,13 +24,14 @@ echo 'a' >file
 echo 'b' >>file
 echo 'c' >>file
 
-test_expect_success setup \
-    'git update-index --add file'
-
+test_expect_success setup '
+    git update-index --add file
+'
 # test
 
-test_expect_success 'apply at the end' \
-    'test_must_fail git apply --index test-patch'
+test_expect_success 'apply at the end' '
+    test_must_fail git apply --index test-patch
+'
 
 cat >test-patch <<\EOF
 diff a/file b/file
@@ -47,7 +48,8 @@ b
 c'
 git update-index file
 
-test_expect_success 'apply at the beginning' \
-	'test_must_fail git apply --index test-patch'
+test_expect_success 'apply at the beginning' '
+    test_must_fail git apply --index test-patch
+'
 
 test_done
-- 
2.39.0

[PATCH v3 2/3] t4113: put executable lines to test_expect_success

[Shuqi Liang]

This script is written in old style,where there are
some executable lines outside test_expect_success. Put the executable
lines inside the test_expect_success.

As t/README says,use "<<-" instead of "<<"
to strip leading TABs used for indentation. Change the "<<" to "<<-"

for example:
-cat >test-patch <<\EOF
-diff a/file b/file

 test_expect_success 'apply at the beginning' '
+	cat >test-patch <<-\EOF
+	diff a/file b/file
+	--- a/file

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 59 +++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index aa57895b22..e0a52a12c4 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -8,47 +8,42 @@ test_description='git apply trying to add an ending line.
 '
 . ./test-lib.sh
 
-# setup
-
-cat >test-patch <<\EOF
-diff --git a/file b/file
---- a/file
-+++ b/file
-@@ -1,2 +1,3 @@
- a
- b
-+c
-EOF
-
-echo 'a' >file
-echo 'b' >>file
-echo 'c' >>file
-
 test_expect_success setup '
+	cat >test-patch <<-\EOF
+	diff --git a/file b/file
+	--- a/file
+	+++ b/file
+	@@ -1,2 +1,3 @@
+	a
+	b
+	+c
+	EOF
+
+	echo 'a' >file
+	echo 'b' >>file
+	echo 'c' >>file
     git update-index --add file
 '
-# test
 
 test_expect_success 'apply at the end' '
     test_must_fail git apply --index test-patch
 '
 
-cat >test-patch <<\EOF
-diff a/file b/file
---- a/file
-+++ b/file
-@@ -1,2 +1,3 @@
-+a
- b
- c
-EOF
-
-echo >file 'a
-b
-c'
-git update-index file
-
 test_expect_success 'apply at the beginning' '
+	cat >test-patch <<-\EOF
+	diff a/file b/file
+	--- a/file
+	+++ b/file
+	@@ -1,2 +1,3 @@
+	+a
+	b
+	c
+	EOF
+
+	echo >file 'a
+	b
+	c'
+	git update-index file
     test_must_fail git apply --index test-patch
 '
 
-- 
2.39.0

[PATCH v3 3/3] t4113: indent with space

[Shuqi Liang]

As Documentation/CodingGuidelines says, the shell scripts
are to use tabs for indentation, but this script
uses 4-column indent with space. Fix it in use tabs for indentation.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index e0a52a12c4..ab5ecaab7f 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -22,11 +22,11 @@ test_expect_success setup '
 	echo 'a' >file
 	echo 'b' >>file
 	echo 'c' >>file
-    git update-index --add file
+	git update-index --add file
 '
 
 test_expect_success 'apply at the end' '
-    test_must_fail git apply --index test-patch
+	test_must_fail git apply --index test-patch
 '
 
 test_expect_success 'apply at the beginning' '
@@ -44,7 +44,7 @@ test_expect_success 'apply at the beginning' '
 	b
 	c'
 	git update-index file
-    test_must_fail git apply --index test-patch
+	test_must_fail git apply --index test-patch
 '
 
 test_done
-- 
2.39.0

Re: [PATCH v3 3/3] t4113: indent with space

[Eric Sunshine]

On Sun, Feb 5, 2023 at 9:56 AM Shuqi Liang <cheskaqiqi@gmail.com> wrote:
> t4113: indent with space

This probably ought to say "indent with tab" since that's what this
patch is doing.

> As Documentation/CodingGuidelines says, the shell scripts
> are to use tabs for indentation, but this script
> uses 4-column indent with space. Fix it in use tabs for indentation.

s/in use/to use/

> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
> diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
> index e0a52a12c4..ab5ecaab7f 100755
> --- a/t/t4113-apply-ending.sh
> +++ b/t/t4113-apply-ending.sh
> @@ -22,11 +22,11 @@ test_expect_success setup '
>         echo 'a' >file
>         echo 'b' >>file
>         echo 'c' >>file
> -    git update-index --add file
> +       git update-index --add file
>  '

As a GSoC microproject, v3 is probably "good enough", so there may not
be a compelling reason to re-roll.

If you do find a reason to re-roll, though, I might suggest swapping
patches 2 and 3 since the current organization leaves a mix of tab and
space indentation in the tests, which makes reviewers do extra work
since they have to look ahead in the patch series to see if you fix
the inconsistent indentation in a later patch.

Re: [PATCH v3 3/3] t4113: indent with space

[Shuqi Liang]

Hi, Eric

On Sun, Feb 5, 2023 at 3:30 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

> This probably ought to say "indent with tab" since that's what this
> patch is doing.

Thanks ,I will fix it .

> If you do find a reason to re-roll, though, I might suggest swapping
> patches 2 and 3 since the current organization leaves a mix of tab and
> space indentation in the tests, which makes reviewers do extra work
> since they have to look ahead in the patch series to see if you fix
> the inconsistent indentation in a later patch.

Yeah ,I didn‘t realize that .Thanks for reply! I will send the V4 soon.
----------
Thanks,
Shuqi

[PATCH v4 0/3] t4113: modernize style

[Shuqi Liang]

Slightly different from V3:

1.Swap patches 2 and 3 because in patch [v3 2/3] leaves a mix of tab and
space indentation in the tests

2.Change the commit message in patch [v3 3/3] to indent with tab.

Shuqi Liang (3):
  t4113: modernize test script
  t4113: indent with tab
  t4113: put executable lines to test_expect_success

 t/t4113-apply-ending.sh | 79 ++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 41 deletions(-)


base-commit: c48035d29b4e524aed3a32f0403676f0d9128863
-- 
2.39.0

[PATCH v4 1/3] t4113: modernize test script

[Shuqi Liang]

Test scripts in file in this script are written in old style,
where the test_expect_success command and test title are written on
separate lines. Change the old style to modern style.

for example :
-test_expect_success setup \
-    'git update-index --add file'
-
+test_expect_success setup '
+    git update-index --add file
+'
Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index 66fa51591e..aa57895b22 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -24,13 +24,14 @@ echo 'a' >file
 echo 'b' >>file
 echo 'c' >>file
 
-test_expect_success setup \
-    'git update-index --add file'
-
+test_expect_success setup '
+    git update-index --add file
+'
 # test
 
-test_expect_success 'apply at the end' \
-    'test_must_fail git apply --index test-patch'
+test_expect_success 'apply at the end' '
+    test_must_fail git apply --index test-patch
+'
 
 cat >test-patch <<\EOF
 diff a/file b/file
@@ -47,7 +48,8 @@ b
 c'
 git update-index file
 
-test_expect_success 'apply at the beginning' \
-	'test_must_fail git apply --index test-patch'
+test_expect_success 'apply at the beginning' '
+    test_must_fail git apply --index test-patch
+'
 
 test_done
-- 
2.39.0

[PATCH v4 2/3] t4113: indent with tab

[Shuqi Liang]

As Documentation/CodingGuidelines says, the shell scripts
are to use tabs for indentation, but this script
uses 4-column indent with space. Fix it in use tabs for indentation.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index aa57895b22..5ee177e8eb 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -25,12 +25,12 @@ echo 'b' >>file
 echo 'c' >>file
 
 test_expect_success setup '
-    git update-index --add file
+	git update-index --add file
 '
 # test
 
 test_expect_success 'apply at the end' '
-    test_must_fail git apply --index test-patch
+	test_must_fail git apply --index test-patch
 '
 
 cat >test-patch <<\EOF
@@ -49,7 +49,7 @@ c'
 git update-index file
 
 test_expect_success 'apply at the beginning' '
-    test_must_fail git apply --index test-patch
+	test_must_fail git apply --index test-patch
 '
 
 test_done
-- 
2.39.0

[PATCH v4 3/3] t4113: put executable lines to test_expect_success

[Shuqi Liang]

This script is written in old style,where there are
some executable lines outside test_expect_success. Put the executable
lines inside the test_expect_success.

As t/README says,use "<<-" instead of "<<"
to strip leading TABs used for indentation. Change the "<<" to "<<-"

for example:
-cat >test-patch <<\EOF
-diff a/file b/file

 test_expect_success 'apply at the beginning' '
+       cat >test-patch <<-\EOF
+       diff a/file b/file
+       --- a/file

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 59 +++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index 5ee177e8eb..ab5ecaab7f 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -8,47 +8,42 @@ test_description='git apply trying to add an ending line.
 '
 . ./test-lib.sh
 
-# setup
-
-cat >test-patch <<\EOF
-diff --git a/file b/file
---- a/file
-+++ b/file
-@@ -1,2 +1,3 @@
- a
- b
-+c
-EOF
-
-echo 'a' >file
-echo 'b' >>file
-echo 'c' >>file
-
 test_expect_success setup '
+	cat >test-patch <<-\EOF
+	diff --git a/file b/file
+	--- a/file
+	+++ b/file
+	@@ -1,2 +1,3 @@
+	a
+	b
+	+c
+	EOF
+
+	echo 'a' >file
+	echo 'b' >>file
+	echo 'c' >>file
 	git update-index --add file
 '
-# test
 
 test_expect_success 'apply at the end' '
 	test_must_fail git apply --index test-patch
 '
 
-cat >test-patch <<\EOF
-diff a/file b/file
---- a/file
-+++ b/file
-@@ -1,2 +1,3 @@
-+a
- b
- c
-EOF
-
-echo >file 'a
-b
-c'
-git update-index file
-
 test_expect_success 'apply at the beginning' '
+	cat >test-patch <<-\EOF
+	diff a/file b/file
+	--- a/file
+	+++ b/file
+	@@ -1,2 +1,3 @@
+	+a
+	b
+	c
+	EOF
+
+	echo >file 'a
+	b
+	c'
+	git update-index file
 	test_must_fail git apply --index test-patch
 '
 
-- 
2.39.0

Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 06 2023, Shuqi Liang wrote:

. ./test-lib.sh
>  
> -# setup
> -
> -cat >test-patch <<\EOF
> -diff --git a/file b/file
> ---- a/file
> -+++ b/file
> -@@ -1,2 +1,3 @@
> - a
> - b
> -+c
> -EOF
> -
> -echo 'a' >file
> -echo 'b' >>file
> -echo 'c' >>file
> -
>  test_expect_success setup '
> +	cat >test-patch <<-\EOF
> +	diff --git a/file b/file
> +	--- a/file
> +	+++ b/file
> +	@@ -1,2 +1,3 @@
> +	a
> +	b
> +	+c
> +	EOF
> +
> +	echo 'a' >file
> +	echo 'b' >>file
> +	echo 'c' >>file

I have not read the rest here, but this immediately fails with a very
large error from chain-lint by default, and even if you manually disable
it (which I assume you're doing, or just not testing these at all before
submission), you'll get:
	
	$ ./t4113-apply-ending.sh --no-chain-lint
	ok 1 - setup
	ok 2 - apply at the end
	ok 3 - apply at the beginning
	./t4113-apply-ending.sh: 44: b: not found
	./t4113-apply-ending.sh: 48: c
	        git update-index file
	        test_must_fail git apply --index test-patch
	: not found
	# passed all 3 test(s)
	1..3

Which shows that even with the &&-chaining fixed you have quoting issues
here, you're trying to execute 'b' etc.

I didn't read the rest of this topic, but please test with chain-lint,
see if there's any unexpected new output from the tests etc. before a v5
re-roll.

Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> -cat >test-patch <<\EOF
> -diff --git a/file b/file
> ---- a/file
> -+++ b/file
> -@@ -1,2 +1,3 @@
> - a
> - b
> -+c
> -EOF
> -
> -echo 'a' >file
> -echo 'b' >>file
> -echo 'c' >>file

Have you run the resulting test?

>  test_expect_success setup '
> +	cat >test-patch <<-\EOF
> +	diff --git a/file b/file
> +	--- a/file
> +	+++ b/file
> +	@@ -1,2 +1,3 @@
> +	a
> +	b
> +	+c
> +	EOF

This creates a "test-patch" file with lines 'a' and 'b' that are
common context lines without any whitespace before them, no?  The
original left the necessary single space in front of them (see the
line removed above).

Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success

[Shuqi Liang]

On Mon, Feb 6, 2023 at 5:44 PM Junio C Hamano <gitster@pobox.com> wrote:

> Have you run the resulting test?

My apologies for not testing after V1. That was a major oversight on
my part.  I'll make sure to thoroughly test before each submission to
avoid any issues with the code in the future.


> This creates a "test-patch" file with lines 'a' and 'b' that are
> common context lines without any whitespace before them, no?  The
> original left the necessary single space in front of them (see the
> line removed above).

I try to change the code to(left the necessary single space in front
of 'a' and 'b':

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index ab5ecaab7f..ef61a3187c 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -14,8 +14,8 @@ test_expect_success setup '
--- a/file
+++ b/file
@@ -1,2 +1,3 @@
- a
- b
+ a
+ b
+c
EOF

Here I only show one part ,but I fix two same issue in the V4 patch
and it still can not pass the test .
It say :

Test Summary Report

-------------------

t4113-apply-ending.sh (Wstat: 256 Tests: 0 Failed: 0)

  Non-zero exit status: 1

  Parse errors: No plan found in TAP output

Files=1, Tests=0,  0 wallclock secs ( 0.01 usr  0.01 sys +  0.05 cusr
0.02 csys =  0.09 CPU)

Result: FAIL.

I'm stumped as to why it's still failing. I've tried searching for
answers on StackOverflow, but I still can't figure it out.
----------------
Thanks,
Shuqi

Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 06 2023, Shuqi Liang wrote:

> On Mon, Feb 6, 2023 at 5:44 PM Junio C Hamano <gitster@pobox.com> wrote:
>
>> Have you run the resulting test?
>
> My apologies for not testing after V1. That was a major oversight on
> my part.  I'll make sure to thoroughly test before each submission to
> avoid any issues with the code in the future.
>
>
>> This creates a "test-patch" file with lines 'a' and 'b' that are
>> common context lines without any whitespace before them, no?  The
>> original left the necessary single space in front of them (see the
>> line removed above).
>
> I try to change the code to(left the necessary single space in front
> of 'a' and 'b':
>
> diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
> index ab5ecaab7f..ef61a3187c 100755
> --- a/t/t4113-apply-ending.sh
> +++ b/t/t4113-apply-ending.sh
> @@ -14,8 +14,8 @@ test_expect_success setup '
> --- a/file
> +++ b/file
> @@ -1,2 +1,3 @@
> - a
> - b
> + a
> + b
> +c
> EOF
>
> Here I only show one part ,but I fix two same issue in the V4 patch
> and it still can not pass the test .
> It say :
>
> Test Summary Report
>
> -------------------
>
> t4113-apply-ending.sh (Wstat: 256 Tests: 0 Failed: 0)
>
>   Non-zero exit status: 1
>
>   Parse errors: No plan found in TAP output
>
> Files=1, Tests=0,  0 wallclock secs ( 0.01 usr  0.01 sys +  0.05 cusr
> 0.02 csys =  0.09 CPU)
>
> Result: FAIL.
>
> I'm stumped as to why it's still failing. I've tried searching for
> answers on StackOverflow, but I still can't figure it out.
> ----------------
> Thanks,
> Shuqi

The error doesn't tell us much, instead of "make prove" or "prove
<name>" running e.g.:

	./t4113-apply-ending.sh -vixd

Gives you better output.

But this is almost certainly that you're trying to insert leading
whitespace into a line that's in a <<-EOF here-doc, the "-" part of that
means that your leading whitespace is being stripped.

A typical idiom for that is have a marker for the start of line, and
strip the whitespace with "sed". See this for existing examples:

	git grep 'sed.*\^.*>.*EOF'

Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success

[Shuqi Liang]

Hi Ævar,

On Tue, Feb 7, 2023 at 3:12 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:


> The error doesn't tell us much, instead of "make prove" or "prove
> <name>" running e.g.:
>
>         ./t4113-apply-ending.sh -vixd
>
> Gives you better output.

Thanks, this is really helpful.

> But this is almost certainly that you're trying to insert leading
> whitespace into a line that's in a <<-EOF here-doc, the "-" part of that
> means that your leading whitespace is being stripped.
>
> A typical idiom for that is have a marker for the start of line, and
> strip the whitespace with "sed". See this for existing examples:
>
>         git grep 'sed.*\^.*>.*EOF'

Thank you for the tip! I'll try to fix the problem as soon as possible.

---------
Thanks,
Shuqi

Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success

[Shuqi Liang]

Hi Ævar,

On Tue, Feb 7, 2023 at 3:12 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> But this is almost certainly that you're trying to insert leading
> whitespace into a line that's in a <<-EOF here-doc, the "-" part of that
> means that your leading whitespace is being stripped.
>
> A typical idiom for that is have a marker for the start of line, and
> strip the whitespace with "sed". See this for existing examples:
>
>         git grep 'sed.*\^.*>.*EOF'


I try to use Z as the marker in front of 'a' and 'b' and use sed -e
"s/Z/ /g" in order to replace Z with white space but it still can not
pass the test.

Then I realize even if I don't add tab in front of the line but with
space in front of 'a' and 'b' like the original test script. It still
says it can't read "b" and "c” ：

test_expect_success 'apply at the beginning' '
cat >test-patch<<\EOF &&
diff a/file b/file
--- a/file
+++ b/file
@@ -1,2 +1,3 @@
+a
 b
 c
EOF

echo >file 'a
b
c'&&
git update-index file&&
test_must_fail git apply --index test-patch
'
Maybe the error is not caused by whitespace?

Then I try to change:

echo >file 'a
b
c'

To:
echo >file "a
b
c"

Then everything passes the test. I think double quotes allow for
variable substitution and command substitution, while single quotes
preserve the literal value of all characters within the quotes. In
this case, the string contains no variables or commands, so either
type of quote would work. Is there something wrong with my idea? Is it
good to modify code like that?

Looking forward to your reply!

------
Shuqi

Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 08 2023, Shuqi Liang wrote:

> On Tue, Feb 7, 2023 at 3:12 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>> But this is almost certainly that you're trying to insert leading
>> whitespace into a line that's in a <<-EOF here-doc, the "-" part of that
>> means that your leading whitespace is being stripped.
>>
>> A typical idiom for that is have a marker for the start of line, and
>> strip the whitespace with "sed". See this for existing examples:
>>
>>         git grep 'sed.*\^.*>.*EOF'
>
>
> I try to use Z as the marker in front of 'a' and 'b' and use sed -e
> "s/Z/ /g" in order to replace Z with white space but it still can not
> pass the test.
>
> Then I realize even if I don't add tab in front of the line but with
> space in front of 'a' and 'b' like the original test script. It still
> says it can't read "b" and "c” ：
>
> test_expect_success 'apply at the beginning' '
> cat >test-patch<<\EOF &&
> diff a/file b/file
> --- a/file
> +++ b/file
> @@ -1,2 +1,3 @@
> +a
>  b
>  c
> EOF
>
> echo >file 'a
> b
> c'&&
> git update-index file&&
> test_must_fail git apply --index test-patch
> '
> Maybe the error is not caused by whitespace?
>
> Then I try to change:
>
> echo >file 'a
> b
> c'
>
> To:
> echo >file "a
> b
> c"
>
> Then everything passes the test. I think double quotes allow for
> variable substitution and command substitution, while single quotes
> preserve the literal value of all characters within the quotes. In
> this case, the string contains no variables or commands, so either
> type of quote would work. Is there something wrong with my idea? Is it
> good to modify code like that?
>
> Looking forward to your reply!

I'm not sure because you're just posting snippets, if you have problems
in the future it would be best to post the full diff to "master" that
you're having issues with, e.g. an RFC per Documentation/SubmittingPatches.

But I think this is because the test itself is using '-quotes, so you
need to use '\'' if you want to single quote, and " for double quotes,
and \" if the test were in double quotes.

But the issues you're having here aren't with Git, but the very basics
of POSIX shell syntax.

I think it would be good for you to read some basic documentation on
POSIX shells, their syntax, common POSIX commands etc. Your local "man
sh" is probably a good place to start, but there's also books, online
tutorials etc.

In this case the syntax you're trying to get working is something we
usually try to avoid in either case, i.e. even if it involves an
external process we usually do:

	cat >out <<-\EOF
	a
        b
	c
	EOF

Rather than:

	echo "a
        b
	c" >out

If you are using "echo" I saw another change of yours had e.g.:

	echo x >f &&
	echo y >>f &&
	echo z >>f

It's better to e.g. (assuming use of "echo", or other built-ins or
commands):

	{
		echo x &&
		echo y &&
		echo z
	} >f

[PATCH v5 0/3] t4113: modernize style

[Shuqi Liang]

different with V4: 
1.chain test assertions

For example:

-cat >test-patch <<\EOF
-diff --git a/file b/file

+ cat >test-patch <<-\EOF &&
+ diff --git a/file b/file


2.change the modern style with echo 
   echo x >file &&
        echo y >>file &&
        echo z >>file

  Change it to this stlye :
        {
        echo x &&
        echo y &&
        echo z
        } >file

3.In order to escape for executable lines inside the test_expect_success.
Change ' in executable lines to '\'' in order to escape.



(V4 didn't test before sumbit so I show the difference between orgin t4113 )

Shuqi Liang (3):
  t4113: modernize test script
  t4113: indent with tab
  t4113: put executable lines to test_expect_success

 t/t4113-apply-ending.sh | 81 ++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 41 deletions(-)


base-commit: c48035d29b4e524aed3a32f0403676f0d9128863
-- 
2.39.0

[PATCH v5 1/3] t4113: modernize test script

[Shuqi Liang]

Test scripts in file in this script are written in old style,
where the test_expect_success command and test title are written on
separate lines. Change the old style to modern style.

for example :
-test_expect_success setup \
-    'git update-index --add file'
-
+test_expect_success setup '
+    git update-index --add file
+'

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index 66fa51591e..41526ca805 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -24,14 +24,14 @@ echo 'a' >file
 echo 'b' >>file
 echo 'c' >>file
 
-test_expect_success setup \
-    'git update-index --add file'
-
+test_expect_success setup '
+    git update-index --add file
+'
 # test
 
-test_expect_success 'apply at the end' \
-    'test_must_fail git apply --index test-patch'
-
+test_expect_success 'apply at the end' '
+    test_must_fail git apply --index test-patch
+'
 cat >test-patch <<\EOF
 diff a/file b/file
 --- a/file
@@ -47,7 +47,7 @@ b
 c'
 git update-index file
 
-test_expect_success 'apply at the beginning' \
-	'test_must_fail git apply --index test-patch'
-
+test_expect_success 'apply at the beginning' '
+	test_must_fail git apply --index test-patch
+'
 test_done
-- 
2.39.0

[PATCH v5 2/3] t4113: indent with tab

[Shuqi Liang]

As Documentation/CodingGuidelines says, the shell scripts
are to use tabs for indentation, but this script
uses 4-column indent with space. Fix it in use tabs for indentation.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index 41526ca805..a470c9ce7b 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -25,12 +25,12 @@ echo 'b' >>file
 echo 'c' >>file
 
 test_expect_success setup '
-    git update-index --add file
+	git update-index --add file
 '
 # test
 
 test_expect_success 'apply at the end' '
-    test_must_fail git apply --index test-patch
+	test_must_fail git apply --index test-patch
 '
 cat >test-patch <<\EOF
 diff a/file b/file
-- 
2.39.0

[PATCH v5 3/3] t4113: put executable lines to test_expect_success

[Shuqi Liang]

As t/README says, put all code inside test_expect_success and
other assertions. This script is written in old style,where there are
some executable lines outside test_expect_success. Put the executable
lines inside the test_expect_success.

As t/README says,use "<<-" instead of "<<"
to strip leading TABs used for indentation. Change the "<<" to "<<-"

for example:
-cat >test-patch <<\EOF
-diff a/file b/file

 test_expect_success 'apply at the beginning' '
+       cat >test-patch <<-\EOF
+       diff a/file b/file
+       --- a/file

As t/README says,chain test assertions.Chain this test assertions
with &&.

For example:

-cat >test-patch <<\EOF
-diff --git a/file b/file

+ cat >test-patch <<-\EOF &&
+ diff --git a/file b/file

This script is written in old style,where there are something like

        echo x >file &&
        echo y >>file &&
        echo z >>file

  Change it to this stlye :
        {
        echo x &&
        echo y &&
        echo z
        } >file

In order to escape for executable lines inside the test_expect_success.
Change ' in executable lines to '\'' in order to escape.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 61 ++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index a470c9ce7b..c70429bd07 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -8,46 +8,45 @@ test_description='git apply trying to add an ending line.
 '
 . ./test-lib.sh
 
-# setup
-
-cat >test-patch <<\EOF
-diff --git a/file b/file
---- a/file
-+++ b/file
-@@ -1,2 +1,3 @@
- a
- b
-+c
-EOF
-
-echo 'a' >file
-echo 'b' >>file
-echo 'c' >>file
-
 test_expect_success setup '
+	cat >test-patch <<-\EOF &&
+	diff --git a/file b/file
+	--- a/file
+	+++ b/file
+	@@ -1,2 +1,3 @@
+	 a
+	 b
+	+c
+	EOF
+
+	{
+	echo '\''a'\'' &&
+	echo '\''b'\'' &&
+	echo '\''c'\''
+	} >file &&
 	git update-index --add file
 '
-# test
 
 test_expect_success 'apply at the end' '
 	test_must_fail git apply --index test-patch
 '
-cat >test-patch <<\EOF
-diff a/file b/file
---- a/file
-+++ b/file
-@@ -1,2 +1,3 @@
-+a
- b
- c
-EOF
-
-echo >file 'a
-b
-c'
-git update-index file
 
 test_expect_success 'apply at the beginning' '
+	cat >test-patch <<-\EOF &&
+	diff a/file b/file
+	--- a/file
+	+++ b/file
+	@@ -1,2 +1,3 @@
+	+a
+	 b
+	 c
+	EOF
+
+	echo >file '\''a
+	b
+	c'\'' &&
+	git update-index file &&
 	test_must_fail git apply --index test-patch
 '
+
 test_done
-- 
2.39.0

Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success

[Shuqi Liang]

Hi Ævar ,

Sorry if  I sent this email twice. I forget to CC  git@vger.kernel.org.

On Wed, Feb 8, 2023 at 2:50 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> I'm not sure because you're just posting snippets, if you have problems
> in the future it would be best to post the full diff to "master" that
> you're having issues with, e.g. an RFC per Documentation/SubmittingPatches.

Yeah, posting snippets doesn't describe the problem very well. Thanks
for the advice.



> But I think this is because the test itself is using '-quotes, so you
> need to use '\'' if you want to single quote, and " for double quotes,
> and \" if the test were in double quotes.


> But the issues you're having here aren't with Git, but the very basics
> of POSIX shell syntax.

> I think it would be good for you to read some basic documentation on
> POSIX shells, their syntax, common POSIX commands etc. Your local "man
> sh" is probably a good place to start, but there's also books, online
> tutorials etc.

Thanks, will do .  I'll  try to avoid making mistakes on POSIX shells
questions and really learn the basic  POSIX shells syntax.

> In this case the syntax you're trying to get working is something we
> usually try to avoid in either case, i.e. even if it involves an
> external process we usually do:

>         cat >out <<-\EOF
>         a
>         b
>         c
>         EOF
>
> Rather than:
>
>         echo "a
>         b
>         c" >out
>
> If you are using "echo" I saw another change of yours had e.g.:
>
>         echo x >f &&
>         echo y >>f &&
>         echo z >>f
>
> It's better to e.g. (assuming use of "echo", or other built-ins or
> commands):
>
>         {
>                 echo x &&
>                 echo y &&
>                 echo z
>         } >f

Seen I pass the test s, I'v a submit  V5 patch instead of RFC, Would
you mind taking a look at this for me? Looking forward to reply.

-----------------------------
Thanks

Shuqi

On Wed, Feb 8, 2023 at 2:50 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> On Wed, Feb 08 2023, Shuqi Liang wrote:
>
> > On Tue, Feb 7, 2023 at 3:12 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >
> >> But this is almost certainly that you're trying to insert leading
> >> whitespace into a line that's in a <<-EOF here-doc, the "-" part of that
> >> means that your leading whitespace is being stripped.
> >>
> >> A typical idiom for that is have a marker for the start of line, and
> >> strip the whitespace with "sed". See this for existing examples:
> >>
> >>         git grep 'sed.*\^.*>.*EOF'
> >
> >
> > I try to use Z as the marker in front of 'a' and 'b' and use sed -e
> > "s/Z/ /g" in order to replace Z with white space but it still can not
> > pass the test.
> >
> > Then I realize even if I don't add tab in front of the line but with
> > space in front of 'a' and 'b' like the original test script. It still
> > says it can't read "b" and "c” ：
> >
> > test_expect_success 'apply at the beginning' '
> > cat >test-patch<<\EOF &&
> > diff a/file b/file
> > --- a/file
> > +++ b/file
> > @@ -1,2 +1,3 @@
> > +a
> >  b
> >  c
> > EOF
> >
> > echo >file 'a
> > b
> > c'&&
> > git update-index file&&
> > test_must_fail git apply --index test-patch
> > '
> > Maybe the error is not caused by whitespace?
> >
> > Then I try to change:
> >
> > echo >file 'a
> > b
> > c'
> >
> > To:
> > echo >file "a
> > b
> > c"
> >
> > Then everything passes the test. I think double quotes allow for
> > variable substitution and command substitution, while single quotes
> > preserve the literal value of all characters within the quotes. In
> > this case, the string contains no variables or commands, so either
> > type of quote would work. Is there something wrong with my idea? Is it
> > good to modify code like that?
> >
> > Looking forward to your reply!
>
> I'm not sure because you're just posting snippets, if you have problems
> in the future it would be best to post the full diff to "master" that
> you're having issues with, e.g. an RFC per Documentation/SubmittingPatches.
>
> But I think this is because the test itself is using '-quotes, so you
> need to use '\'' if you want to single quote, and " for double quotes,
> and \" if the test were in double quotes.
>
> But the issues you're having here aren't with Git, but the very basics
> of POSIX shell syntax.
>
> I think it would be good for you to read some basic documentation on
> POSIX shells, their syntax, common POSIX commands etc. Your local "man
> sh" is probably a good place to start, but there's also books, online
> tutorials etc.
>
> In this case the syntax you're trying to get working is something we
> usually try to avoid in either case, i.e. even if it involves an
> external process we usually do:
>
>         cat >out <<-\EOF
>         a
>         b
>         c
>         EOF
>
> Rather than:
>
>         echo "a
>         b
>         c" >out
>
> If you are using "echo" I saw another change of yours had e.g.:
>
>         echo x >f &&
>         echo y >>f &&
>         echo z >>f
>
> It's better to e.g. (assuming use of "echo", or other built-ins or
> commands):
>
>         {
>                 echo x &&
>                 echo y &&
>                 echo z
>         } >f

Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success

[Shuqi Liang]

Hi Junio,

I didn't see this change in " What's cooking in git.git". I'm not sure
if the V5 patch is overlooked. I didn't receive any review after V5.
Is there anything wrong in V5 that needs to be corrected?

Thanks,
Shuqi

Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> I didn't see this change in " What's cooking in git.git". I'm not sure
> if the V5 patch is overlooked. I didn't receive any review after V5.
> Is there anything wrong in V5 that needs to be corrected?

I dunno (yet).  These days a day did not have enough time to be
looking at all the patches on the list, and patches that are more
about practice than fixing real bugs or adding real features tend to
be placed on the back burner.

THanks for pinging.

Re: [PATCH v5 2/3] t4113: indent with tab

[Eric Sunshine]

On Thu, Feb 9, 2023 at 10:50 AM Shuqi Liang <cheskaqiqi@gmail.com> wrote:
> As Documentation/CodingGuidelines says, the shell scripts
> are to use tabs for indentation, but this script
> uses 4-column indent with space. Fix it in use tabs for indentation.

s/in use/to use/

> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>

The patch itself looks fine.

Re: [PATCH v5 2/3] t4113: indent with tab

[Shuqi Liang]

Hi, Eric

On Tue, Feb 14, 2023 at 7:11 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

> The patch itself looks fine.

Thank you for your recognition!


------
Thanks
Shuqi

Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success

[Eric Sunshine]

On Tue, Feb 7, 2023 at 3:19 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Mon, Feb 06 2023, Shuqi Liang wrote:
> > On Mon, Feb 6, 2023 at 5:44 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> This creates a "test-patch" file with lines 'a' and 'b' that are
> >> common context lines without any whitespace before them, no?  The
> >> original left the necessary single space in front of them (see the
> >> line removed above).
> >
> > I try to change the code to(left the necessary single space in front
> > of 'a' and 'b':
> >
> > @@ -1,2 +1,3 @@
> > - a
> > - b
> > + a
> > + b
> > +c
> > EOF
> >
> > t4113-apply-ending.sh (Wstat: 256 Tests: 0 Failed: 0)
> > Result: FAIL.
> >
> > I'm stumped as to why it's still failing. I've tried searching for
> > answers on StackOverflow, but I still can't figure it out.
>
> But this is almost certainly that you're trying to insert leading
> whitespace into a line that's in a <<-EOF here-doc, the "-" part of that
> means that your leading whitespace is being stripped.

Almost. The `<<-` operator actually only strips leading TABs; other
whitespace following the TABs is left intact.

Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success

[Eric Sunshine]

On Wed, Feb 8, 2023 at 2:57 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> In this case the syntax you're trying to get working is something we
> usually try to avoid in either case, i.e. even if it involves an
> external process we usually do:
>
>         cat >out <<-\EOF
>         a
>         b
>         c
>         EOF
>
> Rather than:
>
>         echo "a
>         b
>         c" >out

The here-doc (<<-\EOF) form is definitely a good idea when the code is
part of an indented test body, whereas the multi-line double-quoted
string will be problematic since lines "b" and "c" will be indented
with TAB, which is undesirable here.

Even better for such a simple case would be:

    test_write_lines a b c >out &&

In fact, Junio made this suggestion in the form of a code snipped much
earlier in this thread.

> If you are using "echo" I saw another change of yours had e.g.:
>
>         echo x >f &&
>         echo y >>f &&
>         echo z >>f
>
> It's better to e.g. (assuming use of "echo", or other built-ins or
> commands):
>
>         {
>                 echo x &&
>                 echo y &&
>                 echo z
>         } >f

This is also an improvement, though test_write_lines would (again) be
even better for such a simple case:

    test_write_lines x y z >f &&

Re: [PATCH v5 3/3] t4113: put executable lines to test_expect_success

[Eric Sunshine]

On Thu, Feb 9, 2023 at 11:00 AM Shuqi Liang <cheskaqiqi@gmail.com> wrote:
> As t/README says, put all code inside test_expect_success and
> other assertions. This script is written in old style,where there are
> some executable lines outside test_expect_success. Put the executable
> lines inside the test_expect_success.

Although it's true that t/README explains why code should be placed
inside tests, you can help readers out by simply explaining the reason
here in the commit message. For instance, you might replace the above
paragraph with:

    Some old test scripts have setup code outside of tests. This
    is problematic since any failures of the setup code will go
    unnoticed. Therefore, move setup code into the tests themselves
    so that failures are properly flagged.

As for the rest of the commit message...

> As t/README says,use "<<-" instead of "<<"
> to strip leading TABs used for indentation. Change the "<<" to "<<-"
>
> for example:
> -cat >test-patch <<\EOF
> -diff a/file b/file
>
>  test_expect_success 'apply at the beginning' '
> +       cat >test-patch <<-\EOF
> +       diff a/file b/file
> +       --- a/file

Certain changes are considered obvious by reviewers, so you don't need
to mention them explicitly in the commit message. This is one such
change. Any reviewer who sees that you indented the here-doc body to
match the indentation of the rest of the test body will understand why
you changed `<<` to `<<-` without the commit message having to explain
it.

> As t/README says,chain test assertions.Chain this test assertions
> with &&.
>
> For example:
>
> -cat >test-patch <<\EOF
> -diff --git a/file b/file
>
> + cat >test-patch <<-\EOF &&
> + diff --git a/file b/file

Same thing. Reviewers understand that all code inside a test body must
have an intact &&-chain, so you needn't mention this in the commit
message.

> This script is written in old style,where there are something like
>
>         echo x >file &&
>         echo y >>file &&
>         echo z >>file
>
>   Change it to this stlye :
>         {
>         echo x &&
>         echo y &&
>         echo z
>         } >file

This is similar. This is such a simple style change, and the code
fragment itself is so tiny, that a reviewer can understand this change
without the commit message spelling it out.

> In order to escape for executable lines inside the test_expect_success.
> Change ' in executable lines to '\'' in order to escape.

Likewise.

Reviewers appreciate well-explained commit messages, but they also
appreciate succinctness. Although it may not always be obvious how
much to write in a commit message, you can assume that reviewers will
understand obvious changes simply by reading the patch itself, thus
you don't need to mention every little detail in the commit message.
The important thing to mention in the commit message is the
explanation of _why_ the change is being made, plus any changes which
might not be obvious. In this case, all the changes are obvious, so,
really, you can collapse this entire commit message to just the first
paragraph.

> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
> diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
> @@ -8,46 +8,45 @@ test_description='git apply trying to add an ending line.
> -# setup
> -

Good to see that you got rid of the now-unnecessary comment.

> -cat >test-patch <<\EOF
> -diff --git a/file b/file
> ---- a/file
> -+++ b/file
> -@@ -1,2 +1,3 @@
> - a
> - b
> -+c
> -EOF
> -
> -echo 'a' >file
> -echo 'b' >>file
> -echo 'c' >>file
> -
>  test_expect_success setup '
> +       cat >test-patch <<-\EOF &&
> +       diff --git a/file b/file
> +       --- a/file
> +       +++ b/file
> +       @@ -1,2 +1,3 @@
> +        a
> +        b
> +       +c
> +       EOF

Okay.

> +       {
> +       echo '\''a'\'' &&
> +       echo '\''b'\'' &&
> +       echo '\''c'\''
> +       } >file &&

A few comments:

This is unnecessarily confusing. Although this does work, it would be
sufficient just to change the single-quotes to double-quotes, like
this:

    {
    echo "a" &&
    echo "b" &&
    echo "c"
    } >file &&

Even simpler, you could drop the quotes altogether for such a simple case:

    {
    echo a &&
    echo b &&
    echo c
    } >file &&

However, as mentioned elsewhere in this thread, a really succinct way
to do this, taking advantage of modern style would be to use
test_write_lines(), so the five lines collapse to a single line:

    test_write_lines a b c >file &&

> -cat >test-patch <<\EOF
> -diff a/file b/file
> ---- a/file
> -+++ b/file
> -@@ -1,2 +1,3 @@
> -+a
> - b
> - c
> -EOF
> -
> -echo >file 'a
> -b
> -c'
> -git update-index file
>
>  test_expect_success 'apply at the beginning' '
> +       cat >test-patch <<-\EOF &&
> +       diff a/file b/file
> +       --- a/file
> +       +++ b/file
> +       @@ -1,2 +1,3 @@
> +       +a
> +        b
> +        c
> +       EOF
> +
> +       echo >file '\''a
> +       b
> +       c'\'' &&

Same comment about simply using double-quotes instead of
single-quotes, however, this is also another really good place to use
test_write_lines:

    test_write_lines a b c >file &&

[PATCH v6 0/3] t4113: modernize style

[Shuqi Liang]

different with 5 :

1.change the commit message to be more succinct.

2.
use  

test_write_lines a b c >file && 

instead of :
 
{
    echo a &&
    echo b &&
    echo c
} >file &&




Shuqi Liang (3):
  t4113: modernize test script
  t4113: indent with tab
  t4113: put executable lines to test_expect_success

 t/t4113-apply-ending.sh | 75 +++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 41 deletions(-)

Range-diff against v5:
1:  4d55e522a6 ! 1:  cb29da5d42 t4113: modernize test script
    @@ Commit message
         where the test_expect_success command and test title are written on
         separate lines. Change the old style to modern style.
     
    -    for example :
    -    -test_expect_success setup \
    -    -    'git update-index --add file'
    -    -
    -    +test_expect_success setup '
    -    +    git update-index --add file
    -    +'
    -
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
     
      ## t/t4113-apply-ending.sh ##
2:  cb997bc422 = 2:  09efa23ba4 t4113: indent with tab
3:  726d2fcb47 ! 3:  0169cbd402 t4113: put executable lines to test_expect_success
    @@ Commit message
         t4113: put executable lines to test_expect_success
     
         As t/README says, put all code inside test_expect_success and
    -    other assertions. This script is written in old style,where there are
    -    some executable lines outside test_expect_success. Put the executable
    -    lines inside the test_expect_success.
    -
    -    As t/README says,use "<<-" instead of "<<"
    -    to strip leading TABs used for indentation. Change the "<<" to "<<-"
    -
    -    for example:
    -    -cat >test-patch <<\EOF
    -    -diff a/file b/file
    -
    -     test_expect_success 'apply at the beginning' '
    -    +       cat >test-patch <<-\EOF
    -    +       diff a/file b/file
    -    +       --- a/file
    -
    -    As t/README says,chain test assertions.Chain this test assertions
    -    with &&.
    -
    -    For example:
    -
    -    -cat >test-patch <<\EOF
    -    -diff --git a/file b/file
    -
    -    + cat >test-patch <<-\EOF &&
    -    + diff --git a/file b/file
    -
    -    This script is written in old style,where there are something like
    -
    -            echo x >file &&
    -            echo y >>file &&
    -            echo z >>file
    -
    -      Change it to this stlye :
    -            {
    -            echo x &&
    -            echo y &&
    -            echo z
    -            } >file
    -
    -    In order to escape for executable lines inside the test_expect_success.
    -    Change ' in executable lines to '\'' in order to escape.
    +    other assertions. This old test scripts have setup code
    +    outside of tests. This is problematic since any failures of the
    +    setup code will go unnoticed. Therefore, move setup code into the tests
    +    themselves so that failures are properly flagged. t/README also says,
    +    use "<<-" instead of "<<" to strip leading TABs used for indentation.
    +    Fix it. We should chain test assertions(t/README). Therefore,Chain
    +    this test assertions with &&. What's more,take advantage of modern
    +    style. Use test_write_lines instead.
     
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
     
    @@ -1,2 +1,3 @@
     +	+c
     +	EOF
     +
    -+	{
    -+	echo '\''a'\'' &&
    -+	echo '\''b'\'' &&
    -+	echo '\''c'\''
    -+	} >file &&
    ++	test_write_lines a b c >file &&
      	git update-index --add file
      '
     -# test
    @@ -1,2 +1,3 @@
     +	 c
     +	EOF
     +
    -+	echo >file '\''a
    -+	b
    -+	c'\'' &&
    ++	test_write_lines a b c >file &&
     +	git update-index file &&
      	test_must_fail git apply --index test-patch
      '
-- 
2.39.0

[PATCH v6 0/3] t4113: modernize style

[Shuqi Liang]

different with 5 :

1.change the commit message to be more succinct.

2.
use  

test_write_lines a b c >file && 

instead of :
 
{
    echo a &&
    echo b &&
    echo c
} >file &&




Shuqi Liang (3):
  t4113: modernize test script
  t4113: indent with tab
  t4113: put executable lines to test_expect_success

 t/t4113-apply-ending.sh | 75 +++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 41 deletions(-)

Range-diff against v5:
1:  4d55e522a6 ! 1:  cb29da5d42 t4113: modernize test script
    @@ Commit message
         where the test_expect_success command and test title are written on
         separate lines. Change the old style to modern style.
     
    -    for example :
    -    -test_expect_success setup \
    -    -    'git update-index --add file'
    -    -
    -    +test_expect_success setup '
    -    +    git update-index --add file
    -    +'
    -
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
     
      ## t/t4113-apply-ending.sh ##
2:  cb997bc422 = 2:  09efa23ba4 t4113: indent with tab
3:  726d2fcb47 ! 3:  0169cbd402 t4113: put executable lines to test_expect_success
    @@ Commit message
         t4113: put executable lines to test_expect_success
     
         As t/README says, put all code inside test_expect_success and
    -    other assertions. This script is written in old style,where there are
    -    some executable lines outside test_expect_success. Put the executable
    -    lines inside the test_expect_success.
    -
    -    As t/README says,use "<<-" instead of "<<"
    -    to strip leading TABs used for indentation. Change the "<<" to "<<-"
    -
    -    for example:
    -    -cat >test-patch <<\EOF
    -    -diff a/file b/file
    -
    -     test_expect_success 'apply at the beginning' '
    -    +       cat >test-patch <<-\EOF
    -    +       diff a/file b/file
    -    +       --- a/file
    -
    -    As t/README says,chain test assertions.Chain this test assertions
    -    with &&.
    -
    -    For example:
    -
    -    -cat >test-patch <<\EOF
    -    -diff --git a/file b/file
    -
    -    + cat >test-patch <<-\EOF &&
    -    + diff --git a/file b/file
    -
    -    This script is written in old style,where there are something like
    -
    -            echo x >file &&
    -            echo y >>file &&
    -            echo z >>file
    -
    -      Change it to this stlye :
    -            {
    -            echo x &&
    -            echo y &&
    -            echo z
    -            } >file
    -
    -    In order to escape for executable lines inside the test_expect_success.
    -    Change ' in executable lines to '\'' in order to escape.
    +    other assertions. This old test scripts have setup code
    +    outside of tests. This is problematic since any failures of the
    +    setup code will go unnoticed. Therefore, move setup code into the tests
    +    themselves so that failures are properly flagged. t/README also says,
    +    use "<<-" instead of "<<" to strip leading TABs used for indentation.
    +    Fix it. We should chain test assertions(t/README). Therefore,Chain
    +    this test assertions with &&. What's more,take advantage of modern
    +    style. Use test_write_lines instead.
     
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
     
    @@ -1,2 +1,3 @@
     +	+c
     +	EOF
     +
    -+	{
    -+	echo '\''a'\'' &&
    -+	echo '\''b'\'' &&
    -+	echo '\''c'\''
    -+	} >file &&
    ++	test_write_lines a b c >file &&
      	git update-index --add file
      '
     -# test
    @@ -1,2 +1,3 @@
     +	 c
     +	EOF
     +
    -+	echo >file '\''a
    -+	b
    -+	c'\'' &&
    ++	test_write_lines a b c >file &&
     +	git update-index file &&
      	test_must_fail git apply --index test-patch
      '
-- 
2.39.0

[PATCH v6 1/3] t4113: modernize test script

[Shuqi Liang]

Test scripts in file in this script are written in old style,
where the test_expect_success command and test title are written on
separate lines. Change the old style to modern style.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index 66fa51591e..41526ca805 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -24,14 +24,14 @@ echo 'a' >file
 echo 'b' >>file
 echo 'c' >>file
 
-test_expect_success setup \
-    'git update-index --add file'
-
+test_expect_success setup '
+    git update-index --add file
+'
 # test
 
-test_expect_success 'apply at the end' \
-    'test_must_fail git apply --index test-patch'
-
+test_expect_success 'apply at the end' '
+    test_must_fail git apply --index test-patch
+'
 cat >test-patch <<\EOF
 diff a/file b/file
 --- a/file
@@ -47,7 +47,7 @@ b
 c'
 git update-index file
 
-test_expect_success 'apply at the beginning' \
-	'test_must_fail git apply --index test-patch'
-
+test_expect_success 'apply at the beginning' '
+	test_must_fail git apply --index test-patch
+'
 test_done
-- 
2.39.0

[PATCH v6 2/3] t4113: indent with tab

[Shuqi Liang]

As Documentation/CodingGuidelines says, the shell scripts
are to use tabs for indentation, but this script
uses 4-column indent with space. Fix it in use tabs for indentation.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index 41526ca805..a470c9ce7b 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -25,12 +25,12 @@ echo 'b' >>file
 echo 'c' >>file
 
 test_expect_success setup '
-    git update-index --add file
+	git update-index --add file
 '
 # test
 
 test_expect_success 'apply at the end' '
-    test_must_fail git apply --index test-patch
+	test_must_fail git apply --index test-patch
 '
 cat >test-patch <<\EOF
 diff a/file b/file
-- 
2.39.0

[PATCH v6 3/3] t4113: put executable lines to test_expect_success

[Shuqi Liang]

As t/README says, put all code inside test_expect_success and
other assertions. This old test scripts have setup code
outside of tests. This is problematic since any failures of the
setup code will go unnoticed. Therefore, move setup code into the tests
themselves so that failures are properly flagged. t/README also says,
use "<<-" instead of "<<" to strip leading TABs used for indentation.
Fix it. We should chain test assertions(t/README). Therefore,Chain
this test assertions with &&. What's more,take advantage of modern
style. Use test_write_lines instead.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t4113-apply-ending.sh | 55 ++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 31 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index a470c9ce7b..56fc2f436b 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -8,46 +8,39 @@ test_description='git apply trying to add an ending line.
 '
 . ./test-lib.sh
 
-# setup
-
-cat >test-patch <<\EOF
-diff --git a/file b/file
---- a/file
-+++ b/file
-@@ -1,2 +1,3 @@
- a
- b
-+c
-EOF
-
-echo 'a' >file
-echo 'b' >>file
-echo 'c' >>file
-
 test_expect_success setup '
+	cat >test-patch <<-\EOF &&
+	diff --git a/file b/file
+	--- a/file
+	+++ b/file
+	@@ -1,2 +1,3 @@
+	 a
+	 b
+	+c
+	EOF
+
+	test_write_lines a b c >file &&
 	git update-index --add file
 '
-# test
 
 test_expect_success 'apply at the end' '
 	test_must_fail git apply --index test-patch
 '
-cat >test-patch <<\EOF
-diff a/file b/file
---- a/file
-+++ b/file
-@@ -1,2 +1,3 @@
-+a
- b
- c
-EOF
-
-echo >file 'a
-b
-c'
-git update-index file
 
 test_expect_success 'apply at the beginning' '
+	cat >test-patch <<-\EOF &&
+	diff a/file b/file
+	--- a/file
+	+++ b/file
+	@@ -1,2 +1,3 @@
+	+a
+	 b
+	 c
+	EOF
+
+	test_write_lines a b c >file &&
+	git update-index file &&
 	test_must_fail git apply --index test-patch
 '
+
 test_done
-- 
2.39.0

Re: [PATCH v5 3/3] t4113: put executable lines to test_expect_success

[Shuqi Liang]

On Tue, Feb 14, 2023 at 8:09 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

> Although it's true that t/README explains why code should be placed
> inside tests, you can help readers out by simply explaining the reason
> here in the commit message. For instance, you might replace the above
> paragraph with:
>
>     Some old test scripts have setup code outside of tests. This
>     is problematic since any failures of the setup code will go
>     unnoticed. Therefore, move setup code into the tests themselves
>     so that failures are properly flagged.
>
Thanks, that really makes the change more clear for the readers. I learn a lot.

> Reviewers appreciate well-explained commit messages, but they also
> appreciate succinctness. Although it may not always be obvious how
> much to write in a commit message, you can assume that reviewers will
> understand obvious changes simply by reading the patch itself, thus
> you don't need to mention every little detail in the commit message.
> The important thing to mention in the commit message is the
> explanation of _why_ the change is being made, plus any changes which
> might not be obvious. In this case, all the changes are obvious, so,
> really, you can collapse this entire commit message to just the first
> paragraph.

Yeah, the commit looks very wordy. I'll make it more succinct.
>     test_write_lines a b c >file &&

> Same comment about simply using double quotes instead of
> single-quotes, however, this is also another really good place to use
> test_write_lines:
>
>     test_write_lines a b c >file &&

Thanks for the tips!

(Sorry for sending the V6 to you twice I send it by accident .)

-------------------------
Thanks
Shuqi