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

On Wed, 2016-04-27 at 14:15 -0700, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

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.

... but my understanding of the point of this patch may be flawed,
in which case I of course am willing to be enlightened ;-)

Since there is a manual check for that case, the code will fail at test
time.

But I don't have strong feelings and am happy to go either way on this.