threads / patch / 53031

patch, 2 partst4131: update test script

Subject: [GSoC][Patch 0/2] t4131: update test script

## tl;dr

17 messages between Mar 19, 2020 and Mar 20, 2020. Diffs are folded; open one to read it.

replies: 16people: 4as markdown or json

Harshit Jain· Mar 19, 2020, 13:29 UTC · lore
Greetings!
Here is my very first contribution to the open-source community. I have always been a great admirer of the open-source developments and am really excited to begin my journey in the open source development.
In this patch, I have:
        - modernized the script code to adhere to the CodingGuidelines
        - replaced 'test -f' with the helper function 'test_path_is_file' as it make the code more readable and also gives better error messages
Just to add, I have done this as a microproject for my GSoC application, and am hoping to contribute more to the git during the coming summers.

Thanks, Harshit Jain

Harshit Jain· Mar 19, 2020, 13:29 UTC · re: Harshit Jain · lore

[GSoC][PATCH 1/2] t4131: modernize style

The tests in 't4131-apply-fake-ancestor.sh' were written a long time ago, and have a few style violations. Update it to adhere to the CodingGuidelines.
Signed-off-by: Harshit Jain <harshitjain1371999@gmail.com>
---
 t/t4131-apply-fake-ancestor.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to t/t4131-apply-fake-ancestor.sh +4 −4
diff --git a/t/t4131-apply-fake-ancestor.sh b/t/t4131-apply-fake-ancestor.sh
index b1361ce546..828d1a355b 100755
--- a/t/t4131-apply-fake-ancestor.sh
+++ b/t/t4131-apply-fake-ancestor.sh
@@ -17,8 +17,8 @@ test_expect_success 'setup' '
 
 test_expect_success 'apply --build-fake-ancestor' '
 	git checkout 2 &&
-	echo "A" > 1.t &&
-	git diff > 1.patch &&
+	echo "A" >1.t &&
+	git diff >1.patch &&
 	git reset --hard &&
 	git checkout 1 &&
 	git apply --build-fake-ancestor 1.ancestor 1.patch
@@ -26,8 +26,8 @@ test_expect_success 'apply --build-fake-ancestor' '
 
 test_expect_success 'apply --build-fake-ancestor in a subdirectory' '
 	git checkout 3 &&
