git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] t6025: updating tests
@ 2020-01-16 20:36 Shourya Shukla
  2020-01-16 20:36 ` [PATCH 1/3] t6025: modernize style Shourya Shukla
                   ` (3 more replies)
  0 siblings, 4 replies; 24+ messages in thread
From: Shourya Shukla @ 2020-01-16 20:36 UTC (permalink / raw)
  To: git, gitster, johannes.schindelin; +Cc: Shourya Shukla

Greetings everyone!

This is my first ever contribution in the Open-Source Community and I chose Git
for this purpose as I have been using this important tool to maintain my projects
regularly.

In this patch, I have:

  - modernized these tests so that they meet current Git CodingGuidlines[1]
  - replaced the pipe operator with the redirection operator so that one can
	detect the errors easily and precisely
  - used helper function 'test_path_is_file()' to replace 'test -f' checks in
	in the program as it improves the readability of the code and provides 
	better error messages

Also, I have some questions to better my understanding of the code:
	- In the statement, 
		> git hash-object -t blob -w --stdin
	  is it necessary to explicitly specify the type 'blob' of the hash-object?
	  I have this question because it is the default type of hash-object.
	- In the statement, 
		> l=$(printf file | git hash-object -t blob -w --stdin)
	  I have not used the redirection operator as this sub-shell will be executed 
	  separately, hence its error cannot be captured therefore the presence of '>' 
	  will not matter. Will using '>' improve the code?

Thanks,
Shourya Shukla

Shourya Shukla (3):
  t6025: modernize style
  t6025: replace pipe with redirection operator
  t6025: use helpers to replace test -f <path>

 t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 47 deletions(-)

-- 
2.20.1

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/3] t6025: modernize style
  2020-01-16 20:36 [PATCH 0/3] t6025: updating tests Shourya Shukla
@ 2020-01-16 20:36 ` Shourya Shukla
  2020-01-16 21:42   ` Johannes Schindelin
  2020-01-16 20:36 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 24+ messages in thread
From: Shourya Shukla @ 2020-01-16 20:36 UTC (permalink / raw)
  To: git, gitster, johannes.schindelin; +Cc: Shourya Shukla

The tests in `t6025-merge-symlinks.sh` were written a long time ago, and
has a lot of style violations, including the mixed-use of tabs and spaces,
missing indentations, and other shell script style violations. Update it to
match the CodingGuidelines.

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 47 deletions(-)

diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
index 433c4de08f..b9219af659 100755
--- a/t/t6025-merge-symlinks.sh
+++ b/t/t6025-merge-symlinks.sh
@@ -10,52 +10,55 @@ if core.symlinks is false.'
 
 . ./test-lib.sh
 
-test_expect_success \
-'setup' '
-git config core.symlinks false &&
-> file &&
-git add file &&
-git commit -m initial &&
-git branch b-symlink &&
-git branch b-file &&
-l=$(printf file | git hash-object -t blob -w --stdin) &&
-echo "120000 $l	symlink" | git update-index --index-info &&
-git commit -m master &&
-git checkout b-symlink &&
-l=$(printf file-different | git hash-object -t blob -w --stdin) &&
-echo "120000 $l	symlink" | git update-index --index-info &&
-git commit -m b-symlink &&
-git checkout b-file &&
-echo plain-file > symlink &&
-git add symlink &&
-git commit -m b-file'
-
-test_expect_success \
-'merge master into b-symlink, which has a different symbolic link' '
-git checkout b-symlink &&
-test_must_fail git merge master'
-
-test_expect_success \
-'the merge result must be a file' '
-test -f symlink'
-
-test_expect_success \
-'merge master into b-file, which has a file instead of a symbolic link' '
-git reset --hard && git checkout b-file &&
-test_must_fail git merge master'
-
-test_expect_success \
-'the merge result must be a file' '
-test -f symlink'
-
-test_expect_success \
-'merge b-file, which has a file instead of a symbolic link, into master' '
-git reset --hard &&
-git checkout master &&
-test_must_fail git merge b-file'
-
-test_expect_success \
-'the merge result must be a file' '
-test -f symlink'
+test_expect_success 'setup' '
+	git config core.symlinks false &&
+	touch file &&
+	git add file &&
+	git commit -m initial &&
+	git branch b-symlink &&
+	git branch b-file &&
+	l=$(printf file | git hash-object -t blob -w --stdin) &&
+	echo "120000 $l	symlink" |
+	git update-index --index-info &&
+	git commit -m master &&
+	git checkout b-symlink &&
+	l=$(printf file-different | git hash-object -t blob -w --stdin) &&
+	echo "120000 $l	symlink" |
+	git update-index --index-info &&
+	git commit -m b-symlink &&
+	git checkout b-file &&
+	echo plain-file >symlink &&
+	git add symlink &&
+	git commit -m b-file
+'
+
+test_expect_success 'merge master into b-symlink, which has a different symbolic link' '
+	git checkout b-symlink &&
+	test_must_fail git merge master
+'
+
+test_expect_success 'the merge result must be a file' '
+	test -f symlink
+'
+
+test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' '
+	git reset --hard &&
+	git checkout b-file &&
+	test_must_fail git merge master
+'
+
+test_expect_success 'the merge result must be a file' '
+	test -f symlink
+'
+
+test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' '
+	git reset --hard &&
+	git checkout master &&
+	test_must_fail git merge b-file
+'
+
+test_expect_success 'the merge result must be a file' '
+	test -f symlink
+'
 
 test_done
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/3] t6025: replace pipe with redirection operator
  2020-01-16 20:36 [PATCH 0/3] t6025: updating tests Shourya Shukla
  2020-01-16 20:36 ` [PATCH 1/3] t6025: modernize style Shourya Shukla
@ 2020-01-16 20:36 ` Shourya Shukla
  2020-01-16 22:57   ` Junio C Hamano
  2020-01-16 20:36 ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla
  2020-01-16 21:46 ` [PATCH 0/3] t6025: updating tests Johannes Schindelin
  3 siblings, 1 reply; 24+ messages in thread
From: Shourya Shukla @ 2020-01-16 20:36 UTC (permalink / raw)
  To: git, gitster, johannes.schindelin; +Cc: Shourya Shukla

