Re: [PATCH v5 5/6] check-whitespace: detect if no base_commit is provided

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Thanks Junio for explaining with examples, really nice of you! I'm on
>> the fence with this, even the existing change from the previous more
>> verbose code. I know this is shorter, but it is always more readable to
>> use the longer version with 'test'. I find it hard to remember the
>> specifics.
>
> You'd never remember unless you practice, but it boils down to one
> question: is it reasonable to expect that most developers who need
> to touch this code find it worth to learn to read and write shell
> scripts well in this day and age?  The answer is probably no.
>
> As you may remember, this R=${A-${B?}} dance started at
>
>   https://lore.kernel.org/git/xmqqwmlpb8er.fsf@gitster.g/
>
> where I said:
>
>     ...
>     in a separate "after the dust settles" clean-up #leftoverbits topic.
>
>     We could replace the first 7 lines with a single-liner
>
>        R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
>
>     if we wanted to, but all of that will be mere clean-up changes.
>
> Even the longhand to set a single R with if/elif cascade so that we
> can have a single location that invokes ci/run-style-check.sh was
> considered extra clean-up for #leftoverbits at least by me.
>
> But after seeing you used the ${A-${B?}} dance, which is more
> advanced than the #leftoverbits clean-up, I thought you were
> interested in using such a construct that pursues parameter
> expansion mastery, and that was the primary reason why the
> demonstration in the message you are responding to was added.
>
> I personally do not care too deeply which one to use wrt the
> readability, but
>
> 	R=${A-${B?}}
> 	if test -z "$R"
> 	then
> 		error
> 	fi
>
> looks strange and inconsistent by spreading the error check to two
> places.  The code would be better off if it were
>
> 	R=${A-$B}
> 	if test -z "$R"
> 	then
> 		error
> 	fi
>
> (or with R=${A:-$B}) instead.  Then it makes it clear that the
> author wanted to take care of the error case with the if part.