Re: [PATCH v2 3/5] commit-graph: create new version flags
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Apr 25, 2019, 11:09 UTC
- Message-ID
- <eb1ec575-601c-d50e-dfd1-beab1debcc96@gmail.com>
- In-Reply-To
- <xmqqftq6y7nv.fsf@gitster-ct.c.googlers.com>
On 4/25/2019 1:29 AM, Junio C Hamano wrote:
Show 29 quoted lines
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> + 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?This is a reasonable idea, as this is a "pick exactly one" option. It is still important to reduce the overall parameter count by combining the other boolean options into flags.
Thanks, -Stolee