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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 16, 2020, 21:46 UTC
Message-ID: <nycvar.QRO.7.76.6.2001162243100.46@tvgsbejvaqbjf.bet>
In-Reply-To: <20200116203622.4694-1-shouryashukla.oo@gmail.com>

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
>