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

From: Michael Haggerty <mhagger@alum.mit.edu>
To: Junio C Hamano <gitster@pobox.com>,
	David Turner <dturner@twopensource.com>
Cc: git@vger.kernel.org, "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Jeff King" <peff@peff.net>,
	"Ramsay Jones" <ramsay@ramsayjones.plus.com>
Subject: Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning
Date: Fri, 29 Apr 2016 08:56:28 +0200	[thread overview]
Message-ID: <5723059C.5080406@alum.mit.edu> (raw)
In-Reply-To: <xmqqvb327nyz.fsf@gitster.mtv.corp.google.com>

On 04/27/2016 11:15 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> 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 ;-)

I was thinking of this patch as documenting and enforcing a limitation
in the current implementation of pruning. But to be honest I can't think
of a reason that we would ever want to remove this limitation, so I am
OK with changing the policy to "REF_ISPRUNING always implies
REF_NODEREF" as you have suggested.

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.

Michael
