On 10/30/2017 05:44 AM, Junio C Hamano wrote: > Michael Haggerty <mhag...@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 <mhag...@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
