Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

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

Two things I forgot to say.

Michael Haggerty <mhagger@alum.mit.edu> writes:

> I was thinking of this patch as documenting and enforcing a limitation
> in the current implementation of pruning.

This actually is an excellent point, and is the reason why I
repeatedly made my suggestion conditional on "If my understanding of
the purpose of this patch correctly" ;-) And I personally am not
convinced that this limitation is fundamental and will not be
lifted.  It may be better to treat this as a limitation of the
current implementation of pruning, and in that case I do agree that
the original patch that started this thread is the best way to
express it.

I said that it makes harder to write incorrect client code of this
API to make REF_ISPRUNING contain both 0x04 and REF_NODEREF, and I
still think that is true, but at the same time, it makes it easier
to write incorrect code in the API implementation.  We no longer can
check if we are in the pruning codepath with

    if (flag & REF_ISPRUNING)

but have to write

    if ((flag & REF_ISPRUNING) == REF_ISPRUNING)

instead.  This "& with the constant and compare the result with that
same constant" pattern could also be used for a single-bit constant,
so if there were short-and-sweet syntax to express that pattern in
the language, consistently using that for all bit checks would make
it less likely for us to write incorrect code, but but there is no
short-and-sweet syntax to do so in C, and spelling it out like the
above is too cumbersome to be practical.  The suggested squash did

    if (flag & REF_ISPRUNING_)

to check only for 0x04 bit, but that is also error prone; it is too
easy to forget the difference between the two and drop the trailing
underscore by mistake.

Even though it is generally a good trade-off to make it harder to
make mistakes in the "user of the API" code even if it makes it
easier to make mistakes in the "implementation of the API" code,
because the "user of the API" in this case is actually still inside
the ref API implementation, I do not think either way makes too much
of a difference.  So perhaps your original might be the best version
among those that have been discussed in this thread.