Re: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Dec 11, 2024, 14:26 UTC
- Message-ID
- <Z1mhCWXowokIaFR5@pks.im>
- In-Reply-To
- <20241209-320-git-refs-migrate-reflogs-v1-5-d4bc37ee860f@gmail.com>
On Mon, Dec 09, 2024 at 12:07:19PM +0100, Karthik Nayak wrote:
Show 27 quoted lines
> 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.
Show 10 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()`?
Show 16 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