The exit code of pipes(|) are always ignored, which will create
errors in subsequent statements. Let's handle it by redirecting
its output to a file and capturing return values. Replace pipe
with redirect(>) operator.

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t6025-merge-symlinks.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
index b9219af659..41bae56ea9 100755
--- a/t/t6025-merge-symlinks.sh
+++ b/t/t6025-merge-symlinks.sh
@@ -18,13 +18,13 @@ test_expect_success 'setup' '
 	git branch b-symlink &&
 	git branch b-file &&
 	l=$(printf file | git hash-object -t blob -w --stdin) &&
-	echo "120000 $l	symlink" |
-	git update-index --index-info &&
+	echo "120000 $l	symlink" >foo &&
+	git update-index --index-info <foo &&
 	git commit -m master &&
 	git checkout b-symlink &&
 	l=$(printf file-different | git hash-object -t blob -w --stdin) &&
-	echo "120000 $l	symlink" |
-	git update-index --index-info &&
+	echo "120000 $l	symlink" >foo &&
+	git update-index --index-info <foo &&
 	git commit -m b-symlink &&
 	git checkout b-file &&
 	echo plain-file >symlink &&
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 3/3] t6025: use helpers to replace test -f <path>
  2020-01-16 20:36 [PATCH 0/3] t6025: updating tests Shourya Shukla
  2020-01-16 20:36 ` [PATCH 1/3] t6025: modernize style Shourya Shukla
  2020-01-16 20:36 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla
@ 2020-01-16 20:36 ` Shourya Shukla
  2020-01-16 22:58   ` Junio C Hamano
  2020-01-16 21:46 ` [PATCH 0/3] t6025: updating tests Johannes Schindelin
  3 siblings, 1 reply; 24+ messages in thread
From: Shourya Shukla @ 2020-01-16 20:36 UTC (permalink / raw)
  To: git, gitster, johannes.schindelin; +Cc: Shourya Shukla

Take advantage of helper function 'test_path_is_file()' to
replace 'test -f' since the function makes the code more
readable and gives better error messages.

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t6025-merge-symlinks.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
index 41bae56ea9..ebbbc03f1d 100755
--- a/t/t6025-merge-symlinks.sh
+++ b/t/t6025-merge-symlinks.sh
@@ -38,7 +38,7 @@ test_expect_success 'merge master into b-symlink, which has a different symbolic
 '
 
 test_expect_success 'the merge result must be a file' '
-	test -f symlink
+	test_path_is_file symlink
 '
 
 test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' '
@@ -48,7 +48,7 @@ test_expect_success 'merge master into b-file, which has a file instead of a sym
 '
 
 test_expect_success 'the merge result must be a file' '
-	test -f symlink
+	test_path_is_file symlink
 '
 
 test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' '
@@ -58,7 +58,7 @@ test_expect_success 'merge b-file, which has a file instead of a symbolic link,
 '
 
 test_expect_success 'the merge result must be a file' '
-	test -f symlink
+	test_path_is_file symlink
 '
 
 test_done
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/3] t6025: modernize style
  2020-01-16 20:36 ` [PATCH 1/3] t6025: modernize style Shourya Shukla
@ 2020-01-16 21:42   ` Johannes Schindelin
  0 siblings, 0 replies; 24+ messages in thread
From: Johannes Schindelin @ 2020-01-16 21:42 UTC (permalink / raw)
  To: Shourya Shukla; +Cc: git, gitster

Hi,

On Fri, 17 Jan 2020, Shourya Shukla wrote:

> The tests in `t6025-merge-symlinks.sh` were written a long time ago, and
> has a lot of style violations, including the mixed-use of tabs and spaces,
> missing indentations, and other shell script style violations. Update it to
> match the CodingGuidelines.
>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---

Sounds good. Just one nit:

>  t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++-------------------
>  1 file changed, 50 insertions(+), 47 deletions(-)
>
> diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
> index 433c4de08f..b9219af659 100755
> --- a/t/t6025-merge-symlinks.sh
> +++ b/t/t6025-merge-symlinks.sh
> @@ -10,52 +10,55 @@ if core.symlinks is false.'
>
>  . ./test-lib.sh
>
> -test_expect_success \
> -'setup' '
> -git config core.symlinks false &&
> -> file &&

Here, `file` is written as a 0-byte file, and...

> -git add file &&
> -git commit -m initial &&
> -git branch b-symlink &&
> -git branch b-file &&
> -l=$(printf file | git hash-object -t blob -w --stdin) &&
> -echo "120000 $l	symlink" | git update-index --index-info &&
> -git commit -m master &&
> -git checkout b-symlink &&
> -l=$(printf file-different | git hash-object -t blob -w --stdin) &&
> -echo "120000 $l	symlink" | git update-index --index-info &&
> -git commit -m b-symlink &&
> -git checkout b-file &&
> -echo plain-file > symlink &&
> -git add symlink &&
> -git commit -m b-file'
> -
> -test_expect_success \
> -'merge master into b-symlink, which has a different symbolic link' '
> -git checkout b-symlink &&
> -test_must_fail git merge master'
> -
> -test_expect_success \
> -'the merge result must be a file' '
> -test -f symlink'
> -
> -test_expect_success \
> -'merge master into b-file, which has a file instead of a symbolic link' '
> -git reset --hard && git checkout b-file &&
> -test_must_fail git merge master'
> -
> -test_expect_success \
> -'the merge result must be a file' '
> -test -f symlink'
> -
> -test_expect_success \
> -'merge b-file, which has a file instead of a symbolic link, into master' '
> -git reset --hard &&
> -git checkout master &&
> -test_must_fail git merge b-file'
> -
> -test_expect_success \
> -'the merge result must be a file' '
> -test -f symlink'
> +test_expect_success 'setup' '
> +	git config core.symlinks false &&
> +	touch file &&

... here we now use `touch` instead. We do prefer `>file` in this
instance, though, I think. At least we do not prohibit it.

Otherwise it looks very good!
Johannes

