threads / patch / 52653

patch, 3 partst6025: updating tests

Subject: [PATCH 0/3] t6025: updating tests

## tl;dr

24 messages between Jan 16, 2020 and Jan 21, 2020. Diffs are folded; open one to read it.

replies: 23people: 4as markdown or json

Shourya Shukla· Jan 16, 2020, 20:36 UTC · lore
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
Shourya Shukla· Jan 16, 2020, 20:36 UTC · re: Shourya Shukla · lore

[PATCH 1/3] t6025: modernize style

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(-)
Show changes to t/t6025-merge-symlinks.sh +50 −47
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
Johannes Schindelin· Jan 16, 2020, 21:42 UTC · re: Shourya Shukla · lore

Re: [PATCH 1/3] t6025: modernize style

Hi,
On Fri, 17 Jan 2020, Shourya Shukla wrote:
Show 7 quoted lines
> 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:
Show 15 quoted lines
>  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...
Show 46 quoted lines
> -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

Show 53 quoted lines
> +	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
>
>
Shourya Shukla· Jan 16, 2020, 20:36 UTC · re: Shourya Shukla · lore

[PATCH 2/3] t6025: replace pipe with redirection operator

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(-)
Show changes to t/t6025-merge-symlinks.sh +4 −4
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
Junio C Hamano· Jan 16, 2020, 22:57 UTC · re: Shourya Shukla · lore

Re: [PATCH 2/3] t6025: replace pipe with redirection operator

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.
Show 10 quoted lines
>  	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 &&
Shourya Shukla· Jan 16, 2020, 20:36 UTC · re: Shourya Shukla · lore

[PATCH 3/3] t6025: use helpers to replace test -f <path>

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(-)
Show changes to t/t6025-merge-symlinks.sh +3 −3
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
Junio C Hamano· Jan 16, 2020, 22:58 UTC · re: Shourya Shukla · lore

Re: [PATCH 3/3] t6025: use helpers to replace test -f <path>

Shourya Shukla <shouryashukla.oo@gmail.com> writes:
Show 8 quoted lines
> 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.
Show 31 quoted lines
> 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
Shourya Shukla· Jan 17, 2020, 20:44 UTC · re: Junio C Hamano · lore

[PATCH 0/3] t6025: amended changes after suggestions from the community

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
Shourya Shukla· Jan 17, 2020, 20:44 UTC · re: Shourya Shukla · lore

[PATCH 1/3] t6025: modernize style

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(-)
Show changes to t/t6025-merge-symlinks.sh +50 −47
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
Eric Sunshine· Jan 17, 2020, 21:15 UTC · re: Shourya Shukla · lore

Re: [PATCH 1/3] t6025: modernize style

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.

Show 19 quoted lines
> 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/
Show 11 quoted lines
> +       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
> +'
Junio C Hamano· Jan 17, 2020, 21:28 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/3] t6025: modernize style

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 15 quoted lines
>> +       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.

Show 6 quoted lines
>> +       git commit -m b-symlink &&
>> +       git checkout b-file &&
>> +       echo plain-file >symlink &&
>> +       git add symlink &&
>> +       git commit -m b-file
>> +'
Shourya Shukla· Jan 17, 2020, 20:44 UTC · re: Shourya Shukla · lore

[PATCH 2/3] t6025: replace pipe with redirection operator

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(-)
Show changes to t/t6025-merge-symlinks.sh +4 −5
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
Eric Sunshine· Jan 17, 2020, 21:24 UTC · re: Shourya Shukla · lore

Re: [PATCH 2/3] t6025: replace pipe with redirection operator

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

Show 8 quoted lines
> 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.)

Shourya Shukla· Jan 18, 2020, 08:33 UTC · re: Eric Sunshine · lore
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
Shourya Shukla· Jan 18, 2020, 08:33 UTC · re: Shourya Shukla · lore

[PATCH 1/3] t6025: modernize style

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(-)
Show changes to t/t6025-merge-symlinks.sh +50 −47
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
Shourya Shukla· Jan 18, 2020, 08:33 UTC · re: Shourya Shukla · lore

[PATCH 2/3] t6025: replace pipe with redirection operator

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(-)
Show changes to t/t6025-merge-symlinks.sh +4 −4
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
Shourya Shukla· Jan 18, 2020, 08:33 UTC · re: Shourya Shukla · lore

[PATCH 3/3] t6025: use helpers to replace test -f <path>

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(-)
Show changes to t/t6025-merge-symlinks.sh +3 −3
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
Shourya Shukla· Jan 18, 2020, 08:33 UTC · re: Shourya Shukla · lore

[PATCH v3 0/2] t025: amended changes after suggestions from the community

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
Shourya Shukla· Jan 18, 2020, 08:33 UTC · re: Shourya Shukla · lore

[PATCH v3 1/2] t6025: modernize style

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(-)
Show changes to t/t6025-merge-symlinks.sh +48 −47
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
Junio C Hamano· Jan 21, 2020, 21:57 UTC · re: Shourya Shukla · lore

Re: [PATCH v3 1/2] t6025: modernize style

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.
Shourya Shukla· Jan 18, 2020, 08:33 UTC · re: Shourya Shukla · lore

[PATCH v3 2/2] t6025: use helpers to replace test -f <path>

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(-)
Show changes to t/t6025-merge-symlinks.sh +3 −3
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
Junio C Hamano· Jan 21, 2020, 21:58 UTC · re: Shourya Shukla · lore

Re: [PATCH v3 2/2] t6025: use helpers to replace test -f <path>

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.
Shourya Shukla· Jan 17, 2020, 20:44 UTC · re: Shourya Shukla · lore

[PATCH 3/3] t6025: use helpers to replace test -f <path>

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(-)
Show changes to t/t6025-merge-symlinks.sh +3 −3
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
Johannes Schindelin· Jan 16, 2020, 21:46 UTC · re: Shourya Shukla · lore

Re: [PATCH 0/3] t6025: updating tests

Hi,
On Fri, 17 Jan 2020, Shourya Shukla wrote:
Show 20 quoted lines
> 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.
Show 5 quoted lines
> 	- 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.
Show 8 quoted lines
>
> 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

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

← back to recent threads