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

David Turner [off-list ref] writes:

On Wed, 2016-04-27 at 11:47 -0700, Junio C Hamano wrote:

quoted

Michael Haggerty [off-list ref] writes:

quoted

It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING
without REF_NODEREF. Forbid it explicitly. Change the one
REF_ISPRUNING
caller to pass REF_NODEREF too.

Signed-off-by: Michael Haggerty <redacted>
---
This also makes later patches a bit clearer.

I wonder if it is more future-proof to solve this by doing

    -#define REF_ISPRUNING	0x04
    +#define REF_ISPRUNING	(0x04 | REF_NODEREF)

instead.  It makes the intention clear that pruning is always about
the single level (i.e. no-deref).

I think the approach in Michael's patch might be clearer than yours,
since someone reading the code doesn't have to look at the definition
of REF_ISPRUNING to understand what is going on.

I have to strongly disagree, assuming that my understanding of the
point of this patch correctly.

If a casual reader sees this code:

    ref_transaction_delete(transaction, r->name, r->sha1,
			   REF_ISPRUNING | REF_NODEREF, NULL, &err)

it gives an incorrect impression that there may also be a valid case
to make a "delete" call with ISPRUNING alone without NODEREF, in
other codepaths and under certain conditions, and write an incorrect

    ref_transaction_delete(transaction, refname, sha1,
			   REF_ISPRUNING, NULL, &err)

in her new code.  Or a careless programmer and reviewer may not even
memorize and remember what the new world order is when they see such
a code and let it pass.

As I understand that we declare that "to prune a ref from set of
loose refs is to prune the named one, never following a symbolic
ref" is the new world order with this patch, making sure that
ISPRUNING automatically and always mean NODEREF will eliminate the
possibility that any new code makes an incorrect call to "delete",
which I think is much better.