> +	git add file &&
> +	git commit -m initial &&
> +	git branch b-symlink &&
> +	git branch b-file &&
> +	l=$(printf file | git hash-object -t blob -w --stdin) &&
> +	echo "120000 $l	symlink" |
> +	git update-index --index-info &&
> +	git commit -m master &&
> +	git checkout b-symlink &&
> +	l=$(printf file-different | git hash-object -t blob -w --stdin) &&
> +	echo "120000 $l	symlink" |
> +	git update-index --index-info &&
> +	git commit -m b-symlink &&
> +	git checkout b-file &&
> +	echo plain-file >symlink &&
> +	git add symlink &&
> +	git commit -m b-file
> +'
> +
> +test_expect_success 'merge master into b-symlink, which has a different symbolic link' '
> +	git checkout b-symlink &&
> +	test_must_fail git merge master
> +'
> +
> +test_expect_success 'the merge result must be a file' '
> +	test -f symlink
> +'
> +
> +test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' '
> +	git reset --hard &&
> +	git checkout b-file &&
> +	test_must_fail git merge master
> +'
> +
> +test_expect_success 'the merge result must be a file' '
> +	test -f symlink
> +'
> +
> +test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' '
> +	git reset --hard &&
> +	git checkout master &&
> +	test_must_fail git merge b-file
> +'
> +
> +test_expect_success 'the merge result must be a file' '
> +	test -f symlink
> +'
>
>  test_done
> --
> 2.20.1
>
>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/3] t6025: updating tests
  2020-01-16 20:36 [PATCH 0/3] t6025: updating tests Shourya Shukla
                   ` (2 preceding siblings ...)
  2020-01-16 20:36 ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla
@ 2020-01-16 21:46 ` Johannes Schindelin
  3 siblings, 0 replies; 24+ messages in thread
From: Johannes Schindelin @ 2020-01-16 21:46 UTC (permalink / raw)
  To: Shourya Shukla; +Cc: git, gitster

Hi,

On Fri, 17 Jan 2020, Shourya Shukla wrote:

> Greetings everyone!
>
> This is my first ever contribution in the Open-Source Community and I chose Git
> for this purpose as I have been using this important tool to maintain my projects
> regularly.
>
> In this patch, I have:
>
>   - modernized these tests so that they meet current Git CodingGuidlines[1]
>   - replaced the pipe operator with the redirection operator so that one can
> 	detect the errors easily and precisely
>   - used helper function 'test_path_is_file()' to replace 'test -f' checks in
> 	in the program as it improves the readability of the code and provides
> 	better error messages
>
> Also, I have some questions to better my understanding of the code:
> 	- In the statement,
> 		> git hash-object -t blob -w --stdin
> 	  is it necessary to explicitly specify the type 'blob' of the hash-object?
> 	  I have this question because it is the default type of hash-object.

It is the default type, but:

1) the code is not broken, so why fix it?

2) it _might_ be possible that the default changes, or can be configured
   in the future. The original author might just have wanted to stay safe.

> 	- In the statement,
> 		> l=$(printf file | git hash-object -t blob -w --stdin)
> 	  I have not used the redirection operator as this sub-shell will be executed
> 	  separately, hence its error cannot be captured therefore the presence of '>'
> 	  will not matter. Will using '>' improve the code?

It will be enhanced, though:

	printf file >file &&
	l=$(git hash-object -t blob -w --stdin)

will have a non-zero exit code if the `hash_object` call fails.

>
> Thanks,
> Shourya Shukla
>
> Shourya Shukla (3):
>   t6025: modernize style
>   t6025: replace pipe with redirection operator
>   t6025: use helpers to replace test -f <path>

Apart from one little issue in the first patch, this all looks very good
to me.

Thanks,
Johannes

>
>  t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++-------------------
>  1 file changed, 50 insertions(+), 47 deletions(-)
>
> --
> 2.20.1
>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/3] t6025: replace pipe with redirection operator
  2020-01-16 20:36 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla
@ 2020-01-16 22:57   ` Junio C Hamano
  0 siblings, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2020-01-16 22:57 UTC (permalink / raw)
  To: Shourya Shukla; +Cc: git, johannes.schindelin

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> -	echo "120000 $l	symlink" |
> -	git update-index --index-info &&
> +	echo "120000 $l	symlink" >foo &&
> +	git update-index --index-info <foo &&

If we had "git" on the left-hand-side (i.e. upstream) of a pipe, it
would make sense to split the pipeline like this, but this (and the
other one this patch touches) is on the right side, whose exit
status is not lost.  And we are not in the business of preparing for
broken implementation of "echo".

So this rewrite is unnecessary and unwarranted.

By the way, I think the pipeline

	echo ... | git update-index --index-info &&

should be written on a single line in the previous step 1/3.

>  	git commit -m master &&
>  	git checkout b-symlink &&
>  	l=$(printf file-different | git hash-object -t blob -w --stdin) &&
> -	echo "120000 $l	symlink" |
> -	git update-index --index-info &&
> +	echo "120000 $l	symlink" >foo &&
> +	git update-index --index-info <foo &&
>  	git commit -m b-symlink &&
>  	git checkout b-file &&
>  	echo plain-file >symlink &&

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 3/3] t6025: use helpers to replace test -f <path>
  2020-01-16 20:36 ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla
@ 2020-01-16 22:58   ` Junio C Hamano
  2020-01-17 20:44     ` [PATCH 0/3] t6025: amended changes after suggestions from the community Shourya Shukla
  0 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2020-01-16 22:58 UTC (permalink / raw)
  To: Shourya Shukla; +Cc: git, johannes.schindelin

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> Take advantage of helper function 'test_path_is_file()' to
> replace 'test -f' since the function makes the code more
> readable and gives better error messages.
>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
>  t/t6025-merge-symlinks.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Makes sense.

> diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
> index 41bae56ea9..ebbbc03f1d 100755
> --- a/t/t6025-merge-symlinks.sh
> +++ b/t/t6025-merge-symlinks.sh
> @@ -38,7 +38,7 @@ test_expect_success 'merge master into b-symlink, which has a different symbolic
>  '
>  
>  test_expect_success 'the merge result must be a file' '
> -	test -f symlink
> +	test_path_is_file symlink
>  '
>  
>  test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' '
> @@ -48,7 +48,7 @@ test_expect_success 'merge master into b-file, which has a file instead of a sym
>  '
>  
>  test_expect_success 'the merge result must be a file' '
> -	test -f symlink
> +	test_path_is_file symlink
>  '
>  
>  test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' '
> @@ -58,7 +58,7 @@ test_expect_success 'merge b-file, which has a file instead of a symbolic link,
>  '
>  
>  test_expect_success 'the merge result must be a file' '
> -	test -f symlink
> +	test_path_is_file symlink
>  '
>  
>  test_done

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 0/3] t6025: amended changes after suggestions from the community
  2020-01-16 22:58   ` Junio C Hamano
@ 2020-01-17 20:44     ` Shourya Shukla
  2020-01-17 20:44       ` [PATCH 1/3] t6025: modernize style Shourya Shukla
                         ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Shourya Shukla @ 2020-01-17 20:44 UTC (permalink / raw)
  To: gitster; +Cc: git, johannes.schindelin, shouryashukla.oo

