Re: [PATCH v2 1/4] t7401: modernize style

[Shourya Shukla <shouryashukla.oo@gmail.com>]

On 13/08 09:46, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> >> @@ -16,12 +16,13 @@ add_file () {
> >>  	owd=$(pwd)
> >>  	cd "$sm"
> >>  	for name; do
> >> -		echo "$name" > "$name" &&
> >> +		echo "$name" >"$name" &&
> >>  		git add "$name" &&
> >>  		test_tick &&
> >>  		git commit -m "Add $name"
> >>  	done >/dev/null
> >> -	git rev-parse --verify HEAD | cut -c1-7
> >> +	git rev-parse --verify HEAD >out &&
> >> +	cut -c1-7 out
> >
> > In any case, I believe we can avoid the 'cut' altogether in both places
> > by doing something like this instead:
> >
> >    git rev-parse --short=7 HEAD
> 
> Ah, I missed the fact that this was a helper function and most of
> the error status is discarded anyway.  For example, we still run the
> rev-parse even after the for loop fails.
> 
> If the focus of this test script were to ensure that rev-parse works
> correctly, being careful to catch its exit status might have had a
> good value, but for that, all the other operations that happen in
> this helper function (including the "what happens when the loop body
> fails for $name that is not at the end of the argument list?") must
> also be checked for their exit status in the first place.
> 
> Since that is not done, and since testing rev-parse should not have
> to be part of the job for submodule test, the net effect of the
> above change has quite dubious value---it clobbered a file 'out'
> that may be used by the caller.
> 
> Doing "cd" without introducing a subshell is a bit harder to fix, as
> test_tick relies on the global counter in the topmost process.  It
> can be done, but I do not think it is worth doing here.  Most of the
> users of this helper function call it in var=$(add_file ...)
> subshell anyway (so test_tick is incrementing the timestamp
> independently for each caller and discarding the resulting
> timestamp).  As a NEEDSWORK comment added in the series says, this
> script may need a bit more work.
> 
> I agree with you that the split of "rev-parse | cut -c1-7" into two
> statements and clobbering 'out' is a bad change---that part should
> be reverted.  The style change on 'echo "$name" >"$name"' line is
> OK, though.
> 
> Thanks.

Understood. I will revert the change. Though, what Kaartic suggested, to
do a '--short=7', that will be okay to keep right? Something like:

    git rev-parse --short=7 HEAD

This way we will not need a 'cut'. This change goes as a separate commit
obviously.