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

[Patrick Steinhardt <ps@pks.im>]

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.

> @@ -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()`?

> @@ -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