[0/3] t6025: updating tests

Message

Shourya Shukla Jan. 16, 2020, 8:36 p.m. UTC

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

Comments

Johannes Schindelin Jan. 16, 2020, 9:46 p.m. UTC | #1

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
>