Greetings everyone!

I have made the changes in my patch on the advise of Junio C Hamano and 
Johannes Schindelin. Thank you for looking into my changes, this has given
me confidence to contribute more in the Git Community.

Thank you,
Shourya Shukla

Shourya Shukla (3):
  t6025: modernize style
  t6025: replace pipe with redirection operator
  t6025: use helpers to replace test -f <path>

 t/t6025-merge-symlinks.sh | 96 ++++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 47 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/3] t6025: modernize style
  2020-01-17 20:44     ` [PATCH 0/3] t6025: amended changes after suggestions from the community Shourya Shukla
@ 2020-01-17 20:44       ` Shourya Shukla
  2020-01-17 21:15         ` Eric Sunshine
  2020-01-17 20:44       ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla
  2020-01-17 20:44       ` [PATCH 3/3] " Shourya Shukla
  2 siblings, 1 reply; 24+ messages in thread
From: Shourya Shukla @ 2020-01-17 20:44 UTC (permalink / raw)
  To: gitster; +Cc: git, johannes.schindelin, shouryashukla.oo

The tests in `t6025-merge-symlinks.sh` were written a long time ago, and
has a lot of style violations, including the mixed-use of tabs and spaces,
missing indentations, and other shell script style violations. Update it to
match the CodingGuidelines.

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 47 deletions(-)

diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
index 433c4de08f..7a19ba8520 100755
--- a/t/t6025-merge-symlinks.sh
+++ b/t/t6025-merge-symlinks.sh
@@ -10,52 +10,55 @@ if core.symlinks is false.'
 
 . ./test-lib.sh
 
-test_expect_success \
-'setup' '
-git config core.symlinks false &&
-> file &&
-git add file &&
-git commit -m initial &&
-git branch b-symlink &&
-git branch b-file &&
-l=$(printf file | git hash-object -t blob -w --stdin) &&
-echo "120000 $l	symlink" | git update-index --index-info &&
-git commit -m master &&
-git checkout b-symlink &&
-l=$(printf file-different | git hash-object -t blob -w --stdin) &&
-echo "120000 $l	symlink" | git update-index --index-info &&
-git commit -m b-symlink &&
-git checkout b-file &&
-echo plain-file > symlink &&
-git add symlink &&
-git commit -m b-file'
-
-test_expect_success \
-'merge master into b-symlink, which has a different symbolic link' '
-git checkout b-symlink &&
-test_must_fail git merge master'
-
-test_expect_success \
-'the merge result must be a file' '
-test -f symlink'
-
-test_expect_success \
-'merge master into b-file, which has a file instead of a symbolic link' '
-git reset --hard && git checkout b-file &&
-test_must_fail git merge master'
-
-test_expect_success \
-'the merge result must be a file' '
-test -f symlink'
-
-test_expect_success \
-'merge b-file, which has a file instead of a symbolic link, into master' '
-git reset --hard &&
-git checkout master &&
-test_must_fail git merge b-file'
-
-test_expect_success \
-'the merge result must be a file' '
-test -f symlink'
+test_expect_success 'setup' '
+	git config core.symlinks false &&
+	>file &&
+	git add file &&
+	git commit -m initial &&
+	git branch b-symlink &&
+	git branch b-file &&
+	l=$(printf file | git hash-object -t blob -w --stdin) &&
+	echo "120000 $l	symlink" |
+	git update-index --index-info &&
+	git commit -m master &&
+	git checkout b-symlink &&
+	l=$(printf file-different | git hash-object -t blob -w --stdin) &&
+	echo "120000 $l	symlink" |
+	git update-index --index-info &&
+	git commit -m b-symlink &&
+	git checkout b-file &&
+	echo plain-file >symlink &&
+	git add symlink &&
+	git commit -m b-file
+'
+
+test_expect_success 'merge master into b-symlink, which has a different symbolic link' '
+	git checkout b-symlink &&
+	test_must_fail git merge master
+'
+
+test_expect_success 'the merge result must be a file' '
+	test -f symlink
+'
+
+test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' '
+	git reset --hard &&
+	git checkout b-file &&
+	test_must_fail git merge master
+'
+
+test_expect_success 'the merge result must be a file' '
+	test -f symlink
+'
+
+test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' '
+	git reset --hard &&
+	git checkout master &&
+	test_must_fail git merge b-file
+'
+
+test_expect_success 'the merge result must be a file' '
+	test -f symlink
+'
 
 test_done
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/3] t6025: replace pipe with redirection operator
  2020-01-17 20:44     ` [PATCH 0/3] t6025: amended changes after suggestions from the community Shourya Shukla
  2020-01-17 20:44       ` [PATCH 1/3] t6025: modernize style Shourya Shukla
@ 2020-01-17 20:44       ` Shourya Shukla
  2020-01-17 21:24         ` Eric Sunshine
  2020-01-17 20:44       ` [PATCH 3/3] " Shourya Shukla
  2 siblings, 1 reply; 24+ messages in thread
From: Shourya Shukla @ 2020-01-17 20:44 UTC (permalink / raw)
  To: gitster; +Cc: git, johannes.schindelin, shouryashukla.oo

The exit code of pipes(|) are always ignored, which will create
errors in subsequent statements. Let's handle it by redirecting
its output to a file and capturing return values. Replace pipe
with redirect(>) operator.

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t6025-merge-symlinks.sh | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
index 7a19ba8520..5136bf1e13 100755
--- a/t/t6025-merge-symlinks.sh
+++ b/t/t6025-merge-symlinks.sh
@@ -17,14 +17,13 @@ test_expect_success 'setup' '
 	git commit -m initial &&
 	git branch b-symlink &&
 	git branch b-file &&
-	l=$(printf file | git hash-object -t blob -w --stdin) &&
-	echo "120000 $l	symlink" |
-	git update-index --index-info &&
+	printf file >file &&
+	l=$(git hash-object -t blob -w --stdin) &&
+	echo "120000 $l	symlink" | git update-index --index-info &&
 	git commit -m master &&
 	git checkout b-symlink &&
 	l=$(printf file-different | git hash-object -t blob -w --stdin) &&
-	echo "120000 $l	symlink" |
-	git update-index --index-info &&
+	echo "120000 $l	symlink" | git update-index --index-info &&
 	git commit -m b-symlink &&
 	git checkout b-file &&
 	echo plain-file >symlink &&
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 3/3] t6025: use helpers to replace test -f <path>
  2020-01-17 20:44     ` [PATCH 0/3] t6025: amended changes after suggestions from the community Shourya Shukla
  2020-01-17 20:44       ` [PATCH 1/3] t6025: modernize style Shourya Shukla
  2020-01-17 20:44       ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla
@ 2020-01-17 20:44       ` Shourya Shukla
  2 siblings, 0 replies; 24+ messages in thread
