Re: [PATCHv3 2/2] read-tree: migrate to parse-options

[Junio C Hamano <gitster@pobox.com>]

Stephen Boyd <bebarino@gmail.com> writes:

> Sorry I went a little overboard with s/:1// on unpack_tree_options.
> You'll probably want to squash this on top.
>
> diff --git a/unpack-trees.h b/unpack-trees.h
> index d19df44..f344679 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -21,14 +21,14 @@ struct unpack_trees_options {
>                      merge,
>                      update,
>                      index_only,
> -                    nontrivial_merge,
> +                    nontrivial_merge:1,
>                      trivial_merges_only,
>                      verbose_update,
>                      aggressive,
> -                    skip_unmerged,
> -                    initial_checkout,
> -                    diff_index_cached,
> -                    gently;
> +                    skip_unmerged:1,
> +                    initial_checkout:1,
> +                    diff_index_cached:1,
> +                    gently:1;

Let's look at this (not this follow-up patch) the other way around.

Six months down the load, somebody may ask you:

    Is there a good reason why many are not bitfields but only selected
    few are bitfields in this structure?  Most of these can and should be
    bitfields, as far as I can see, because the code uses them as
    booleans, and the only breakage it may cause if we change them to
    bitfields to shrink this structure would be the option parsing code.

What would be your answer?  Doesn't it feel wrong to do such a conversion
only to work around the current limitation of parseopt?

By the way, has it been verified that all the users of these fields are OK
with this change when they use these fields?  I am not worried about them
reading the value command line option parser set, but more worried about
reading after other codepaths set/modified these fields.  The command line
parser that uses parseopt may correctly set only 0 or 1 to these fields
initially and we should be able to verify that from the patch text, but
there is no guarantee that this conversion is even correct at runtime
without an audit, no?

The callers have long relied on the fact that reading from these bitfields
yields either 0 or 1 and never 2 or larger, but they are now widened to
full-width unsigned.  A pattern like this:

	uto.field = ~uto.field;
        if (uto.field == 1) {
        	field now set;
	} else {
        	field now unset;
	}

would be broken by widening "unsigned field:1" to "unsigned field", right?
I am not saying this is the only pattern that would break, nor I know
there are codepaths that use this pattern, but I think you got my point.