Re: [PATCH-w 101/105] t6300 (for-each-ref): modernize style

From: Tom Grennan <tmgrennan@gmail.com>
Date: Mar 1, 2012, 03:20 UTC
Message-ID: <20120301032053.GD2572@tgrennan-laptop>
In-Reply-To: <7v62epqd9a.fsf@alter.siamese.dyndns.org>

On Wed, Feb 29, 2012 at 06:13:53PM -0800, Junio C Hamano wrote:

Show 8 quoted lines

>Tom Grennan <tmgrennan@gmail.com> writes:
>
>> +if ! test -r test-lib.sh ; then
>> +	(cd ${0%/*} && ./${0##*/} $@)
>> +	exit $?
>> +fi
>
>Not very nice; why is this an improvement?

I liked being able to,
	t/t1234-frotz.sh [-q|-v]
in addition to,
	cd t && sh ./t1234-frotz.sh

Show 7 quoted lines

>>  . ./test-lib.sh
>>  . "$TEST_DIRECTORY"/lib-gpg.sh
>>  
>> +quiet () { "$@" >/dev/null; }
>> +silent () { "$@" >/dev/null 2>&1; }
>
>Not nice, either.

"Not nice" b/c they're on one line? I like simple things compressed so I quickly glance pass them. Sort of forest through the trees.

Show 22 quoted lines

>>  # Mon Jul 3 15:18:43 2006 +0000
>>  datestamp=1151939923
>>  setdate_and_increment () {
>> @@ -22,9 +30,9 @@ test_expect_success 'Create sample commit with known timestamp' '
>>  	setdate_and_increment &&
>>  	echo "Using $datestamp" > one &&
>>  	git add one &&
>> -	git commit -m "Initial" &&
>> +	git commit -q -m "Initial" &&
>>  	setdate_and_increment &&
>> -	git tag -a -m "Tagging at $datestamp" testtag
>> +	quiet git tag -a -m "Tagging at $datestamp" testtag
>
>Why? Why? Why?
>
>	cd t && sh ./t1234-frotz.sh
>
>would be silent enough and suppressing the output from the commands like
>this patch does makes it _harder_ for people to debug their changes to the
>script with
>
>	sh ./t1234-frotz.sh -v

Hmm, I found the noise distracting. For example, w/o redirecting stderr, most of the test_must_fail puke as expected. If it's expected, why show it? BTW since it's a different stream, to get the error near the exec log one has to:

	sh ./t1234-frotz.sh -v |& less

Otherwise it's challenging to separate the expected vs unexpected failures.

Most of the "quiet's" are with git-tag and git-branch. I'd prefer --quiet and --silent options to these commands like git-init, git-commit, etc.

>Most of the change in this patch does nothing to do with "modernize style".

It's trivial to remove these "quiet" and "silent", but to me that's the only value added by these patches. More seriously, the remaining modernization still seems much larger than its value.

-- 
TomG