Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 29, 2016, 08:19 UTC
- Message-ID
- <xmqq4mak3k08.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <5723059C.5080406@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 52 quoted lines
> 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.