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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 26, 2009, 17:23 UTC
Message-ID: <7vab3uucxi.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4A445CB0.4010506@gmail.com>

Stephen Boyd <bebarino@gmail.com> writes:

Show 24 quoted lines

> 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.