* [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script @ 2023-01-31 22:49 Shuqi Liang 2023-02-01 2:21 ` Andrei Rybak 2023-02-02 17:18 ` [PATCH v2 0/4] t4113: modernize test style Shuqi Liang 0 siblings, 2 replies; 49+ messages in thread From: Shuqi Liang @ 2023-01-31 22:49 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script 2023-01-31 22:49 [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script Shuqi Liang @ 2023-02-01 2:21 ` Andrei Rybak 2023-02-02 17:20 ` cheska fran 2023-02-02 17:18 ` [PATCH v2 0/4] t4113: modernize test style Shuqi Liang 1 sibling, 1 reply; 49+ messages in thread From: Andrei Rybak @ 2023-02-01 2:21 UTC (permalink / raw) To: Shuqi Liang, git 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script 2023-02-01 2:21 ` Andrei Rybak @ 2023-02-02 17:20 ` cheska fran 0 siblings, 0 replies; 49+ messages in thread From: cheska fran @ 2023-02-02 17:20 UTC (permalink / raw) To: Andrei Rybak, git 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 0/4] t4113: modernize test style 2023-01-31 22:49 [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script Shuqi Liang 2023-02-01 2:21 ` Andrei Rybak @ 2023-02-02 17:18 ` Shuqi Liang 2023-02-02 17:18 ` [PATCH v2 1/4]t4113: replace backslash with single quote Shuqi Liang ` (4 more replies) 1 sibling, 5 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-02 17:18 UTC (permalink / raw) To: git; +Cc: Shuqi Liang, rybak.a.v 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 1/4]t4113: replace backslash with single quote 2023-02-02 17:18 ` [PATCH v2 0/4] t4113: modernize test style Shuqi Liang @ 2023-02-02 17:18 ` Shuqi Liang 2023-02-02 21:00 ` Junio C Hamano 2023-02-02 17:18 ` [PATCH v2 2/4] t4113:put second creation in own step Shuqi Liang ` (3 subsequent siblings) 4 siblings, 1 reply; 49+ messages in thread From: Shuqi Liang @ 2023-02-02 17:18 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH v2 1/4]t4113: replace backslash with single quote 2023-02-02 17:18 ` [PATCH v2 1/4]t4113: replace backslash with single quote Shuqi Liang @ 2023-02-02 21:00 ` Junio C Hamano 2023-02-05 14:28 ` Shuqi Liang 0 siblings, 1 reply; 49+ messages in thread From: Junio C Hamano @ 2023-02-02 21:00 UTC (permalink / raw) To: Shuqi Liang; +Cc: git 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v2 1/4]t4113: replace backslash with single quote 2023-02-02 21:00 ` Junio C Hamano @ 2023-02-05 14:28 ` Shuqi Liang 0 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-05 14:28 UTC (permalink / raw) To: Junio C Hamano, git 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 2/4] t4113:put second creation in own step 2023-02-02 17:18 ` [PATCH v2 0/4] t4113: modernize test style Shuqi Liang 2023-02-02 17:18 ` [PATCH v2 1/4]t4113: replace backslash with single quote Shuqi Liang @ 2023-02-02 17:18 ` Shuqi Liang 2023-02-02 17:18 ` [PATCH v2 3/4] t4113: use "<<-" instead of "<<" Shuqi Liang ` (2 subsequent siblings) 4 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-02 17:18 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v2 3/4] t4113: use "<<-" instead of "<<" 2023-02-02 17:18 ` [PATCH v2 0/4] t4113: modernize test style Shuqi Liang 2023-02-02 17:18 ` [PATCH v2 1/4]t4113: replace backslash with single quote Shuqi Liang 2023-02-02 17:18 ` [PATCH v2 2/4] t4113:put second creation in own step Shuqi Liang @ 2023-02-02 17:18 ` Shuqi Liang 2023-02-02 21:05 ` Junio C Hamano 2023-02-02 17:18 ` [PATCH v2 4/4] t4113: indent with tabs Shuqi Liang 2023-02-05 14:52 ` [PATCH v3 0/3] modernize style Shuqi Liang 4 siblings, 1 reply; 49+ messages in thread From: Shuqi Liang @ 2023-02-02 17:18 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH v2 3/4] t4113: use "<<-" instead of "<<" 2023-02-02 17:18 ` [PATCH v2 3/4] t4113: use "<<-" instead of "<<" Shuqi Liang @ 2023-02-02 21:05 ` Junio C Hamano 2023-02-05 14:38 ` Shuqi Liang 0 siblings, 1 reply; 49+ messages in thread From: Junio C Hamano @ 2023-02-02 21:05 UTC (permalink / raw) To: Shuqi Liang; +Cc: git 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v2 3/4] t4113: use "<<-" instead of "<<" 2023-02-02 21:05 ` Junio C Hamano @ 2023-02-05 14:38 ` Shuqi Liang 0 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-05 14:38 UTC (permalink / raw) To: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 4/4] t4113: indent with tabs 2023-02-02 17:18 ` [PATCH v2 0/4] t4113: modernize test style Shuqi Liang ` (2 preceding siblings ...) 2023-02-02 17:18 ` [PATCH v2 3/4] t4113: use "<<-" instead of "<<" Shuqi Liang @ 2023-02-02 17:18 ` Shuqi Liang 2023-02-02 21:10 ` Junio C Hamano 2023-02-05 14:52 ` [PATCH v3 0/3] modernize style Shuqi Liang 4 siblings, 1 reply; 49+ messages in thread From: Shuqi Liang @ 2023-02-02 17:18 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH v2 4/4] t4113: indent with tabs 2023-02-02 17:18 ` [PATCH v2 4/4] t4113: indent with tabs Shuqi Liang @ 2023-02-02 21:10 ` Junio C Hamano 2023-02-05 14:51 ` Shuqi Liang 0 siblings, 1 reply; 49+ messages in thread From: Junio C Hamano @ 2023-02-02 21:10 UTC (permalink / raw) To: Shuqi Liang; +Cc: git 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH v2 4/4] t4113: indent with tabs 2023-02-02 21:10 ` Junio C Hamano @ 2023-02-05 14:51 ` Shuqi Liang 0 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-05 14:51 UTC (permalink / raw) To: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v3 0/3] modernize style 2023-02-02 17:18 ` [PATCH v2 0/4] t4113: modernize test style Shuqi Liang ` (3 preceding siblings ...) 2023-02-02 17:18 ` [PATCH v2 4/4] t4113: indent with tabs Shuqi Liang @ 2023-02-05 14:52 ` Shuqi Liang 2023-02-05 14:52 ` [PATCH v3 1/3]t4113: modernize a test script Shuqi Liang ` (3 more replies) 4 siblings, 4 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-05 14:52 UTC (permalink / raw) To: git; +Cc: Shuqi Liang, rybak.a.v, gitster 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v3 1/3]t4113: modernize a test script 2023-02-05 14:52 ` [PATCH v3 0/3] modernize style Shuqi Liang @ 2023-02-05 14:52 ` Shuqi Liang 2023-02-05 14:52 ` [PATCH v3 2/3] t4113: put executable lines to test_expect_success Shuqi Liang ` (2 subsequent siblings) 3 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-05 14:52 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v3 2/3] t4113: put executable lines to test_expect_success 2023-02-05 14:52 ` [PATCH v3 0/3] modernize style Shuqi Liang 2023-02-05 14:52 ` [PATCH v3 1/3]t4113: modernize a test script Shuqi Liang @ 2023-02-05 14:52 ` Shuqi Liang 2023-02-05 14:52 ` [PATCH v3 3/3] t4113: indent with space Shuqi Liang 2023-02-06 21:18 ` [PATCH v4 0/3] t4113: modernize style Shuqi Liang 3 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-05 14:52 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v3 3/3] t4113: indent with space 2023-02-05 14:52 ` [PATCH v3 0/3] modernize style Shuqi Liang 2023-02-05 14:52 ` [PATCH v3 1/3]t4113: modernize a test script Shuqi Liang 2023-02-05 14:52 ` [PATCH v3 2/3] t4113: put executable lines to test_expect_success Shuqi Liang @ 2023-02-05 14:52 ` Shuqi Liang 2023-02-05 20:29 ` Eric Sunshine 2023-02-06 21:18 ` [PATCH v4 0/3] t4113: modernize style Shuqi Liang 3 siblings, 1 reply; 49+ messages in thread From: Shuqi Liang @ 2023-02-05 14:52 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH v3 3/3] t4113: indent with space 2023-02-05 14:52 ` [PATCH v3 3/3] t4113: indent with space Shuqi Liang @ 2023-02-05 20:29 ` Eric Sunshine 2023-02-06 21:17 ` Shuqi Liang 0 siblings, 1 reply; 49+ messages in thread From: Eric Sunshine @ 2023-02-05 20:29 UTC (permalink / raw) To: Shuqi Liang; +Cc: git 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3 3/3] t4113: indent with space 2023-02-05 20:29 ` Eric Sunshine @ 2023-02-06 21:17 ` Shuqi Liang 0 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-06 21:17 UTC (permalink / raw) To: Eric Sunshine, git 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v4 0/3] t4113: modernize style 2023-02-05 14:52 ` [PATCH v3 0/3] modernize style Shuqi Liang ` (2 preceding siblings ...) 2023-02-05 14:52 ` [PATCH v3 3/3] t4113: indent with space Shuqi Liang @ 2023-02-06 21:18 ` Shuqi Liang 2023-02-06 21:18 ` [PATCH v4 1/3] t4113: modernize test script Shuqi Liang ` (3 more replies) 3 siblings, 4 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-06 21:18 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v4 1/3] t4113: modernize test script 2023-02-06 21:18 ` [PATCH v4 0/3] t4113: modernize style Shuqi Liang @ 2023-02-06 21:18 ` Shuqi Liang 2023-02-06 21:18 ` [PATCH v4 2/3] t4113: indent with tab Shuqi Liang ` (2 subsequent siblings) 3 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-06 21:18 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v4 2/3] t4113: indent with tab 2023-02-06 21:18 ` [PATCH v4 0/3] t4113: modernize style Shuqi Liang 2023-02-06 21:18 ` [PATCH v4 1/3] t4113: modernize test script Shuqi Liang @ 2023-02-06 21:18 ` Shuqi Liang 2023-02-06 21:18 ` [PATCH v4 3/3] t4113: put executable lines to test_expect_success Shuqi Liang 2023-02-09 15:44 ` [PATCH v5 0/3] t4113: modernize style Shuqi Liang 3 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-06 21:18 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v4 3/3] t4113: put executable lines to test_expect_success 2023-02-06 21:18 ` [PATCH v4 0/3] t4113: modernize style Shuqi Liang 2023-02-06 21:18 ` [PATCH v4 1/3] t4113: modernize test script Shuqi Liang 2023-02-06 21:18 ` [PATCH v4 2/3] t4113: indent with tab Shuqi Liang @ 2023-02-06 21:18 ` Shuqi Liang 2023-02-06 21:50 ` Ævar Arnfjörð Bjarmason 2023-02-06 22:44 ` Junio C Hamano 2023-02-09 15:44 ` [PATCH v5 0/3] t4113: modernize style Shuqi Liang 3 siblings, 2 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-06 21:18 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success 2023-02-06 21:18 ` [PATCH v4 3/3] t4113: put executable lines to test_expect_success Shuqi Liang @ 2023-02-06 21:50 ` Ævar Arnfjörð Bjarmason 2023-02-06 22:44 ` Junio C Hamano 1 sibling, 0 replies; 49+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2023-02-06 21:50 UTC (permalink / raw) To: Shuqi Liang; +Cc: git 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success 2023-02-06 21:18 ` [PATCH v4 3/3] t4113: put executable lines to test_expect_success Shuqi Liang 2023-02-06 21:50 ` Ævar Arnfjörð Bjarmason @ 2023-02-06 22:44 ` Junio C Hamano 2023-02-06 23:42 ` Shuqi Liang 2023-02-14 22:17 ` Shuqi Liang 1 sibling, 2 replies; 49+ messages in thread From: Junio C Hamano @ 2023-02-06 22:44 UTC (permalink / raw) To: Shuqi Liang; +Cc: git 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). ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success 2023-02-06 22:44 ` Junio C Hamano @ 2023-02-06 23:42 ` Shuqi Liang 2023-02-07 8:05 ` Ævar Arnfjörð Bjarmason 2023-02-14 22:17 ` Shuqi Liang 1 sibling, 1 reply; 49+ messages in thread From: Shuqi Liang @ 2023-02-06 23:42 UTC (permalink / raw) To: Junio C Hamano, git 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success 2023-02-06 23:42 ` Shuqi Liang @ 2023-02-07 8:05 ` Ævar Arnfjörð Bjarmason 2023-02-07 19:55 ` Shuqi Liang ` (2 more replies) 0 siblings, 3 replies; 49+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2023-02-07 8:05 UTC (permalink / raw) To: Shuqi Liang; +Cc: Junio C Hamano, git 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' ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success 2023-02-07 8:05 ` Ævar Arnfjörð Bjarmason @ 2023-02-07 19:55 ` Shuqi Liang 2023-02-08 5:44 ` Shuqi Liang 2023-02-15 0:26 ` Eric Sunshine 2 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-07 19:55 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, git 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success 2023-02-07 8:05 ` Ævar Arnfjörð Bjarmason 2023-02-07 19:55 ` Shuqi Liang @ 2023-02-08 5:44 ` Shuqi Liang 2023-02-08 7:44 ` Ævar Arnfjörð Bjarmason 2023-02-15 0:26 ` Eric Sunshine 2 siblings, 1 reply; 49+ messages in thread From: Shuqi Liang @ 2023-02-08 5:44 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, git, Junio C Hamano, Andrei Rybak 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success 2023-02-08 5:44 ` Shuqi Liang @ 2023-02-08 7:44 ` Ævar Arnfjörð Bjarmason 2023-02-10 15:29 ` Shuqi Liang 2023-02-15 0:34 ` Eric Sunshine 0 siblings, 2 replies; 49+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2023-02-08 7:44 UTC (permalink / raw) To: Shuqi Liang; +Cc: git, Junio C Hamano, Andrei Rybak 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success 2023-02-08 7:44 ` Ævar Arnfjörð Bjarmason @ 2023-02-10 15:29 ` Shuqi Liang 2023-02-15 0:34 ` Eric Sunshine 1 sibling, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-10 15:29 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, git 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success 2023-02-08 7:44 ` Ævar Arnfjörð Bjarmason 2023-02-10 15:29 ` Shuqi Liang @ 2023-02-15 0:34 ` Eric Sunshine 1 sibling, 0 replies; 49+ messages in thread From: Eric Sunshine @ 2023-02-15 0:34 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Shuqi Liang, git, Junio C Hamano, Andrei Rybak 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 && ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success 2023-02-07 8:05 ` Ævar Arnfjörð Bjarmason 2023-02-07 19:55 ` Shuqi Liang 2023-02-08 5:44 ` Shuqi Liang @ 2023-02-15 0:26 ` Eric Sunshine 2 siblings, 0 replies; 49+ messages in thread From: Eric Sunshine @ 2023-02-15 0:26 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Shuqi Liang, Junio C Hamano, git 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success 2023-02-06 22:44 ` Junio C Hamano 2023-02-06 23:42 ` Shuqi Liang @ 2023-02-14 22:17 ` Shuqi Liang 2023-02-14 22:29 ` Junio C Hamano 1 sibling, 1 reply; 49+ messages in thread From: Shuqi Liang @ 2023-02-14 22:17 UTC (permalink / raw) To: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v4 3/3] t4113: put executable lines to test_expect_success 2023-02-14 22:17 ` Shuqi Liang @ 2023-02-14 22:29 ` Junio C Hamano 0 siblings, 0 replies; 49+ messages in thread From: Junio C Hamano @ 2023-02-14 22:29 UTC (permalink / raw) To: Shuqi Liang; +Cc: git 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 0/3] t4113: modernize style 2023-02-06 21:18 ` [PATCH v4 0/3] t4113: modernize style Shuqi Liang ` (2 preceding siblings ...) 2023-02-06 21:18 ` [PATCH v4 3/3] t4113: put executable lines to test_expect_success Shuqi Liang @ 2023-02-09 15:44 ` Shuqi Liang 2023-02-09 15:44 ` [PATCH v5 1/3] t4113: modernize test script Shuqi Liang ` (4 more replies) 3 siblings, 5 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-09 15:44 UTC (permalink / raw) To: git; +Cc: Shuqi Liang, gitster, rybak.a.v, avarab, sunshine 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 1/3] t4113: modernize test script 2023-02-09 15:44 ` [PATCH v5 0/3] t4113: modernize style Shuqi Liang @ 2023-02-09 15:44 ` Shuqi Liang 2023-02-09 15:44 ` [PATCH v5 2/3] t4113: indent with tab Shuqi Liang ` (3 subsequent siblings) 4 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-09 15:44 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v5 2/3] t4113: indent with tab 2023-02-09 15:44 ` [PATCH v5 0/3] t4113: modernize style Shuqi Liang 2023-02-09 15:44 ` [PATCH v5 1/3] t4113: modernize test script Shuqi Liang @ 2023-02-09 15:44 ` Shuqi Liang 2023-02-15 0:10 ` Eric Sunshine 2023-02-09 15:44 ` [PATCH v5 3/3] t4113: put executable lines to test_expect_success Shuqi Liang ` (2 subsequent siblings) 4 siblings, 1 reply; 49+ messages in thread From: Shuqi Liang @ 2023-02-09 15:44 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH v5 2/3] t4113: indent with tab 2023-02-09 15:44 ` [PATCH v5 2/3] t4113: indent with tab Shuqi Liang @ 2023-02-15 0:10 ` Eric Sunshine 2023-02-15 0:18 ` Shuqi Liang 0 siblings, 1 reply; 49+ messages in thread From: Eric Sunshine @ 2023-02-15 0:10 UTC (permalink / raw) To: Shuqi Liang; +Cc: git 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v5 2/3] t4113: indent with tab 2023-02-15 0:10 ` Eric Sunshine @ 2023-02-15 0:18 ` Shuqi Liang 0 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-15 0:18 UTC (permalink / raw) To: Eric Sunshine, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 3/3] t4113: put executable lines to test_expect_success 2023-02-09 15:44 ` [PATCH v5 0/3] t4113: modernize style Shuqi Liang 2023-02-09 15:44 ` [PATCH v5 1/3] t4113: modernize test script Shuqi Liang 2023-02-09 15:44 ` [PATCH v5 2/3] t4113: indent with tab Shuqi Liang @ 2023-02-09 15:44 ` Shuqi Liang 2023-02-15 1:09 ` Eric Sunshine 2023-02-15 2:38 ` [PATCH v6 0/3] t4113: modernize style Shuqi Liang 2023-02-15 2:39 ` Shuqi Liang 4 siblings, 1 reply; 49+ messages in thread From: Shuqi Liang @ 2023-02-09 15:44 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH v5 3/3] t4113: put executable lines to test_expect_success 2023-02-09 15:44 ` [PATCH v5 3/3] t4113: put executable lines to test_expect_success Shuqi Liang @ 2023-02-15 1:09 ` Eric Sunshine 2023-02-15 2:40 ` Shuqi Liang 0 siblings, 1 reply; 49+ messages in thread From: Eric Sunshine @ 2023-02-15 1:09 UTC (permalink / raw) To: Shuqi Liang; +Cc: git 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 && ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v5 3/3] t4113: put executable lines to test_expect_success 2023-02-15 1:09 ` Eric Sunshine @ 2023-02-15 2:40 ` Shuqi Liang 0 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-15 2:40 UTC (permalink / raw) To: Eric Sunshine, git 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v6 0/3] t4113: modernize style 2023-02-09 15:44 ` [PATCH v5 0/3] t4113: modernize style Shuqi Liang ` (2 preceding siblings ...) 2023-02-09 15:44 ` [PATCH v5 3/3] t4113: put executable lines to test_expect_success Shuqi Liang @ 2023-02-15 2:38 ` Shuqi Liang 2023-02-15 2:39 ` Shuqi Liang 4 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-15 2:38 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v6 0/3] t4113: modernize style 2023-02-09 15:44 ` [PATCH v5 0/3] t4113: modernize style Shuqi Liang ` (3 preceding siblings ...) 2023-02-15 2:38 ` [PATCH v6 0/3] t4113: modernize style Shuqi Liang @ 2023-02-15 2:39 ` Shuqi Liang 2023-02-15 2:39 ` [PATCH v6 1/3] t4113: modernize test script Shuqi Liang ` (2 more replies) 4 siblings, 3 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-15 2:39 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v6 1/3] t4113: modernize test script 2023-02-15 2:39 ` Shuqi Liang @ 2023-02-15 2:39 ` Shuqi Liang 2023-02-15 2:39 ` [PATCH v6 2/3] t4113: indent with tab Shuqi Liang 2023-02-15 2:39 ` [PATCH v6 3/3] t4113: put executable lines to test_expect_success Shuqi Liang 2 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-15 2:39 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v6 2/3] t4113: indent with tab 2023-02-15 2:39 ` Shuqi Liang 2023-02-15 2:39 ` [PATCH v6 1/3] t4113: modernize test script Shuqi Liang @ 2023-02-15 2:39 ` Shuqi Liang 2023-02-15 2:39 ` [PATCH v6 3/3] t4113: put executable lines to test_expect_success Shuqi Liang 2 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-15 2:39 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v6 3/3] t4113: put executable lines to test_expect_success 2023-02-15 2:39 ` Shuqi Liang 2023-02-15 2:39 ` [PATCH v6 1/3] t4113: modernize test script Shuqi Liang 2023-02-15 2:39 ` [PATCH v6 2/3] t4113: indent with tab Shuqi Liang @ 2023-02-15 2:39 ` Shuqi Liang 2 siblings, 0 replies; 49+ messages in thread From: Shuqi Liang @ 2023-02-15 2:39 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
end of thread, other threads:[~2023-02-15 2:40 UTC | newest] Thread overview: 49+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-31 22:49 [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script Shuqi Liang 2023-02-01 2:21 ` Andrei Rybak 2023-02-02 17:20 ` cheska fran 2023-02-02 17:18 ` [PATCH v2 0/4] t4113: modernize test style Shuqi Liang 2023-02-02 17:18 ` [PATCH v2 1/4]t4113: replace backslash with single quote Shuqi Liang 2023-02-02 21:00 ` Junio C Hamano 2023-02-05 14:28 ` Shuqi Liang 2023-02-02 17:18 ` [PATCH v2 2/4] t4113:put second creation in own step Shuqi Liang 2023-02-02 17:18 ` [PATCH v2 3/4] t4113: use "<<-" instead of "<<" Shuqi Liang 2023-02-02 21:05 ` Junio C Hamano 2023-02-05 14:38 ` Shuqi Liang 2023-02-02 17:18 ` [PATCH v2 4/4] t4113: indent with tabs Shuqi Liang 2023-02-02 21:10 ` Junio C Hamano 2023-02-05 14:51 ` Shuqi Liang 2023-02-05 14:52 ` [PATCH v3 0/3] modernize style Shuqi Liang 2023-02-05 14:52 ` [PATCH v3 1/3]t4113: modernize a test script Shuqi Liang 2023-02-05 14:52 ` [PATCH v3 2/3] t4113: put executable lines to test_expect_success Shuqi Liang 2023-02-05 14:52 ` [PATCH v3 3/3] t4113: indent with space Shuqi Liang 2023-02-05 20:29 ` Eric Sunshine 2023-02-06 21:17 ` Shuqi Liang 2023-02-06 21:18 ` [PATCH v4 0/3] t4113: modernize style Shuqi Liang 2023-02-06 21:18 ` [PATCH v4 1/3] t4113: modernize test script Shuqi Liang 2023-02-06 21:18 ` [PATCH v4 2/3] t4113: indent with tab Shuqi Liang 2023-02-06 21:18 ` [PATCH v4 3/3] t4113: put executable lines to test_expect_success Shuqi Liang 2023-02-06 21:50 ` Ævar Arnfjörð Bjarmason 2023-02-06 22:44 ` Junio C Hamano 2023-02-06 23:42 ` Shuqi Liang 2023-02-07 8:05 ` Ævar Arnfjörð Bjarmason 2023-02-07 19:55 ` Shuqi Liang 2023-02-08 5:44 ` Shuqi Liang 2023-02-08 7:44 ` Ævar Arnfjörð Bjarmason 2023-02-10 15:29 ` Shuqi Liang 2023-02-15 0:34 ` Eric Sunshine 2023-02-15 0:26 ` Eric Sunshine 2023-02-14 22:17 ` Shuqi Liang 2023-02-14 22:29 ` Junio C Hamano 2023-02-09 15:44 ` [PATCH v5 0/3] t4113: modernize style Shuqi Liang 2023-02-09 15:44 ` [PATCH v5 1/3] t4113: modernize test script Shuqi Liang 2023-02-09 15:44 ` [PATCH v5 2/3] t4113: indent with tab Shuqi Liang 2023-02-15 0:10 ` Eric Sunshine 2023-02-15 0:18 ` Shuqi Liang 2023-02-09 15:44 ` [PATCH v5 3/3] t4113: put executable lines to test_expect_success Shuqi Liang 2023-02-15 1:09 ` Eric Sunshine 2023-02-15 2:40 ` Shuqi Liang 2023-02-15 2:38 ` [PATCH v6 0/3] t4113: modernize style Shuqi Liang 2023-02-15 2:39 ` Shuqi Liang 2023-02-15 2:39 ` [PATCH v6 1/3] t4113: modernize test script Shuqi Liang 2023-02-15 2:39 ` [PATCH v6 2/3] t4113: indent with tab Shuqi Liang 2023-02-15 2:39 ` [PATCH v6 3/3] t4113: put executable lines to test_expect_success Shuqi Liang
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).