From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 25 Jan 2013 20:06:39 GMT
Subject: Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
Message-ID: <7vwqv12fjk.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130125195446.GA7498@serenity.lan>

John Keeping <john@keeping.me.uk> writes:

>> +		tool="$(basename "$i")"
>
> Quotes are unnecessary here.

Yeah, the outer quotes aren't needed; the inner ones are.

>> +		if test "$tool" = "defaults"
>> +		then
>> +			continue
>> +		elif merge_mode && ! can_merge
>> +		then
>> +			continue
>> +		elif diff_mode && ! can_diff
>> +		then
>> +			continue
>> +		fi
>
> Would this be better as:
>
>     test "$tool" = "defaults" && continue
>
>     can_merge || ! merge_mode || continue
>     can_diff || ! diff_mode || continue
>
> or is that a bit too concise?

It is beyond "too concise"; it is unreadable, and more importantly,
the latter two lines are illogical (why do you even ask if it can be
used for merging, before asking merge_mode to see if the answer to
that question matters to you?)