Re: [PATCH 1/7] files_transaction_prepare(): don't leak flags to packed transaction

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Nov 5, 2017, 06:46 UTC
Message-ID: <4d2e6a29-e612-2eab-1b20-0c97a8871c5c@alum.mit.edu>
In-Reply-To: <xmqq60ax6y95.fsf@gitster.mtv.corp.google.com>

On 10/30/2017 05:44 AM, Junio C Hamano wrote:

Show 43 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> The files backend uses `ref_update::flags` for several internal flags.
>> But those flags have no meaning to the packed backend. So when adding
>> updates for the packed-refs transaction, only use flags that make
>> sense to the packed backend.
>>
>> `REF_NODEREF` is part of the public interface, and it's logically what
>> we want, so include it. In fact it is actually ignored by the packed
>> backend (which doesn't support symbolic references), but that's its
>> own business.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  refs/files-backend.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 2bd54e11ae..fadf1036d3 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -2594,8 +2594,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,
>>  
>>  			ref_transaction_add_update(
>>  					packed_transaction, update->refname,
>> -					update->flags & ~REF_HAVE_OLD,
>> -					&update->new_oid, &update->old_oid,
>> +					REF_HAVE_NEW | REF_NODEREF,
>> +					&update->new_oid, NULL,
> 
> Hmph, so we earlier passed all flags except HAVE_OLD down, which
> meant that update->flags that this transaction for packed backend
> does not have to see are given to it nevertheless.  The new way the
> parameter is prepared does nto depend on update->flags at all, so
> that is about "don't leak flags".
> 
> That much I can understand.  But it is not explained why (1) we do
> not pass old_oid anymore and (2) we do give HAVE_NEW.  
> 
> Presumably the justification for (1) is something like "because we
> are not passing HAVE_OLD, we shouldn't have been passing old_oid at
> all---it was a harmless bug because lack of HAVE_OLD made the callee
> ignore old_oid"

It's debatable whether the old code should even be called a bug. The callee is documented to ignore `old_oid` if `HAVE_OLD` is not set. But it was certainly misleading to pass unneeded information to the function.

>                     (2) is "we need to pass HAVE_NEW, and we have
> been always passing HAVE_NEW because update->flags at this point is
> guaranteed to have it" or something like that?

Correct. `REF_DELETING` is set by `lock_ref_for_update()` only if `update->flags & REF_HAVE_NEW` was set, and this code is only called if `REF_DELETING` is set.

Michael