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

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

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

> But I think it would be cleaner to achieve that goal with the following
> change:
>
> diff --git a/refs.c b/refs.c
> index 5dc2473..1d4c12a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -790,8 +790,10 @@ int ref_transaction_update(struct ref_transaction
> *transaction,
>  	if (transaction->state != REF_TRANSACTION_OPEN)
>  		die("BUG: update called for transaction that is not open");
>
> -	if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
> -		die("BUG: REF_ISPRUNING set without REF_NODEREF");
> +	if (flags & REF_ISPRUNING) {
> +		/* Pruning is always non-recursive */
> +		flags |= REF_NODEREF;
> +	}
>
>  	if (new_sha1 && !is_null_sha1(new_sha1) &&
>  	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 8fcbd7d..9faf17c 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2116,7 +2116,7 @@ static void prune_ref(struct ref_to_prune *r)
>  	transaction = ref_transaction_begin(&err);
>  	if (!transaction ||
>  	    ref_transaction_delete(transaction, r->name, r->sha1,
> -				   REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
> +				   REF_ISPRUNING, NULL, &err) ||
>  	    ref_transaction_commit(transaction, &err)) {
>  		ref_transaction_free(transaction);
>  		error("%s", err.buf);
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 37a1a37..704eea7 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -15,7 +15,7 @@
>
>  /*
>   * Used as a flag in ref_update::flags when a loose ref is being
> - * pruned.
> + * pruned. This flag implies REF_NODEREF.
>   */
>  #define REF_ISPRUNING	0x04
>
>
> Note that patch "add_update(): initialize the whole ref_update" should
> then be adjusted to do the flag-tweak in the add_update() function.
> ...
> If there are no objections, I will implement these changes in v2.

One worrysome point the above approach leaves is that nothing
guarantees that nobody in the codepath from the callsite of
ref-transaction-delete to ref-transaction-update looks at the flag
and acts differently depending on REF_NODEREF bit.  During that
time, REF_ISPRUNING call would not trigger (flag & REF_NODEREF)
check, because "pruning implies no-deref" is done only after you
reach transaction-update (i.e. the code you added in the first
hunk), but any code after that "implie no-deref" happens will see
REF_NODEREF bit in the flag word.

As long as you can add some mechanism to make that a non-issue, I am
fine with that approach.

Thanks.