Derrick Stolee <stolee@xxxxxxxxx> writes:
>> Probably not worth a re-roll, but the above has higher cognitive load than:
>>
>> if (!value)
>> return 0;
>>
>> which indicates clearly that the command succeeded, whereas `return
>> result` makes the reader scan all the code above the conditional to
>> figure out what values `result` could possibly hold.
>
> True. Looking at this again, it might be better to just update the
> loop to be
>
> for (i = 0; values && i < values->nr; i++) {
>
> which would run the loop zero times when there are zero results.
It is misleading, though. It forces readers to first assume that
the loop body may update "values" from non-NULL to NULL in some
condition and hunt for such a code in vain.
If some clean-up starts to become needed after the loop, the "if the
value array is empty, immediately return 0" may have to be rewritten
to "if empty, jump to the clean-up part after the loop", but until
then, what Eric gave us would be the easiest to read, I would think.
> To me, "nonexistent" or "does not exist" doesn't adequately describe
> this (hypothetical) situation.
>
> Perhaps "fail-subcommand" might be better?
I wonder if "false" or "exit 1" would fit the bill. In any case, a
comment may help, perhaps?
test_expect_success 'do nothing and succeed on empty/missing config' '
# if this runs even once, "false" ensures a failure
git for-each-repo --config=bogus.config -- false
'
