Re: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function

From: karthik nayak <karthik.188@gmail.com>
Date: Dec 11, 2024, 18:09 UTC
Message-ID: <CAOLa=ZTLK=PL7YmkxzH5cZiHA21r1pM1qqbvu=fXi_rpRmHCYQ@mail.gmail.com>
In-Reply-To: <Z1mhCWXowokIaFR5@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 34 quoted lines

> On Mon, Dec 09, 2024 at 12:07:19PM +0100, Karthik Nayak wrote:
>> diff --git a/refs.c b/refs.c
>> index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction)
>>  	free(transaction);
>>  }
>>
>> -struct ref_update *ref_transaction_add_update(
>> -		struct ref_transaction *transaction,
>> -		const char *refname, unsigned int flags,
>> -		const struct object_id *new_oid,
>> -		const struct object_id *old_oid,
>> -		const char *new_target, const char *old_target,
>> -		const char *msg)
>> +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction,
>> +					      const char *refname,
>> +					      unsigned int flags,
>> +					      const struct object_id *new_oid,
>> +					      const struct object_id *old_oid,
>> +					      const char *new_target,
>> +					      const char *old_target,
>> +					      const char *committer_info,
>> +					      const char *msg)
>>  {
>>  	struct ref_update *update;
>>
>
> I'd personally avoid reindenting this block. It's somewhat-common
> practice to not align all arguments with the opening brace when the line
> would become too long. The reindents also distract a bit from the actual
> changes done in other places further down.
>

Makes sense, I'll undo that.

Show 13 quoted lines

>> @@ -1190,8 +1192,15 @@ 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) {
>> +			struct strbuf sb = STRBUF_INIT;
>> +			strbuf_addstr(&sb, committer_info);
>> +			update->committer_info = strbuf_detach(&sb, NULL);
>
> Can't we simplify this via `xstrdup()`?
>

Yup, Christian suggested the same too, will fix it up.

Show 23 quoted lines

>> @@ -3080,10 +3081,12 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
>>  		}
>>
>>  		/*
>> -		 * packed-refs don't support symbolic refs and root refs, so we
>> -		 * have to queue these references via the loose transaction.
>> +		 * packed-refs don't support symbolic refs, root refs and reflogs,
>> +		 * so we have to queue these references via the loose transaction.
>>  		 */
>> -		if (update->new_target || is_root_ref(update->refname)) {
>> +		if (update->new_target ||
>> +		    is_root_ref(update->refname) ||
>> +		    (update->flags & REF_LOG_ONLY)) {
>>  			if (!loose_transaction) {
>>  				loose_transaction = ref_store_transaction_begin(&refs->base, 0, err);
>>  				if (!loose_transaction) {
>
> Makes sense. While we already had REF_LOG_ONLY beforehand, it was only
> used in very specific cases and thus the support implemented by the
> backends is lacking. And given that the packed-ref backend does not
> support reflogs we have to queue these up via the loose backend.
>
> Patrick