-	echo "C" > sub/3.t &&
-	git diff > 3.patch &&
+	echo "C" >sub/3.t &&
+	git diff >3.patch &&
 	git reset --hard &&
 	git checkout 4 &&
 	(

-- 
2.26.0.rc2
Shourya Shukla· Mar 19, 2020, 16:38 UTC · re: Harshit Jain · lore

Re: [GSoC][PATCH 1/2] t4131: modernize style

Hello Harshit,
> The tests in 't4131-apply-fake-ancestor.sh' were written a long time ago, and have a few style violations. Update it to adhere to the CodingGuidelines.

Maybe add a commit title and then have a body? To do so, do a 'git commit' instead of 'git commit -m "message"'. This will open a text editor in which you can edit your commit message. You may refer to this answer I gave on StackOverflow on commit messages:

https://stackoverflow.com/a/60755299/10751129

Also, commit messages are generally around 72 characters per line. What are the style violations you are talking about BTW?

The commit title can be of the form:
t4131: modernise style
<<commit description>>

Regards, Shourya Shukla

Harshit Jain· Mar 19, 2020, 17:45 UTC · re: Shourya Shukla · lore

Re: [GSoC][PATCH 1/2] t4131: modernize style

Hi Shourya,
Show 6 quoted lines
> > The tests in 't4131-apply-fake-ancestor.sh' were written a long time ago, and have a few style violations. Update it to adhere to the CodingGuidelines.
>
> Maybe add a commit title and then have a body? To do so, do a 'git commit' instead of 'git commit -m "message"'. This will open a text editor
> in which you can edit your commit message. You may refer to this answer I gave on StackOverflow on commit messages:
>
> https://stackoverflow.com/a/60755299/10751129

I used 'git commit' only and not 'git commit -m "message". But apparently, the git format-patch tool takes the first line of commit message i.e. the commit title as the file name and the lines after that as the text for the body. And hence, the patch emails, just start with the commit description and not the commit title.

So, should I explicitly add the commit title in the patch files generated or else how to handle this?

> Also, commit messages are generally around 72 characters per line. What are the
> style violations you are talking about BTW?

The git coding guidelines says that we shouldn't have a space after the redirection operators, hence I corrected this in the test file.

Regards, Harshit Jain

Junio C Hamano· Mar 19, 2020, 21:55 UTC · re: Harshit Jain · lore

Re: [GSoC][PATCH 1/2] t4131: modernize style

Harshit Jain <harshitjain1371999@gmail.com> writes:
Show 5 quoted lines
>> > The tests in 't4131-apply-fake-ancestor.sh' were written a long
>> > time ago, and have a few style violations. Update it to adhere
>> > to the CodingGuidelines.
>> ...
> I used 'git commit' only and not 'git commit -m "message". But

I'd suggest developers, especially the new ones, to stay away from using '-m "message"' form, too.

In your editor, you would probably have written something like
	-- -- -- -- -- the contents of editor window -- -- -- -- --
	t4131: modernize style
	The tests in 't4131-apply-fake-ancestor.sh' were written ...
	...
	-- -- -- -- -- the contents of editor window -- -- -- -- --

As you observed, the first paragraph of the log message text is taken as the title of the commit, and "git format-patch" places the title on the "Subject:" line (if you had more than one line in the first paragraph, since the payload on the "Subject: " line has to be a logically single line, you'd end up getting a single long line that has the contents on all lines in the first paragraph).

The second and subsequent paragraphs become the body of the message.

Your title looks reasonable; there is nothing that needs to be "fixed" or "improved" there.

Your second paragraph is not so good---it should wrap the lines at a reasonable length (say 65-70 columns).

Your last paragraph, which consists of a single "Signed-off-by:" line in this case, is good. It matches the identity recorded on the "From:" line of the message.

Show 5 quoted lines
>> Also, commit messages are generally around 72 characters per line. What are the
>> style violations you are talking about BTW?
>
> The git coding guidelines says that we shouldn't have a space after
> the redirection operators, hence I corrected this in the test file.
That is a good thing to write in the commit log message.

"written a long time ago" does not have much value by itself (it does serve as a backstory to explain a half of why it does not use the more modern style, though). "have a few style violations." is almost meaningless (otherwise, you would not be doing a "modernize style" patch in the first place ;-).

	t4131: modernize style.
	The tests in t4131 leaves a SP between a redirection
	operator and the file that is the redirection target,
	which does not conform to the modern coding style.
	Fix them.
	Signed-off-by: ...
perhaps.
Harshit Jain· Mar 19, 2020, 13:29 UTC · re: Harshit Jain · lore

[GSoC][PATCH 2/2] t4131: use helper function to replace test -f <path>

Replace 'test -f' with the helper function 'test_path_is_file' as the helper function improves the code readability and also gives better error messages.
Signed-off-by: Harshit Jain <harshitjain1371999@gmail.com>
---
 t/t4131-apply-fake-ancestor.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t4131-apply-fake-ancestor.sh +1 −1
diff --git a/t/t4131-apply-fake-ancestor.sh b/t/t4131-apply-fake-ancestor.sh
index 828d1a355b..21ee359632 100755
--- a/t/t4131-apply-fake-ancestor.sh
+++ b/t/t4131-apply-fake-ancestor.sh
@@ -33,7 +33,7 @@ test_expect_success 'apply --build-fake-ancestor in a subdirectory' '
 	(
 		cd sub &&
 		git apply --build-fake-ancestor 3.ancestor ../3.patch &&
-		test -f 3.ancestor
+		test_path_is_file 3.ancestor
 	) &&
 	git apply --build-fake-ancestor 3.ancestor 3.patch &&
 	test_cmp sub/3.ancestor 3.ancestor

-- 
2.26.0.rc2
Shourya Shukla· Mar 19, 2020, 16:42 UTC · re: Harshit Jain · lore

Re: [GSoC][PATCH 2/2] t4131: use helper function to replace test -f <path>

Hello Harshit,
> Replace 'test -f' with the helper function 'test_path_is_file' as the helper function improves the code readability and also gives better error messages.
Again the same thing, you may follow what I stated before regarding commit messages.
The commit title can be of the form:
t4131: use helpers to replace test -f <path>
<<commit description>>
If you still face any problem, feel free to drop a message :)

Regards, Shourya Shukla

Kaartic Sivaraam· Mar 19, 2020, 17:33 UTC · re: Shourya Shukla · lore

Re: [GSoC][PATCH 2/2] t4131: use helper function to replace test -f <path>

On 19-03-2020 22:12, Shourya Shukla wrote:
Show 12 quoted lines
> Hello Harshit,
> 
>> Replace 'test -f' with the helper function 'test_path_is_file' as the helper function improves the code readability and also gives better error messages.
> 
> Again the same thing, you may follow what I stated before regarding commit messages.
> 
> The commit title can be of the form:
> 
> t4131: use helpers to replace test -f <path>
> 
> <<commit description>>
>

Just curious, isn't the commit title already like that in this patch? The subject does read:

   [GSoC][PATCH 2/2] t4131: use helper function to replace test -f <path>"
What am I missing?
-- 
Sivaraam
Harshit Jain· Mar 19, 2020, 20:18 UTC · re: Kaartic Sivaraam · lore

Re: [GSoC][PATCH 2/2] t4131: use helper function to replace test -f <path>

On Thu, Mar 19, 2020 at 11:04 PM Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:

Show 22 quoted lines
>
> On 19-03-2020 22:12, Shourya Shukla wrote:
> > Hello Harshit,
> >
> >> Replace 'test -f' with the helper function 'test_path_is_file' as the helper function improves the code readability and also gives better error messages.
> >
> > Again the same thing, you may follow what I stated before regarding commit messages.
> >
> > The commit title can be of the form:
> >
> > t4131: use helpers to replace test -f <path>
> >
> > <<commit description>>
> >
>
> Just curious, isn't the commit title already like that in this patch?
> The subject does read:
>
>    [GSoC][PATCH 2/2] t4131: use helper function to replace test -f <path>"
>
> What am I missing?
>

Hey Shourya, Can you please clarify, I am also a bit confused.

Regards, Harshit Jain

Junio C Hamano· Mar 19, 2020, 21:58 UTC · re: Shourya Shukla · lore

Re: [GSoC][PATCH 2/2] t4131: use helper function to replace test -f <path>

Shourya Shukla <shouryashukla.oo@gmail.com> writes:
Show 11 quoted lines
> Hello Harshit,
>
>> Replace 'test -f' with the helper function 'test_path_is_file' as the helper function improves the code readability and also gives better error messages.
>
> Again the same thing, you may follow what I stated before regarding commit messages.
>
> The commit title can be of the form:
>
> t4131: use helpers to replace test -f <path>
>
> <<commit description>>

I think Harshit is writing the title of the commit in the right place. Format-wise, the only thing that is wrong is that each paragraph is too long without line wrapping.

What is wrong in these two e-mail thread is that you are not reading the log message correctly. When made into a piece of e-mail, the title goes to the "Subject:" field in the header and there is no need to repeat it in the body of the e-mail.

Harshit Jain· Mar 20, 2020, 13:08 UTC · re: Junio C Hamano · lore

[GSoC][Patch 0/2] made the changes as per community suggestions

Greetings!

Thank you for suggesting the changes in my patches. I have made the changes as advised by Junio C Hamano in the following patch emails. Please look into those patch mails and suggest any further changes if needed.

Thank you once again.
Harshit Jain
Harshit Jain· Mar 20, 2020, 13:08 UTC · re: Harshit Jain · lore

[PATCH 1/2] t4131: modernize style

The tests in t4131 leave a space character between the redirection operator and the file i.e. the redirection target which does not conform to the modern coding style.

Fix them.
Signed-off-by: Harshit Jain <harshitjain1371999@gmail.com>
---
 t/t4131-apply-fake-ancestor.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to t/t4131-apply-fake-ancestor.sh +4 −4
diff --git a/t/t4131-apply-fake-ancestor.sh b/t/t4131-apply-fake-ancestor.sh
index b1361ce546..828d1a355b 100755
--- a/t/t4131-apply-fake-ancestor.sh
+++ b/t/t4131-apply-fake-ancestor.sh
@@ -17,8 +17,8 @@ test_expect_success 'setup' '
 
 test_expect_success 'apply --build-fake-ancestor' '
 	git checkout 2 &&
-	echo "A" > 1.t &&
-	git diff > 1.patch &&
+	echo "A" >1.t &&
+	git diff >1.patch &&
 	git reset --hard &&
 	git checkout 1 &&
 	git apply --build-fake-ancestor 1.ancestor 1.patch
@@ -26,8 +26,8 @@ test_expect_success 'apply --build-fake-ancestor' '
 
 test_expect_success 'apply --build-fake-ancestor in a subdirectory' '
 	git checkout 3 &&
-	echo "C" > sub/3.t &&
-	git diff > 3.patch &&
+	echo "C" >sub/3.t &&
+	git diff >3.patch &&
 	git reset --hard &&
 	git checkout 4 &&
 	(

-- 
2.26.0.rc2
Shourya Shukla· Mar 20, 2020, 15:56 UTC · re: Harshit Jain · lore

Re: Re: [GSoC][Patch]

Hello Harshit,
> The tests in t4131 leave a space character between the redirection operator
> and the file i.e. the redirection target which does not conform to the
> modern coding style.
> Fix them.
I think something like,

The tests in t4131 were written a long time ago and hence contain style violations such as an extra space between the redirection operator(>) and the redirection target. Update it to match the latest CodingGuidelines.

may be better.

Also, when you deliver a newer version of the patch, i.e., version 2 in your case, you have a [PATCH v2 1/n] as the subject, so that people know that it is the v2 and hence avoid confusion.

If you are using 'git format-patch' to formulate your mails, you can do:
'git format-patch -v2 <..>' to get a v2 based mail.

Regards, Shourya Shukla

Harshit Jain· Mar 20, 2020, 17:14 UTC · re: Shourya Shukla · lore

Re: Re: [GSoC][Patch]

Hi Shourya,

On Fri, Mar 20, 2020 at 9:26 PM Shourya Shukla <shouryashukla.oo@gmail.com> wrote:

Show 17 quoted lines
>
> Hello Harshit,
>
> > The tests in t4131 leave a space character between the redirection operator
> > and the file i.e. the redirection target which does not conform to the
> > modern coding style.
>
> > Fix them.
>
> I think something like,
>
> The tests in t4131 were written a long time ago and hence contain style violations
> such as an extra space between the redirection operator(>) and the redirection target.
> Update it to match the latest CodingGuidelines.
>
> may be better.
>
Please see the comment made by Junio Hamano, pasted below:

"written a long time ago" does not have much value by itself (it does serve as a backstory to explain a half of why it does not use the more modern style, though). "have a few style violations." is almost meaningless (otherwise, you would not be doing a "modernize style" patch in the first place ;-).

I myself also agree with the above comment and hence, wrote the commit message accordingly. What do you think?

Show 8 quoted lines
> Also, when you deliver a newer version of the patch, i.e., version 2 in your case,
> you have a [PATCH v2 1/n] as the subject, so that people know that it is the v2 and
> hence avoid confusion.
>
> If you are using 'git format-patch' to formulate your mails, you can do:
>
> 'git format-patch -v2 <..>' to get a v2 based mail.
>

Oh nice, didn't know about this. I will keep this in mind for future patch submissions. Should I do this for the current patch as well?

Regards, Harshit Jain

Harshit Jain· Mar 20, 2020, 13:08 UTC · re: Harshit Jain · lore

[PATCH 2/2] t4131: use helper function to replace 'test -f'

Replace 'test -f' with the helper function 'test_path_is_file' as the helper function improves the code readability and also gives better error messages.

Signed-off-by: Harshit Jain <harshitjain1371999@gmail.com>
---
 t/t4131-apply-fake-ancestor.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t4131-apply-fake-ancestor.sh +1 −1
diff --git a/t/t4131-apply-fake-ancestor.sh b/t/t4131-apply-fake-ancestor.sh
index 828d1a355b..21ee359632 100755
--- a/t/t4131-apply-fake-ancestor.sh
+++ b/t/t4131-apply-fake-ancestor.sh
@@ -33,7 +33,7 @@ test_expect_success 'apply --build-fake-ancestor in a subdirectory' '
 	(
 		cd sub &&
 		git apply --build-fake-ancestor 3.ancestor ../3.patch &&
-		test -f 3.ancestor
+		test_path_is_file 3.ancestor
 	) &&
 	git apply --build-fake-ancestor 3.ancestor 3.patch &&
 	test_cmp sub/3.ancestor 3.ancestor

-- 
2.26.0.rc2

← back to recent threads