Junio C Hamano <gitster@xxxxxxxxx> writes:
> 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.
Yeah fair enough, we are deep into it and might and makes sense to make
it more consistent. I'll incorporate your suggestion and send in a new
version.
Attachment:
signature.asc
Description: PGP signature
