* [GSOC][PATCH] t4121: modernize test style
@ 2023-02-20 23:51 Vivan Garg
2023-02-21 17:22 ` Victoria Dye
2023-02-21 21:46 ` [GSOC][PATCH v2 0/1] " Vivan Garg
0 siblings, 2 replies; 11+ messages in thread
From: Vivan Garg @ 2023-02-20 23:51 UTC (permalink / raw)
To: git, vdye; +Cc: christian.couder, hariom18599, Vivan Garg
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(-)
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)
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [GSOC][PATCH] t4121: modernize test style
2023-02-20 23:51 [GSOC][PATCH] t4121: modernize test style Vivan Garg
@ 2023-02-21 17:22 ` Victoria Dye
2023-02-21 19:48 ` Vivan Garg
2023-02-21 21:46 ` [GSOC][PATCH v2 0/1] " Vivan Garg
1 sibling, 1 reply; 11+ messages in thread
From: Victoria Dye @ 2023-02-21 17:22 UTC (permalink / raw)
To: Vivan Garg, git; +Cc: christian.couder, hariom18599
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 <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!
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GSOC][PATCH] t4121: modernize test style
2023-02-21 17:22 ` Victoria Dye
@ 2023-02-21 19:48 ` Vivan Garg
2023-02-21 20:21 ` Junio C Hamano
0 siblings, 1 reply; 11+ messages in thread
From: Vivan Garg @ 2023-02-21 19:48 UTC (permalink / raw)
To: Victoria Dye; +Cc: Vivan Garg, git, christian.couder, hariom18599
On Tue, Feb 21, 2023 at 10:27 AM Victoria Dye <vdye@github.com> 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?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GSOC][PATCH] t4121: modernize test style
2023-02-21 19:48 ` Vivan Garg
@ 2023-02-21 20:21 ` Junio C Hamano
2023-02-21 21:27 ` [GSOC][PATCH v2 0/1] " Vivan Garg
2023-02-22 5:19 ` [GSOC][PATCH] " Vivan Garg
0 siblings, 2 replies; 11+ messages in thread
From: Junio C Hamano @ 2023-02-21 20:21 UTC (permalink / raw)
To: Vivan Garg; +Cc: Victoria Dye, Vivan Garg, git, christian.couder, hariom18599
Vivan Garg <v.garg.work@gmail.com> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [GSOC][PATCH v2 0/1] t4121: modernize test style
2023-02-21 20:21 ` Junio C Hamano
@ 2023-02-21 21:27 ` Vivan Garg
2023-02-21 21:27 ` [GSOC][PATCH v2 1/1] " Vivan Garg
2023-02-22 5:19 ` [GSOC][PATCH] " Vivan Garg
1 sibling, 1 reply; 11+ messages in thread
From: Vivan Garg @ 2023-02-21 21:27 UTC (permalink / raw)
To: git, vdye; +Cc: christian.couder, hariom18599, Vivan Garg
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
--
2.37.0 (Apple Git-136)
^ permalink raw reply [flat|nested] 11+ messages in thread
* [GSOC][PATCH v2 1/1] t4121: modernize test style
2023-02-21 21:27 ` [GSOC][PATCH v2 0/1] " Vivan Garg
@ 2023-02-21 21:27 ` Vivan Garg
0 siblings, 0 replies; 11+ messages in thread
From: Vivan Garg @ 2023-02-21 21:27 UTC (permalink / raw)
To: git, vdye; +Cc: christian.couder, hariom18599, Vivan Garg
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(-)
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)
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [GSOC][PATCH v2 0/1] t4121: modernize test style
2023-02-20 23:51 [GSOC][PATCH] t4121: modernize test style Vivan Garg
2023-02-21 17:22 ` Victoria Dye
@ 2023-02-21 21:46 ` Vivan Garg
2023-02-21 21:46 ` [GSOC][PATCH v2 1/1] " Vivan Garg
1 sibling, 1 reply; 11+ messages in thread
From: Vivan Garg @ 2023-02-21 21:46 UTC (permalink / raw)
To: git, vdye; +Cc: christian.couder, hariom18599, Vivan Garg
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
--
2.37.0 (Apple Git-136)
^ permalink raw reply [flat|nested] 11+ messages in thread
* [GSOC][PATCH v2 1/1] t4121: modernize test style
2023-02-21 21:46 ` [GSOC][PATCH v2 0/1] " Vivan Garg
@ 2023-02-21 21:46 ` Vivan Garg
2023-02-21 22:05 ` Victoria Dye
0 siblings, 1 reply; 11+ messages in thread
From: Vivan Garg @ 2023-02-21 21:46 UTC (permalink / raw)
To: git, vdye; +Cc: christian.couder, hariom18599, Vivan Garg
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(-)
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)
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [GSOC][PATCH v2 1/1] t4121: modernize test style
2023-02-21 21:46 ` [GSOC][PATCH v2 1/1] " Vivan Garg
@ 2023-02-21 22:05 ` Victoria Dye
2023-02-21 22:34 ` Vivan Garg
0 siblings, 1 reply; 11+ messages in thread
From: Victoria Dye @ 2023-02-21 22:05 UTC (permalink / raw)
To: Vivan Garg, git; +Cc: christian.couder, hariom18599
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 <gvivan6@gmail.com>
> ---
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GSOC][PATCH v2 1/1] t4121: modernize test style
2023-02-21 22:05 ` Victoria Dye
@ 2023-02-21 22:34 ` Vivan Garg
0 siblings, 0 replies; 11+ messages in thread
From: Vivan Garg @ 2023-02-21 22:34 UTC (permalink / raw)
To: Victoria Dye; +Cc: Vivan Garg, git, christian.couder, hariom18599
On Tue, Feb 21, 2023 at 3:17 PM Victoria Dye <vdye@github.com> 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!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GSOC][PATCH] t4121: modernize test style
2023-02-21 20:21 ` Junio C Hamano
2023-02-21 21:27 ` [GSOC][PATCH v2 0/1] " Vivan Garg
@ 2023-02-22 5:19 ` Vivan Garg
1 sibling, 0 replies; 11+ messages in thread
From: Vivan Garg @ 2023-02-22 5:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Vivan Garg, Victoria Dye, git, christian.couder, hariom18599
On Tue, Feb 21, 2023 at 1:21 PM Junio C Hamano <gitster@pobox.com> 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!
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-02-22 5:19 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-20 23:51 [GSOC][PATCH] t4121: modernize test style Vivan Garg
2023-02-21 17:22 ` Victoria Dye
2023-02-21 19:48 ` Vivan Garg
2023-02-21 20:21 ` Junio C Hamano
2023-02-21 21:27 ` [GSOC][PATCH v2 0/1] " Vivan Garg
2023-02-21 21:27 ` [GSOC][PATCH v2 1/1] " Vivan Garg
2023-02-22 5:19 ` [GSOC][PATCH] " Vivan Garg
2023-02-21 21:46 ` [GSOC][PATCH v2 0/1] " Vivan Garg
2023-02-21 21:46 ` [GSOC][PATCH v2 1/1] " Vivan Garg
2023-02-21 22:05 ` Victoria Dye
2023-02-21 22:34 ` Vivan Garg
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 NNTP newsgroup(s).