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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 6, 2021, 08:05 UTC
Message-ID: <xmqqa6tmtrr1.fsf@gitster.c.googlers.com>
In-Reply-To: <db7bf751-7541-59b9-a3a0-ec07e28eb9de@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 15 quoted lines

>> 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
	'