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

On 1/6/2021 3:05 AM, Junio C Hamano wrote:

Derrick Stolee [off-list ref] writes:

quoted

quoted

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.

quoted

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