Re: [PATCH 09/11] read-cache: require flags for `write_locked_index()`

From: Martin Ågren <martin.agren@gmail.com>
Date: Oct 2, 2017, 04:14 UTC
Message-ID: <CAN0heSqPVpgYSfUpmiBaCVtZkMiqWC3rgzFcBu2TVr3m7Gq4UA@mail.gmail.com>
In-Reply-To: <xmqqd166qkeg.fsf@gitster.mtv.corp.google.com>

On 2 October 2017 at 05:49, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Martin Ågren <martin.agren@gmail.com> writes:
>
>> ... Instead, require that one of the
>> flags is set. Adjust documentation and the assert we already have for
>> checking that we don't have too many flags. Add a macro `HAS_SINGLE_BIT`
>> (inspired by `HAS_MULTI_BITS`) to simplify this check and similar checks
>> in the future.
>
> I do not have a strong opinion against this approach, but if
> something can take only one of two values, wouldn't it make more
> sense to express it as a single boolean, I wonder.  Then there is no
> need to invent a cute HAS_SINGLE_BIT() macro, either.
>
> "commit and leave it open" cannot be expressed with such a scheme,
> but with the HAS_SINGLE_BIT() scheme it can't anyway, so...

I did briefly consider renaming `flags` to `commit` and re-#defining the two flags to 0 and 1 (or even updating all the callers to use literal zeros and ones). It felt a bit awkward to downgrade `flags` to a bool -- normally we'd to the reverse change. But maybe I shouldn't have rejected that so easily. If we have a feeling we won't need other flags (or the "don't even close the file") any time soon, maybe it'd be good to tighten things up a bit. Thanks for looking at these.