Karthik Nayak <karthik.188@xxxxxxxxx> 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.
