Re: [PATCH v2 3/5] commit-graph: create new version flags

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 25, 2019, 05:29 UTC
Message-ID: <xmqqftq6y7nv.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <4ddb82916321f093d88282ef4bcab99993339fb5.1556135881.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 11 quoted lines

> +	int version = 0;
> ...
> +	if (flags & COMMIT_GRAPH_VERSION_1)
> +		version = 1;
> +	if (!version)
> +		version = 1;
> +	if (version != 1) {
> +		error(_("unsupported commit-graph version %d"),
> +		      version);
> +		return 1;
> +	}

The above sequence had a certain "Huh?" factor before 5/5 introduced the support for a later version that is in use by default.

Is it sensible to define VERSION_$N as if they are independent bits in a single flags variable? What does it mean for the flags variable to have both GRAPH_VERSION_1 and GRAPH_VERSION_2 bits set?

What I am getting at is if this is better done as a n-bit bitfield that represents a small unsigned integer (e.g. "unsigned char" that lets you play with up to 255 versions, or "unsigned version : 3" that limits you to up to 7 versions).

You use an 8-bit byte in the file format anyway, so it might not be so bad to have a separate version parameter that is not mixed with the flag bits, perhaps?