On 1/6/2021 3:05 AM, Junio C Hamano wrote:
> 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.
Ok. That works for me.
>> 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
> '
I can add a comment, but keep in mind that this example would run the
subcommand as "git false". This isn't intended as an arbitrary script
runner, but a "please run the same Git command on a list of repos".
Thanks,
-Stolee