From: Shourya Shukla @ 2020-01-17 20:44 UTC (permalink / raw)
  To: gitster; +Cc: git, johannes.schindelin, shouryashukla.oo

Take advantage of helper function 'test_path_is_file()' to
replace 'test -f' since the function makes the code more
readable and gives better error messages.

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t6025-merge-symlinks.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
index 5136bf1e13..18a204bb65 100755
--- a/t/t6025-merge-symlinks.sh
+++ b/t/t6025-merge-symlinks.sh
@@ -37,7 +37,7 @@ test_expect_success 'merge master into b-symlink, which has a different symbolic
 '
 
 test_expect_success 'the merge result must be a file' '
-	test -f symlink
+	test_path_is_file symlink
 '
 
 test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' '
@@ -47,7 +47,7 @@ test_expect_success 'merge master into b-file, which has a file instead of a sym
 '
 
 test_expect_success 'the merge result must be a file' '
-	test -f symlink
+	test_path_is_file symlink
 '
 
 test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' '
@@ -57,7 +57,7 @@ test_expect_success 'merge b-file, which has a file instead of a symbolic link,
 '
 
 test_expect_success 'the merge result must be a file' '
-	test -f symlink
+	test_path_is_file symlink
 '
 
 test_done
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/3] t6025: modernize style
  2020-01-17 20:44       ` [PATCH 1/3] t6025: modernize style Shourya Shukla
@ 2020-01-17 21:15         ` Eric Sunshine
  2020-01-17 21:28           ` Junio C Hamano
  0 siblings, 1 reply; 24+ messages in thread
From: Eric Sunshine @ 2020-01-17 21:15 UTC (permalink / raw)
  To: Shourya Shukla; +Cc: Junio C Hamano, Git List, Johannes Schindelin

On Fri, Jan 17, 2020 at 3:45 PM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:
> [PATCH 1/3] t6025: modernize style

When sending a new version of a patch series, indicate this via
"[PATCH v2 1/3]", for instance. The -v option of "git format-patch"
can help automate this for you.

> The tests in `t6025-merge-symlinks.sh` were written a long time ago, and
> has a lot of style violations, including the mixed-use of tabs and spaces,
> missing indentations, and other shell script style violations. Update it to
> match the CodingGuidelines.
>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
> @@ -10,52 +10,55 @@ if core.symlinks is false.'
> +test_expect_success 'setup' '
> +       git config core.symlinks false &&
> +       >file &&
> +       git add file &&
> +       git commit -m initial &&
> +       git branch b-symlink &&
> +       git branch b-file &&
> +       l=$(printf file | git hash-object -t blob -w --stdin) &&
> +       echo "120000 $l symlink" |
> +       git update-index --index-info &&

As mentioned[1] in the review of v1, this should be written:

    echo "120000 $l symlink" | git update-index --index-info &&

[1]: https://lore.kernel.org/git/xmqqftgff1r0.fsf@gitster-ct.c.googlers.com/

> +       git commit -m master &&
> +       git checkout b-symlink &&
> +       l=$(printf file-different | git hash-object -t blob -w --stdin) &&
> +       echo "120000 $l symlink" |
> +       git update-index --index-info &&
> +       git commit -m b-symlink &&
> +       git checkout b-file &&
> +       echo plain-file >symlink &&
> +       git add symlink &&
> +       git commit -m b-file
> +'

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/3] t6025: replace pipe with redirection operator
  2020-01-17 20:44       ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla
@ 2020-01-17 21:24         ` Eric Sunshine
  2020-01-18  8:33           ` [PATCH 0/3] t6025: updating tests Shourya Shukla
  0 siblings, 1 reply; 24+ messages in thread
From: Eric Sunshine @ 2020-01-17 21:24 UTC (permalink / raw)
  To: Shourya Shukla; +Cc: Junio C Hamano, Git List, Johannes Schindelin

On Fri, Jan 17, 2020 at 3:45 PM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:
> The exit code of pipes(|) are always ignored, which will create
> errors in subsequent statements. Let's handle it by redirecting
> its output to a file and capturing return values. Replace pipe
> with redirect(>) operator.

This is not an accurate description. The proper way to explain this is
that exit code of a command upstream of a pipe is lost; the exit code
of a command downstream is not lost. We don't want to lose the exit
code of a git command, so a git command should not be upstream. (We
don't care about non-git commands being upstream when that command's
exit code is not relevant.)

> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
> @@ -17,14 +17,13 @@ test_expect_success 'setup' '
>         git commit -m initial &&
>         git branch b-symlink &&
>         git branch b-file &&
> -       l=$(printf file | git hash-object -t blob -w --stdin) &&

This command is fine as-is. We are interested in the exit code of the
git command (which is correctly downstream), and we don't care about
the exit code of 'printf' (which is upstream), so there is no reason
to rewrite this to use temporary files instead.

> +       printf file >file &&
> +       l=$(git hash-object -t blob -w --stdin) &&

Sorry, but this just doesn't make sense. You're telling "git
hash-object" to take its input from the standard input stream, yet you
don't feed anything to it on that stream. If anything, it should have
been written like this:

     l=$(git hash-object -t blob -w --stdin <file) &&

however, as noted above, there is no reason to avoid pipes in this
case, so this rewrite is unnecessary.

By the way, it's hard to imagine that this test passed once this
change was made (and, if it did pass, then that would likely indicate
that the test is somehow flawed.)

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/3] t6025: modernize style
  2020-01-17 21:15         ` Eric Sunshine
