Re: [PATCH v2 5/8] refs: add `committer_info` to `ref_transaction_add_update()`

From: karthik nayak <karthik.188@gmail.com>
Date: Dec 13, 2024, 19:43 UTC
Message-ID: <CAOLa=ZT9KDNaz84DLoRBzY_yaZ3kZ2FUWADohZhM4EcrJgmS+A@mail.gmail.com>
In-Reply-To: <Z1wnbCoidG-lC7ui@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 15 quoted lines

> On Fri, Dec 13, 2024 at 11:36:50AM +0100, Karthik Nayak wrote:
>> The `ref_transaction_add_update()` creates the `ref_update` struct. To
>> facilitate addition of reflogs in the next commit, the function needs to
>> accommodate setting the `committer_info` field in the struct. So modify
>> the function to also take `committer_info` as an argument and set it
>> accordingly.
>
> I was wondering a bit whether we could instead pull out a
> `add_update_internal()` function so that we don't need to modify all
> callers of `ref_transaction_add_update()`. Because ultimately, we don't
> use the field anywhere except from `ref_transaction_add_reflog_update()`
> as far as I can see.
>
> This is more of a thought than a strong opinion, so feel free to ignore.
>

Yes, that is a possible change, but the number of code changes are relatively low and I didn't think it made so much difference. Also because we'd now have one more function. But I don't mind doing it either, if anyone feels strongly about it, I'll happily make that change.

Show 18 quoted lines

>> @@ -1190,8 +1191,12 @@ struct ref_update *ref_transaction_add_update(
>>  		oidcpy(&update->new_oid, new_oid);
>>  	if ((flags & REF_HAVE_OLD) && old_oid)
>>  		oidcpy(&update->old_oid, old_oid);
>> -	if (!(flags & REF_SKIP_CREATE_REFLOG))
>> +	if (!(flags & REF_SKIP_CREATE_REFLOG)) {
>> +		if (committer_info)
>> +			update->committer_info = xstrdup(committer_info);
>> +
>>  		update->msg = normalize_reflog_message(msg);
>> +	}
>>
>>  	return update;
>>  }
>
> This can use `xstrdup_or_null()` and then drop the condition.
>
> Patrick

That is good improvement, will add.

Thanks!