patcht4121: modernize test style

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 <gvivan6@gmail.com>
---
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(-)

Vivan Garg 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?

Welcome to the Git community, and thanks for your contribution! :) 

This test looks good.

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)?

On Tue, Feb 21, 2023 at 10:27 AM Victoria Dye <vdye@github.com> wrote:

For sure! Is it correct that I will need to amend the commit message and send out a v2 of the patch?

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?

Vivan Garg <v.garg.work@gmail.com> writes:

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.

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 <gvivan6@gmail.com>
     
    @@ 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

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 <gvivan6@gmail.com>
---
 t/t4121-apply-diffs.sh | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

On Tue, Feb 21, 2023 at 1:21 PM Junio C Hamano <gitster@pobox.com> wrote:

That makes sense. Thanks for the clarification!

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 <gvivan6@gmail.com>
     
    @@ 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

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 <gvivan6@gmail.com>
---
 t/t4121-apply-diffs.sh | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

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 <gvivan6@gmail.com>
     
    @@ 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

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 <gvivan6@gmail.com>
---
 t/t4121-apply-diffs.sh | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Vivan Garg 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. :)

Whitespace looks good here. I think this is ready-to-merge; thanks!

On Tue, Feb 21, 2023 at 3:17 PM Victoria Dye <vdye@github.com> wrote:

I've taken note of it. Thanks!

Thanks again for the review!