Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning
- From
David Turner <dturner@twopensource.com>
- Date
- Apr 28, 2016, 17:48 UTC
- Message-ID
- <1461865718.4123.4.camel@twopensource.com>
- In-Reply-To
- <xmqqvb327nyz.fsf@gitster.mtv.corp.google.com>
On Wed, 2016-04-27 at 14:15 -0700, Junio C Hamano wrote:
Show 31 quoted lines
> 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 ;-)
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.