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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 30, 2017, 04:44 UTC
Message-ID: <xmqq60ax6y95.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <b8e61183ad5d5c046a9eff75ea0401cf2c2567c6.1509183413.git.mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 27 quoted lines

> 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" 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);
>  		}
>  	}