From: David Turner <dturner@twopensource.com>
Date: Thu, 28 Apr 2016 17:48:38 GMT
Subject: Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning
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:
> 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.