David Turner <dturner@xxxxxxxxxxxxxxxx> writes:
> On Wed, 2016-04-27 at 11:47 -0700, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@xxxxxxxxxxxx> writes:
>>
>> > It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING
>> > without REF_NODEREF. Forbid it explicitly. Change the one
>> > REF_ISPRUNING
>> > caller to pass REF_NODEREF too.
>> >
>> > Signed-off-by: Michael Haggerty <mhagger@xxxxxxxxxxxx>
>> > ---
>> > This also makes later patches a bit clearer.
>>
>> I wonder if it is more future-proof to solve this by doing
>>
>> -#define REF_ISPRUNING 0x04
>> +#define REF_ISPRUNING (0x04 | REF_NODEREF)
>>
>> instead. It makes the intention clear that pruning is always about
>> the single level (i.e. no-deref).
>
> I think the approach in Michael's patch might be clearer than yours,
> since someone reading the code doesn't have to look at the definition
> of REF_ISPRUNING to understand what is going on.
I have to strongly disagree, assuming that my understanding of the
point of this patch correctly.
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.
--
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