Re: [PATCH v3 3/4] fetch-pack: expose fsckObjects configuration logic

From: Junio C Hamano <gitster@pobox.com>
Date: May 28, 2024, 17:24 UTC
Message-ID: <xmqqbk4p6e98.fsf@gitster.g>
In-Reply-To: <xmqqikyx6ew9.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 31 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
>
>>> +int fetch_pack_fsck_objects(void)
>>> +{
>>> +	fetch_pack_setup();
>>> +
>>> +	return fetch_fsck_objects >= 0
>>> +	       ? fetch_fsck_objects
>>> +	       : transfer_fsck_objects >= 0
>>> +	       ? transfer_fsck_objects
>>> +	       : 0;
>>> +}
>>
>> ... can we maybe rewrite it to something more customary here? The
>> following is way easier to read, at least for me.
>>
>> 	int fetch_pack_fsck_objects(void)
>> 	{
>> 		fetch_pack_setup();
>> 		if (fetch_fsck_objects >= 0 ||
>> 		    transfer_fsck_objects >= 0)
>> 			return 1;
>> 		return 0;
>> 	}
>
> But do they mean the same thing?  In a repository where
>
>     [fetch] fsckobjects = no
>
> is set, no matter what transfer.fsckobjects says (or left unspecified),
> we want to return "no, we are not doing fsck".

The original before it was made into a helper function was written as a cascade of ?: operators, because it had to be a single expression. As the body of a helper function, we now can sprinkle multiple return statements in it. I think the way that is easiest to understand is

	/* the most specific, if specified */
	if (fetch_fsck_objects >= 0)
		return fetch_fsck_objects;
	/* the less specific, catch-all for both directions */
	if (transfer_fsck_objects >= 0)
        	return transfer_fsck_objects;
	/* the fallback hardcoded default */
	return 0;

without the /* comments */.