Re: [PATCH v2 4/6] parse-options.c: use optbug() instead of BUG() "opts" check

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 1, 2022, 18:55 UTC
Message-ID: <xmqqczfsrz6f.fsf@gitster.g>
In-Reply-To: <YpZSrNM/bZ3i6Yvu@google.com>

Josh Steadmon <steadmon@google.com> writes:

Show 14 quoted lines

>> +			if (!opts->ll_callback) {
>> +				optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs a callback");
>> +				break;
>> +			}
>> +			if (opts->callback) {
>> +				optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs no high level callback");
>> +				break;
>> +			}
>>  			break;
>
> A minor point, but I'm not sure I understand why we're adding breaks for
> the two cases above. In the OPTION_CALLBACK case, the if conditions are
> mutually exclusive and are followed by an unconditional break, so adding
> additional breaks seems unnecessary.

Yeah, good thinking.

> For the OPTION_LOWLEVEL_CALLBACK
> case, the conditions are not mutually exclusive, but isn't this exactly
> the issue that optbug() is intended to address? I.e., wouldn't the
> caller want to see both optbug()s if both are relevant?

Exactly.

Thanks for a careful reading.