# [GSOC][PATCH] t4121: modernize test style 13 messages from 2023-02-20 to 2023-02-22. Participants: Vivan Garg, Victoria Dye, Junio C Hamano. Thread: https://gitlist.dev/t/59275 ## Vivan Garg, 2023-02-20 23:51 Subject: [GSOC][PATCH] t4121: modernize test style Message-ID: <20230220235121.34375-1-gvivan6@gmail.com> URL: https://gitlist.dev/e/20230220235121.34375-1-gvivan6%40gmail.com ``` Test scripts in file t4121-apply-diffs.sh are written in old style, where the test_expect_success command and test title are written on separate lines Signed-off-by: Vivan Garg --- Greetings, my name is Vivan Garg. I am currently pursuing a double major in computer science and finance at the University of Waterloo in Canada. I am currently completing my third software developer internship at Morgan Stanley. As part of my coursework, I studied C and shell scripting, which I then applied in internships and personal projects. C++ is the programming language that I am most comfortable with right now. Please feel free to address me as Vivan, and my pronouns are he/him/his. I meet the requirements for GSOC participation. So far, I've either read or skimmed the following documents based on prior knowledge: Submitting patches, Coding guidelines, Myfirstcontribution.txt, gittutorial, Giteveryday, readme, Hacking-Git, General-Microproject-Information, SoC-2023-Ideas, and General-Application-Information. I'm looking forward to having a fantastic time here! t/t4121-apply-diffs.sh | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/t/t4121-apply-diffs.sh b/t/t4121-apply-diffs.sh index a80cec9d11..2ff38ededa 100755 --- a/t/t4121-apply-diffs.sh +++ b/t/t4121-apply-diffs.sh @@ -16,8 +16,8 @@ echo '1 7 8' >file -test_expect_success 'setup' \ - 'git add file && +test_expect_success 'setup' ' + git add file && git commit -q -m 1 && git checkout -b test && mv file file.tmp && @@ -27,10 +27,11 @@ test_expect_success 'setup' \ git commit -a -q -m 2 && echo 9 >>file && git commit -a -q -m 3 && - git checkout main' + git checkout main +' -test_expect_success \ - 'check if contextually independent diffs for the same file apply' \ - '( git diff test~2 test~1 && git diff test~1 test~0 )| git apply' +test_expect_success 'check if contextually independent diffs for the same file apply' ' + ( git diff test~2 test~1 && git diff test~1 test~0 )| git apply +' test_done -- 2.37.0 (Apple Git-136) ``` ## Victoria Dye, 2023-02-21 17:22 Subject: Re: [GSOC][PATCH] t4121: modernize test style Message-ID: <38cb184d-e47a-2129-a93e-16ffd2afe67a@github.com> URL: https://gitlist.dev/e/38cb184d-e47a-2129-a93e-16ffd2afe67a%40github.com In-Reply-To: <20230220235121.34375-1-gvivan6@gmail.com> ``` Vivan Garg wrote: > Test scripts in file t4121-apply-diffs.sh are written in old style, > where the test_expect_success command and test title are written on > separate lines nit: period at the end of the sentence (s/lines/lines.) Also, this commit message explains *why* you're making the change, but not what the commit actually does (that is, update the tests to adhere to the new style). Would you mind adding a note about that to the message? > > Signed-off-by: Vivan Garg > --- > Greetings, my name is Vivan Garg. I am currently pursuing a double major > in computer science and finance at the University of Waterloo in Canada. > I am currently completing my third software developer internship at Morgan > Stanley. As part of my coursework, I studied C and shell scripting, which > I then applied in internships and personal projects. C++ is the programming > language that I am most comfortable with right now. Please feel free to > address me as Vivan, and my pronouns are he/him/his. I meet the requirements > for GSOC participation. So far, I've either read or skimmed the following > documents based on prior knowledge: Submitting patches, Coding guidelines, > Myfirstcontribution.txt, gittutorial, Giteveryday, readme, Hacking-Git, > General-Microproject-Information, SoC-2023-Ideas, and > General-Application-Information. I'm looking forward to having a fantastic > time here! Welcome to the Git community, and thanks for your contribution! :) > > t/t4121-apply-diffs.sh | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/t/t4121-apply-diffs.sh b/t/t4121-apply-diffs.sh > index a80cec9d11..2ff38ededa 100755 > --- a/t/t4121-apply-diffs.sh > +++ b/t/t4121-apply-diffs.sh > @@ -16,8 +16,8 @@ echo '1 > 7 > 8' >file > > -test_expect_success 'setup' \ > - 'git add file && > +test_expect_success 'setup' ' > + git add file && > git commit -q -m 1 && > git checkout -b test && > mv file file.tmp && > @@ -27,10 +27,11 @@ test_expect_success 'setup' \ > git commit -a -q -m 2 && > echo 9 >>file && > git commit -a -q -m 3 && > - git checkout main' > + git checkout main > +' This test looks good. > > -test_expect_success \ > - 'check if contextually independent diffs for the same file apply' \ > - '( git diff test~2 test~1 && git diff test~1 test~0 )| git apply' > +test_expect_success 'check if contextually independent diffs for the same file apply' ' > + ( git diff test~2 test~1 && git diff test~1 test~0 )| git apply > +' As for this one, the test is correctly updated to the new style (per the microproject prompt). However, the spacing around the '|' is a little weird - I think there should be a space after ')'. On your next re-roll, could you fix that spacing (in this patch is fine - it's not a substantial enough change to warrant its own commit)? > > test_done ``` ## Vivan Garg, 2023-02-21 19:48 Subject: Re: [GSOC][PATCH] t4121: modernize test style Message-ID: URL: https://gitlist.dev/e/CADupsJNPVm13iSv84SXpqtP8Dhm_-htPTr59yPs_J%3DaQzs2FwQ%40mail.gmail.com In-Reply-To: <38cb184d-e47a-2129-a93e-16ffd2afe67a@github.com> ``` On Tue, Feb 21, 2023 at 10:27 AM Victoria Dye wrote: > > nit: period at the end of the sentence (s/lines/lines.) > > Also, this commit message explains *why* you're making the change, but not > what the commit actually does (that is, update the tests to adhere to the > new style). Would you mind adding a note about that to the message? For sure! Is it correct that I will need to amend the commit message and send out a v2 of the patch? > > > > -test_expect_success \ > > - 'check if contextually independent diffs for the same file apply' \ > > - '( git diff test~2 test~1 && git diff test~1 test~0 )| git apply' > > +test_expect_success 'check if contextually independent diffs for the same file apply' ' > > + ( git diff test~2 test~1 && git diff test~1 test~0 )| git apply > > +' > > As for this one, the test is correctly updated to the new style (per the > microproject prompt). However, the spacing around the '|' is a little weird > - I think there should be a space after ')'. On your next re-roll, could you > fix that spacing (in this patch is fine - it's not a substantial enough > change to warrant its own commit)? I'm not sure what you mean by "next re-roll," are you referring to v2? But then you said it was fine in this patch, so I'm confused. If I am going to be sending a v2, couldn't I just revert the last commit and add this change to the same commit? ``` ## Junio C Hamano, 2023-02-21 20:21 Subject: Re: [GSOC][PATCH] t4121: modernize test style Message-ID: URL: https://gitlist.dev/e/xmqq8rgqeplt.fsf%40gitster.g In-Reply-To: ``` Vivan Garg writes: > I'm not sure what you mean by "next re-roll," are you referring to v2? But > then you said it was fine in this patch, so I'm confused. If I am going to be > sending a v2, couldn't I just revert the last commit and add this change to the > same commit? I am not Victoria, but I think * You had two (or more) changes in your patch. * She found one of them is good. * The other one(s) were found lacking. When a patch is reviewed and found to be lacking (as a whole), and if you agree with the issues pointed out by the review(s), you'd redo the patch (with either "commit --amend" for a single patch, or "rebase -i" for a series with multiple patches), and produce v2, as if v1 never happened. The act of doing so is colloquially called "to reroll". Documentation/MyFirstContribution.txt::[[now-what]] may have more details. Thanks. ``` ## Vivan Garg, 2023-02-21 21:27 Subject: [GSOC][PATCH v2 0/1] t4121: modernize test style Message-ID: <20230221212753.85371-1-gvivan6@gmail.com> URL: https://gitlist.dev/e/20230221212753.85371-1-gvivan6%40gmail.com In-Reply-To: ``` Edited the commit message to add what the commit does and added a period at the end of the sentence. Added a space after the closing bracket in the second test. Vivan Garg (1): t4121: modernize test style t/t4121-apply-diffs.sh | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) Range-diff against v1: 1: 61f1ed51f4 ! 1: 77fded7759 t4121: modernize test style @@ Commit message Test scripts in file t4121-apply-diffs.sh are written in old style, where the test_expect_success command and test title are written on - separate lines + separate lines, therefore update the tests to adhere to the new + style. Signed-off-by: Vivan Garg @@ t/t4121-apply-diffs.sh: test_expect_success 'setup' \ - 'check if contextually independent diffs for the same file apply' \ - '( git diff test~2 test~1 && git diff test~1 test~0 )| git apply' +test_expect_success 'check if contextually independent diffs for the same file apply' ' -+ ( git diff test~2 test~1 && git diff test~1 test~0 )| git apply ++ ( git diff test~2 test~1 && git diff test~1 test~0 ) | git apply +' test_done -- 2.37.0 (Apple Git-136) ``` ## Vivan Garg, 2023-02-21 21:27 Subject: [GSOC][PATCH v2 1/1] t4121: modernize test style Message-ID: <20230221212753.85371-2-gvivan6@gmail.com> URL: https://gitlist.dev/e/20230221212753.85371-2-gvivan6%40gmail.com In-Reply-To: <20230221212753.85371-1-gvivan6@gmail.com> ``` Test scripts in file t4121-apply-diffs.sh are written in old style, where the test_expect_success command and test title are written on separate lines, therefore update the tests to adhere to the new style. Signed-off-by: Vivan Garg --- t/t4121-apply-diffs.sh | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/t/t4121-apply-diffs.sh b/t/t4121-apply-diffs.sh index a80cec9d11..f1cc42ff71 100755 --- a/t/t4121-apply-diffs.sh +++ b/t/t4121-apply-diffs.sh @@ -16,8 +16,8 @@ echo '1 7 8' >file -test_expect_success 'setup' \ - 'git add file && +test_expect_success 'setup' ' + git add file && git commit -q -m 1 && git checkout -b test && mv file file.tmp && @@ -27,10 +27,11 @@ test_expect_success 'setup' \ git commit -a -q -m 2 && echo 9 >>file && git commit -a -q -m 3 && - git checkout main' + git checkout main +' -test_expect_success \ - 'check if contextually independent diffs for the same file apply' \ - '( git diff test~2 test~1 && git diff test~1 test~0 )| git apply' +test_expect_success 'check if contextually independent diffs for the same file apply' ' + ( git diff test~2 test~1 && git diff test~1 test~0 ) | git apply +' test_done -- 2.37.0 (Apple Git-136) ``` ## Vivan Garg, 2023-02-21 21:35 Subject: [GSOC][PATCH v2 0/1] t4121: modernize test style Message-ID: <20230221213526.85591-1-gvivan6@gmail.com> URL: https://gitlist.dev/e/20230221213526.85591-1-gvivan6%40gmail.com In-Reply-To: <20230220235121.34375-1-gvivan6@gmail.com> ``` Edited the commit message to add what the commit does and added a period at the end of the sentence. Added a space after the closing bracket in the second test. Vivan Garg (1): t4121: modernize test style t/t4121-apply-diffs.sh | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) Range-diff against v1: 1: 61f1ed51f4 ! 1: 77fded7759 t4121: modernize test style @@ Commit message Test scripts in file t4121-apply-diffs.sh are written in old style, where the test_expect_success command and test title are written on - separate lines + separate lines, therefore update the tests to adhere to the new + style. Signed-off-by: Vivan Garg @@ t/t4121-apply-diffs.sh: test_expect_success 'setup' \ - 'check if contextually independent diffs for the same file apply' \ - '( git diff test~2 test~1 && git diff test~1 test~0 )| git apply' +test_expect_success 'check if contextually independent diffs for the same file apply' ' -+ ( git diff test~2 test~1 && git diff test~1 test~0 )| git apply ++ ( git diff test~2 test~1 && git diff test~1 test~0 ) | git apply +' test_done -- 2.37.0 (Apple Git-136) ``` ## Vivan Garg, 2023-02-21 21:35 Subject: [GSOC][PATCH v2 1/1] t4121: modernize test style Message-ID: <20230221213526.85591-2-gvivan6@gmail.com> URL: https://gitlist.dev/e/20230221213526.85591-2-gvivan6%40gmail.com In-Reply-To: <20230221213526.85591-1-gvivan6@gmail.com> ``` Test scripts in file t4121-apply-diffs.sh are written in old style, where the test_expect_success command and test title are written on separate lines, therefore update the tests to adhere to the new style. Signed-off-by: Vivan Garg --- t/t4121-apply-diffs.sh | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/t/t4121-apply-diffs.sh b/t/t4121-apply-diffs.sh index a80cec9d11..f1cc42ff71 100755 --- a/t/t4121-apply-diffs.sh +++ b/t/t4121-apply-diffs.sh @@ -16,8 +16,8 @@ echo '1 7 8' >file -test_expect_success 'setup' \ - 'git add file && +test_expect_success 'setup' ' + git add file && git commit -q -m 1 && git checkout -b test && mv file file.tmp && @@ -27,10 +27,11 @@ test_expect_success 'setup' \ git commit -a -q -m 2 && echo 9 >>file && git commit -a -q -m 3 && - git checkout main' + git checkout main +' -test_expect_success \ - 'check if contextually independent diffs for the same file apply' \ - '( git diff test~2 test~1 && git diff test~1 test~0 )| git apply' +test_expect_success 'check if contextually independent diffs for the same file apply' ' + ( git diff test~2 test~1 && git diff test~1 test~0 ) | git apply +' test_done -- 2.37.0 (Apple Git-136) ``` ## Vivan Garg, 2023-02-21 21:46 Subject: [GSOC][PATCH v2 0/1] t4121: modernize test style Message-ID: <20230221214653.85830-1-gvivan6@gmail.com> URL: https://gitlist.dev/e/20230221214653.85830-1-gvivan6%40gmail.com In-Reply-To: <20230220235121.34375-1-gvivan6@gmail.com> ``` Edited the commit message to add what the commit does and added a period at the end of the sentence. Added a space after the closing bracket in the second test. Vivan Garg (1): t4121: modernize test style t/t4121-apply-diffs.sh | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) Range-diff against v1: 1: 61f1ed51f4 ! 1: 77fded7759 t4121: modernize test style @@ Commit message Test scripts in file t4121-apply-diffs.sh are written in old style, where the test_expect_success command and test title are written on - separate lines + separate lines, therefore update the tests to adhere to the new + style. Signed-off-by: Vivan Garg @@ t/t4121-apply-diffs.sh: test_expect_success 'setup' \ - 'check if contextually independent diffs for the same file apply' \ - '( git diff test~2 test~1 && git diff test~1 test~0 )| git apply' +test_expect_success 'check if contextually independent diffs for the same file apply' ' -+ ( git diff test~2 test~1 && git diff test~1 test~0 )| git apply ++ ( git diff test~2 test~1 && git diff test~1 test~0 ) | git apply +' test_done -- 2.37.0 (Apple Git-136) ``` ## Vivan Garg, 2023-02-21 21:46 Subject: [GSOC][PATCH v2 1/1] t4121: modernize test style Message-ID: <20230221214653.85830-2-gvivan6@gmail.com> URL: https://gitlist.dev/e/20230221214653.85830-2-gvivan6%40gmail.com In-Reply-To: <20230221214653.85830-1-gvivan6@gmail.com> ``` Test scripts in file t4121-apply-diffs.sh are written in old style, where the test_expect_success command and test title are written on separate lines, therefore update the tests to adhere to the new style. Signed-off-by: Vivan Garg --- t/t4121-apply-diffs.sh | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/t/t4121-apply-diffs.sh b/t/t4121-apply-diffs.sh index a80cec9d11..f1cc42ff71 100755 --- a/t/t4121-apply-diffs.sh +++ b/t/t4121-apply-diffs.sh @@ -16,8 +16,8 @@ echo '1 7 8' >file -test_expect_success 'setup' \ - 'git add file && +test_expect_success 'setup' ' + git add file && git commit -q -m 1 && git checkout -b test && mv file file.tmp && @@ -27,10 +27,11 @@ test_expect_success 'setup' \ git commit -a -q -m 2 && echo 9 >>file && git commit -a -q -m 3 && - git checkout main' + git checkout main +' -test_expect_success \ - 'check if contextually independent diffs for the same file apply' \ - '( git diff test~2 test~1 && git diff test~1 test~0 )| git apply' +test_expect_success 'check if contextually independent diffs for the same file apply' ' + ( git diff test~2 test~1 && git diff test~1 test~0 ) | git apply +' test_done -- 2.37.0 (Apple Git-136) ``` ## Victoria Dye, 2023-02-21 22:05 Subject: Re: [GSOC][PATCH v2 1/1] t4121: modernize test style Message-ID: <77734da5-e711-f653-b022-ba3b26823701@github.com> URL: https://gitlist.dev/e/77734da5-e711-f653-b022-ba3b26823701%40github.com In-Reply-To: <20230221214653.85830-2-gvivan6@gmail.com> ``` Vivan Garg wrote: > Test scripts in file t4121-apply-diffs.sh are written in old style, > where the test_expect_success command and test title are written on > separate lines, therefore update the tests to adhere to the new > style. The new commit message is sufficiently descriptive, thanks for updating. In terms of readability, it is a bit of a run-on sentence (the comma after "lines" could be a period, i.e. "...separate lines. Therefore, update the..."). I don't think it needs to be updated, but it's something to keep in mind for future contributions. :) > > Signed-off-by: Vivan Garg > --- > t/t4121-apply-diffs.sh | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/t/t4121-apply-diffs.sh b/t/t4121-apply-diffs.sh > index a80cec9d11..f1cc42ff71 100755 > --- a/t/t4121-apply-diffs.sh > +++ b/t/t4121-apply-diffs.sh > @@ -16,8 +16,8 @@ echo '1 > 7 > 8' >file > > -test_expect_success 'setup' \ > - 'git add file && > +test_expect_success 'setup' ' > + git add file && > git commit -q -m 1 && > git checkout -b test && > mv file file.tmp && > @@ -27,10 +27,11 @@ test_expect_success 'setup' \ > git commit -a -q -m 2 && > echo 9 >>file && > git commit -a -q -m 3 && > - git checkout main' > + git checkout main > +' > > -test_expect_success \ > - 'check if contextually independent diffs for the same file apply' \ > - '( git diff test~2 test~1 && git diff test~1 test~0 )| git apply' > +test_expect_success 'check if contextually independent diffs for the same file apply' ' > + ( git diff test~2 test~1 && git diff test~1 test~0 ) | git apply > +' Whitespace looks good here. I think this is ready-to-merge; thanks! > > test_done ``` ## Vivan Garg, 2023-02-21 22:34 Subject: Re: [GSOC][PATCH v2 1/1] t4121: modernize test style Message-ID: URL: https://gitlist.dev/e/CADupsJMGgOzrd7L%2BEt%3D5t0a9vXNpZqhT4qzFi_7YbHwMMR6KWg%40mail.gmail.com In-Reply-To: <77734da5-e711-f653-b022-ba3b26823701@github.com> ``` On Tue, Feb 21, 2023 at 3:17 PM Victoria Dye wrote: > The new commit message is sufficiently descriptive, thanks for updating. In > terms of readability, it is a bit of a run-on sentence (the comma after > "lines" could be a period, i.e. "...separate lines. Therefore, update > the..."). I don't think it needs to be updated, but it's something to keep > in mind for future contributions. :) I've taken note of it. Thanks! > Whitespace looks good here. I think this is ready-to-merge; thanks! Thanks again for the review! ``` ## Vivan Garg, 2023-02-22 05:19 Subject: Re: [GSOC][PATCH] t4121: modernize test style Message-ID: URL: https://gitlist.dev/e/CACzddJrZ8YdJ72ng3UpMGN9CJx0qW1%2BfZfyi3q01z2487V8fxw%40mail.gmail.com In-Reply-To: ``` On Tue, Feb 21, 2023 at 1:21 PM Junio C Hamano wrote: > I am not Victoria, but I think > > * You had two (or more) changes in your patch. > > * She found one of them is good. > > * The other one(s) were found lacking. > > When a patch is reviewed and found to be lacking (as a whole), and > if you agree with the issues pointed out by the review(s), you'd > redo the patch (with either "commit --amend" for a single patch, or > "rebase -i" for a series with multiple patches), and produce v2, as > if v1 never happened. The act of doing so is colloquially called > "to reroll". > > Documentation/MyFirstContribution.txt::[[now-what]] may have more > details. > > Thanks. That makes sense. Thanks for the clarification! ```