Junio C Hamano [off-list ref] 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 ;-)
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.
On 04/27/2016 11:15 PM, 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 ;-)
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