@ 2020-01-17 21:28           ` Junio C Hamano
  0 siblings, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2020-01-17 21:28 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Shourya Shukla, Git List, Johannes Schindelin

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +       l=$(printf file | git hash-object -t blob -w --stdin) &&
>> +       echo "120000 $l symlink" |
>> +       git update-index --index-info &&
>
> As mentioned[1] in the review of v1, this should be written:
>
>     echo "120000 $l symlink" | git update-index --index-info &&
>
> [1]: https://lore.kernel.org/git/xmqqftgff1r0.fsf@gitster-ct.c.googlers.com/
>
>> +       git commit -m master &&
>> +       git checkout b-symlink &&
>> +       l=$(printf file-different | git hash-object -t blob -w --stdin) &&
>> +       echo "120000 $l symlink" |
>> +       git update-index --index-info &&

Same here.  Funnily, 2/3 improves on this, but I agree that we
should get it right from the get-go.

>> +       git commit -m b-symlink &&
>> +       git checkout b-file &&
>> +       echo plain-file >symlink &&
>> +       git add symlink &&
>> +       git commit -m b-file
>> +'

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 0/3] t6025: updating tests
  2020-01-17 21:24         ` Eric Sunshine
@ 2020-01-18  8:33           ` Shourya Shukla
  2020-01-18  8:33             ` [PATCH 1/3] t6025: modernize style Shourya Shukla
                               ` (5 more replies)
  0 siblings, 6 replies; 24+ messages in thread
From: Shourya Shukla @ 2020-01-18  8:33 UTC (permalink / raw)
  To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo

Greetings everyone!

This is my first ever contribution in the Open-Source Community and I chose Git
for this purpose as I have been using this important tool to maintain my projects
regularly.

In this patch, I have:

  - modernized these tests so that they meet current Git CodingGuidlines[1]
  - replaced the pipe operator with the redirection operator so that one can
	detect the errors easily and precisely
  - used helper function 'test_path_is_file()' to replace 'test -f' checks in
	in the program as it improves the readability of the code and provides 
	better error messages

Also, I have some questions to better my understanding of the code:
	- In the statement, 
		> git hash-object -t blob -w --stdin
	  is it necessary to explicitly specify the type 'blob' of the hash-object?
	  I have this question because it is the default type of hash-object.
	- In the statement, 
		> l=$(printf file | git hash-object -t blob -w --stdin)
	  I have not used the redirection operator as this sub-shell will be executed 
	  separately, hence its error cannot be captured therefore the presence of '>' 
	  will not matter. Will using '>' improve the code?

Thanks,
Shourya Shukla

Shourya Shukla (3):
  t6025: modernize style
  t6025: replace pipe with redirection operator
  t6025: use helpers to replace test -f <path>

 t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 47 deletions(-)

-- 
2.20.1

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/3] t6025: modernize style
  2020-01-18  8:33           ` [PATCH 0/3] t6025: updating tests Shourya Shukla
@ 2020-01-18  8:33             ` Shourya Shukla
  2020-01-18  8:33             ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla
                               ` (4 subsequent siblings)
  5 siblings, 0 replies; 24+ messages in thread
From: Shourya Shukla @ 2020-01-18  8:33 UTC (permalink / raw)
  To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo

The tests in `t6025-merge-symlinks.sh` were written a long time ago, and
has a lot of style violations, including the mixed-use of tabs and spaces,
missing indentations, and other shell script style violations. Update it to
match the CodingGuidelines.

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 47 deletions(-)

diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
index 433c4de08f..b9219af659 100755
--- a/t/t6025-merge-symlinks.sh
+++ b/t/t6025-merge-symlinks.sh
@@ -10,52 +10,55 @@ if core.symlinks is false.'
 
 . ./test-lib.sh
 
-test_expect_success \
-'setup' '
-git config core.symlinks false &&
-> file &&
-git add file &&
-git commit -m initial &&
-git branch b-symlink &&
-git branch b-file &&
-l=$(printf file | git hash-object -t blob -w --stdin) &&
-echo "120000 $l	symlink" | git update-index --index-info &&
-git commit -m master &&
-git checkout b-symlink &&
-l=$(printf file-different | git hash-object -t blob -w --stdin) &&
-echo "120000 $l	symlink" | git update-index --index-info &&
-git commit -m b-symlink &&
-git checkout b-file &&
-echo plain-file > symlink &&
-git add symlink &&
-git commit -m b-file'
-
-test_expect_success \
-'merge master into b-symlink, which has a different symbolic link' '
-git checkout b-symlink &&
-test_must_fail git merge master'
-
-test_expect_success \
-'the merge result must be a file' '
-test -f symlink'
-
-test_expect_success \
-'merge master into b-file, which has a file instead of a symbolic link' '
-git reset --hard && git checkout b-file &&
-test_must_fail git merge master'
-
-test_expect_success \
-'the merge result must be a file' '
-test -f symlink'
-
-test_expect_success \
-'merge b-file, which has a file instead of a symbolic link, into master' '
-git reset --hard &&
-git checkout master &&
-test_must_fail git merge b-file'
-
-test_expect_success \
-'the merge result must be a file' '
-test -f symlink'
+test_expect_success 'setup' '
+	git config core.symlinks false &&
+	touch file &&
+	git add file &&
+	git commit -m initial &&
+	git branch b-symlink &&
+	git branch b-file &&
+	l=$(printf file | git hash-object -t blob -w --stdin) &&
+	echo "120000 $l	symlink" |
+	git update-index --index-info &&
+	git commit -m master &&
+	git checkout b-symlink &&
+	l=$(printf file-different | git hash-object -t blob -w --stdin) &&
+	echo "120000 $l	symlink" |
+	git update-index --index-info &&
+	git commit -m b-symlink &&
+	git checkout b-file &&
+	echo plain-file >symlink &&
+	git add symlink &&
+	git commit -m b-file
+'
+
+test_expect_success 'merge master into b-symlink, which has a different symbolic link' '
+	git checkout b-symlink &&
+	test_must_fail git merge master
+'
+
+test_expect_success 'the merge result must be a file' '
+	test -f symlink
+'
+
+test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' '
+	git reset --hard &&
+	git checkout b-file &&
+	test_must_fail git merge master
+'
+
+test_expect_success 'the merge result must be a file' '
+	test -f symlink
+'
+
+test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' '
+	git reset --hard &&
+	git checkout master &&
+	test_must_fail git merge b-file
+'
+
+test_expect_success 'the merge result must be a file' '
+	test -f symlink
+'
 
 test_done
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/3] t6025: replace pipe with redirection operator
  2020-01-18  8:33           ` [PATCH 0/3] t6025: updating tests Shourya Shukla
  2020-01-18  8:33             ` [PATCH 1/3] t6025: modernize style Shourya Shukla
@ 2020-01-18  8:33             ` Shourya Shukla
  2020-01-18  8:33             ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla
                               ` (3 subsequent siblings)
  5 siblings, 0 replies; 24+ messages in thread
From: Shourya Shukla @ 2020-01-18  8:33 UTC (permalink / raw)
  To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo

