On 2 October 2017 at 06:14, Martin Ågren <martin.agren@xxxxxxxxx> wrote: > On 2 October 2017 at 05:49, Junio C Hamano <gitster@xxxxxxxxx> wrote: >> Martin Ågren <martin.agren@xxxxxxxxx> 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. Of course it wouldn't have to be as invasive. It could be "the lock will always be closed and with COMMIT_LOCK, it will also be committed". CLOSE_LOCK would be removed and the few current users of CLOSE_LOCK would be converted to use 0.
