Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2025, 17:50 UTC
Message-ID: <xmqqzfjiwxei.fsf@gitster.g>
In-Reply-To: <Z5DgxQuc2j_-5GHg@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 14 quoted lines

>> +static void t_reftable_invalid_limit_updates(void)
>> +{
>> +	struct reftable_ref_record ref = {
>> +		.refname = (char *) "HEAD",
>> +		.update_index = 1,
>> +		.value_type = REFTABLE_REF_SYMREF,
>> +		.value.symref = (char *) "master",
>> +	};
>> +	struct reftable_write_options opts = {
>> +		.default_permissions = 0660,
>> +	};
>
> Nit: it's unnecessary to pass write options. Other than that the test
> looks good to me, and this nit isn't worth a reroll.

This write_options opts is used later in this call.

	err = reftable_new_stack(&st, dir, &opts);

and "git grep reftable_new_stack" finds many hits, almost all in t/unit-tests/ hierarchy, only two among many of them passing NULL as the "use the default set of options" signal. And majority of them initialize their opts like so:

	struct reftable_write_options opts = { 0 };

So I agree that this one should pass NULL to be more explicit that we do not exercise any special features from the API, but so should many other existing callers that pass such meaningless &opts, I would think.

Thanks.