The exit code of pipes(|) are always ignored, which will create
errors in subsequent statements. Let's handle it by redirecting
its output to a file and capturing return values. Replace pipe
with redirect(>) operator.

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t6025-merge-symlinks.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
index b9219af659..41bae56ea9 100755
--- a/t/t6025-merge-symlinks.sh
+++ b/t/t6025-merge-symlinks.sh
@@ -18,13 +18,13 @@ test_expect_success 'setup' '
 	git branch b-symlink &&
 	git branch b-file &&
 	l=$(printf file | git hash-object -t blob -w --stdin) &&
-	echo "120000 $l	symlink" |
-	git update-index --index-info &&
+	echo "120000 $l	symlink" >foo &&
+	git update-index --index-info <foo &&
 	git commit -m master &&
 	git checkout b-symlink &&
 	l=$(printf file-different | git hash-object -t blob -w --stdin) &&
-	echo "120000 $l	symlink" |
-	git update-index --index-info &&
+	echo "120000 $l	symlink" >foo &&
+	git update-index --index-info <foo &&
 	git commit -m b-symlink &&
 	git checkout b-file &&
 	echo plain-file >symlink &&
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 3/3] t6025: use helpers to replace test -f <path>
  2020-01-18  8:33           ` [PATCH 0/3] t6025: updating tests Shourya Shukla
  2020-01-18  8:33             ` [PATCH 1/3] t6025: modernize style Shourya Shukla
  2020-01-18  8:33             ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla
@ 2020-01-18  8:33             ` Shourya Shukla
  2020-01-18  8:33             ` [PATCH v3 0/2] t025: amended changes after suggestions from the community Shourya Shukla
                               ` (2 subsequent siblings)
  5 siblings, 0 replies; 24+ messages in thread
From: Shourya Shukla @ 2020-01-18  8:33 UTC (permalink / raw)
  To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo

Take advantage of helper function 'test_path_is_file()' to
replace 'test -f' since the function makes the code more
readable and gives better error messages.

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t6025-merge-symlinks.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
index 41bae56ea9..ebbbc03f1d 100755
--- a/t/t6025-merge-symlinks.sh
+++ b/t/t6025-merge-symlinks.sh
@@ -38,7 +38,7 @@ test_expect_success 'merge master into b-symlink, which has a different symbolic
 '
 
 test_expect_success 'the merge result must be a file' '
-	test -f symlink
+	test_path_is_file symlink
 '
 
 test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' '
@@ -48,7 +48,7 @@ test_expect_success 'merge master into b-file, which has a file instead of a sym
 '
 
 test_expect_success 'the merge result must be a file' '
-	test -f symlink
+	test_path_is_file symlink
 '
 
 test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' '
@@ -58,7 +58,7 @@ test_expect_success 'merge b-file, which has a file instead of a symbolic link,
 '
 
 test_expect_success 'the merge result must be a file' '
-	test -f symlink
+	test_path_is_file symlink
 '
 
 test_done
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v3 0/2] t025: amended changes after suggestions from the community
  2020-01-18  8:33           ` [PATCH 0/3] t6025: updating tests Shourya Shukla
                               ` (2 preceding siblings ...)
  2020-01-18  8:33             ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla
@ 2020-01-18  8:33             ` Shourya Shukla
  2020-01-18  8:33             ` [PATCH v3 1/2] t6025: modernize style Shourya Shukla
  2020-01-18  8:33             ` [PATCH v3 2/2] t6025: use helpers to replace test -f <path> Shourya Shukla
  5 siblings, 0 replies; 24+ messages in thread
From: Shourya Shukla @ 2020-01-18  8:33 UTC (permalink / raw)
  To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo

Greetings everyone!

After some advise from Eric Sunshine, I have removed the commit to replace pipes
with redirection operators. Also, as Junio Habano and Eric Sunshine pointed out,
I made a small mistake while committing changes in my first commit and it has been
corrected as well.

Thanks,
Shourya Shukla

Shourya Shukla (2):
  t6025: modernize style
  t6025: use helpers to replace test -f <path>

 t/t6025-merge-symlinks.sh | 95 ++++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 47 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v3 1/2] t6025: modernize style
  2020-01-18  8:33           ` [PATCH 0/3] t6025: updating tests Shourya Shukla
                               ` (3 preceding siblings ...)
  2020-01-18  8:33             ` [PATCH v3 0/2] t025: amended changes after suggestions from the community Shourya Shukla
@ 2020-01-18  8:33             ` Shourya Shukla
  2020-01-21 21:57               ` Junio C Hamano
  2020-01-18  8:33             ` [PATCH v3 2/2] t6025: use helpers to replace test -f <path> Shourya Shukla
  5 siblings, 1 reply; 24+ messages in thread
From: Shourya Shukla @ 2020-01-18  8:33 UTC (permalink / raw)
  To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo

The tests in `t6025-merge-symlinks.sh` were written a long time ago, and
has a lot of style violations, including the mixed-use of tabs and spaces,
missing indentations, and other shell script style violations. Update it to
match the CodingGuidelines.

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t6025-merge-symlinks.sh | 95 ++++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 47 deletions(-)

diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
index 433c4de08f..d257dcf34d 100755
--- a/t/t6025-merge-symlinks.sh
+++ b/t/t6025-merge-symlinks.sh
@@ -10,52 +10,53 @@ if core.symlinks is false.'
 
 . ./test-lib.sh
 
