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

[Junio C Hamano]

Michael Haggerty  writes:

>> 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.

It is is extra nice for you to give answers that are customized for
me like the above to my questions, but the questions came because
the log message did not answer them, so please make sure the next
person who did not read this exchange but reads only the resulting
commit does not have to ask the same question.

Thanks.

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

[Michael Haggerty]

On 10/30/2017 05:44 AM, Junio C Hamano wrote:
> Michael Haggerty  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 
>> ---
>>  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,
>> ->new_oid, >old_oid,
>> +REF_HAVE_NEW | REF_NODEREF,
>> +>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

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

[Junio C Hamano]

Michael Haggerty  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 
> ---
>  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,
> - >new_oid, >old_oid,
> + REF_HAVE_NEW | REF_NODEREF,
> + >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" and (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?



>   NULL);
>   }
>   }

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

[Michael Haggerty]

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 
---
 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,
-   >new_oid, >old_oid,
+   REF_HAVE_NEW | REF_NODEREF,
+   >new_oid, NULL,
NULL);
}
}
-- 
2.14.1