On 04/27/2016 11:15 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html