-test_expect_success \
-'setup' '
-git config core.symlinks false &&
-> file &&
-git add file &&
-git commit -m initial &&
-git branch b-symlink &&
-git branch b-file &&
-l=$(printf file | git hash-object -t blob -w --stdin) &&
-echo "120000 $l	symlink" | git update-index --index-info &&
-git commit -m master &&
-git checkout b-symlink &&
-l=$(printf file-different | git hash-object -t blob -w --stdin) &&
-echo "120000 $l	symlink" | git update-index --index-info &&
-git commit -m b-symlink &&
-git checkout b-file &&
-echo plain-file > symlink &&
-git add symlink &&
-git commit -m b-file'
-
-test_expect_success \
-'merge master into b-symlink, which has a different symbolic link' '
-git checkout b-symlink &&
-test_must_fail git merge master'
-
-test_expect_success \
-'the merge result must be a file' '
-test -f symlink'
-
-test_expect_success \
-'merge master into b-file, which has a file instead of a symbolic link' '
-git reset --hard && git checkout b-file &&
-test_must_fail git merge master'
-
-test_expect_success \
-'the merge result must be a file' '
-test -f symlink'
-
-test_expect_success \
-'merge b-file, which has a file instead of a symbolic link, into master' '
-git reset --hard &&
-git checkout master &&
-test_must_fail git merge b-file'
-
-test_expect_success \
-'the merge result must be a file' '
-test -f symlink'
+test_expect_success 'setup' '
+	git config core.symlinks false &&
+	>file &&
+	git add file &&
+	git commit -m initial &&
+	git branch b-symlink &&
+	git branch b-file &&
+	l=$(printf file | git hash-object -t blob -w --stdin) &&
+	echo "120000 $l	symlink" | git update-index --index-info &&
+	git commit -m master &&
+	git checkout b-symlink &&
+	l=$(printf file-different | git hash-object -t blob -w --stdin) &&
+	echo "120000 $l	symlink" | git update-index --index-info &&
+	git commit -m b-symlink &&
+	git checkout b-file &&
+	echo plain-file >symlink &&
+	git add symlink &&
+	git commit -m b-file
+'
+
+test_expect_success 'merge master into b-symlink, which has a different symbolic link' '
+	git checkout b-symlink &&
+	test_must_fail git merge master
+'
+
+test_expect_success 'the merge result must be a file' '
+	test -f symlink
+'
+
+test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' '
+	git reset --hard &&
+	git checkout b-file &&
+	test_must_fail git merge master
+'
+
+test_expect_success 'the merge result must be a file' '
+	test -f symlink
+'
+
+test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' '
+	git reset --hard &&
+	git checkout master &&
+	test_must_fail git merge b-file
+'
+
+test_expect_success 'the merge result must be a file' '
+	test -f symlink
+'
 
 test_done
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v3 2/2] t6025: use helpers to replace test -f <path>
  2020-01-18  8:33           ` [PATCH 0/3] t6025: updating tests Shourya Shukla
                               ` (4 preceding siblings ...)
  2020-01-18  8:33             ` [PATCH v3 1/2] t6025: modernize style Shourya Shukla
@ 2020-01-18  8:33             ` Shourya Shukla
  2020-01-21 21:58               ` Junio C Hamano
  5 siblings, 1 reply; 24+ messages in thread
From: Shourya Shukla @ 2020-01-18  8:33 UTC (permalink / raw)
  To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo

Take advantage of helper function 'test_path_is_file()' to
replace 'test -f' since the function makes the code more
readable and gives better error messages.

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t6025-merge-symlinks.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
index d257dcf34d..6c0a90d044 100755
--- a/t/t6025-merge-symlinks.sh
+++ b/t/t6025-merge-symlinks.sh
@@ -36,7 +36,7 @@ test_expect_success 'merge master into b-symlink, which has a different symbolic
 '
 
 test_expect_success 'the merge result must be a file' '
-	test -f symlink
+	test_path_is_file symlink
 '
 
 test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' '
@@ -46,7 +46,7 @@ test_expect_success 'merge master into b-file, which has a file instead of a sym
 '
 
 test_expect_success 'the merge result must be a file' '
-	test -f symlink
+	test_path_is_file symlink
 '
 
 test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' '
@@ -56,7 +56,7 @@ test_expect_success 'merge b-file, which has a file instead of a symbolic link,
 '
 
 test_expect_success 'the merge result must be a file' '
-	test -f symlink
+	test_path_is_file symlink
 '
 
 test_done
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v3 1/2] t6025: modernize style
  2020-01-18  8:33             ` [PATCH v3 1/2] t6025: modernize style Shourya Shukla
@ 2020-01-21 21:57               ` Junio C Hamano
  0 siblings, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2020-01-21 21:57 UTC (permalink / raw)
  To: Shourya Shukla; +Cc: sunshine, git, johannes.schindelin

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> The tests in `t6025-merge-symlinks.sh` were written a long time ago, and
> has a lot of style violations, including the mixed-use of tabs and spaces,
> missing indentations, and other shell script style violations. Update it to
> match the CodingGuidelines.

Looks good.  Will queue.

Thanks.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v3 2/2] t6025: use helpers to replace test -f <path>
  2020-01-18  8:33             ` [PATCH v3 2/2] t6025: use helpers to replace test -f <path> Shourya Shukla
@ 2020-01-21 21:58               ` Junio C Hamano
  0 siblings, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2020-01-21 21:58 UTC (permalink / raw)
  To: Shourya Shukla; +Cc: sunshine, git, johannes.schindelin

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> Take advantage of helper function 'test_path_is_file()' to
> replace 'test -f' since the function makes the code more
> readable and gives better error messages.

Looks good.  Thanks.

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2020-01-21 21:58 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-16 20:36 [PATCH 0/3] t6025: updating tests Shourya Shukla
2020-01-16 20:36 ` [PATCH 1/3] t6025: modernize style Shourya Shukla
2020-01-16 21:42   ` Johannes Schindelin
2020-01-16 20:36 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla
2020-01-16 22:57   ` Junio C Hamano
2020-01-16 20:36 ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla
2020-01-16 22:58   ` Junio C Hamano
2020-01-17 20:44     ` [PATCH 0/3] t6025: amended changes after suggestions from the community Shourya Shukla
2020-01-17 20:44       ` [PATCH 1/3] t6025: modernize style Shourya Shukla
2020-01-17 21:15         ` Eric Sunshine
2020-01-17 21:28           ` Junio C Hamano
2020-01-17 20:44       ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla
2020-01-17 21:24         ` Eric Sunshine
2020-01-18  8:33           ` [PATCH 0/3] t6025: updating tests Shourya Shukla
2020-01-18  8:33             ` [PATCH 1/3] t6025: modernize style Shourya Shukla
2020-01-18  8:33             ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla
2020-01-18  8:33             ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla
2020-01-18  8:33             ` [PATCH v3 0/2] t025: amended changes after suggestions from the community Shourya Shukla
2020-01-18  8:33             ` [PATCH v3 1/2] t6025: modernize style Shourya Shukla
2020-01-21 21:57               ` Junio C Hamano
2020-01-18  8:33             ` [PATCH v3 2/2] t6025: use helpers to replace test -f <path> Shourya Shukla
2020-01-21 21:58               ` Junio C Hamano
2020-01-17 20:44       ` [PATCH 3/3] " Shourya Shukla
2020-01-16 21:46 ` [PATCH 0/3] t6025: updating tests Johannes Schindelin

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