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

On Tue, May 28, 2024 at 10:24:35AM -0700, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

Patrick Steinhardt [off-list ref] writes:

quoted

quoted

+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 */.

Ah, right, didn't see this mail. My revised version looks the same as
yours, except for the added comments.

Patrick