Re: [PATCH] for-each-repo: do nothing on empty config

[Derrick Stolee <stolee@gmail.com>]

On 1/6/2021 3:05 AM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> 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