Re: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Dec 11, 2024, 10:10 UTC
- Message-ID
- <CAP8UFD1vTEH=7HBX1sudwkv7now9VUkFR3Y347EVNGfToZt8fA@mail.gmail.com>
- In-Reply-To
- <20241209-320-git-refs-migrate-reflogs-v1-5-d4bc37ee860f@gmail.com>
On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote:
Show 10 quoted lines
> > Introduce a new function `ref_transaction_update_reflog`, for clients to > add a reflog update to a transaction. While the existing function > `ref_transaction_update` also allows clients to add a reflog entry, this > function does a few things more, It: > - Enforces that only a reflog entry is added and does not update the > ref itself. > - Allows the users to also provide the committer information. This > means clients can add reflog entries with custom committer > information.
Show 35 quoted lines
> A follow up commit will utilize this function to add reflog support to > `git refs migrate`. > > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > --- > refs.c | 89 +++++++++++++++++++++++++++++++++++++------------ > refs.h | 12 +++++++ > refs/files-backend.c | 48 +++++++++++++++----------- > refs/refs-internal.h | 16 +++++---- > refs/reftable-backend.c | 6 ++-- > 5 files changed, 122 insertions(+), 49 deletions(-) > > 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,
This change (adding a 'const char *committer_info' argument to ref_transaction_add_update()) is not described in the commit message and it requires a number of changes to the callers of this function, so I think it might want to be in its own preparatory commit before this one.
Show 14 quoted lines
> + const char *msg)
> {
> struct ref_update *update;
>
> @@ -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);Maybe:
update->committer_info = xstrdup(committer_info);Show 26 quoted lines
> + }
> +
> update->msg = normalize_reflog_message(msg);
> + }
>
> return update;
> }
> @@ -1199,20 +1208,29 @@ struct ref_update *ref_transaction_add_update(
> static int transaction_refname_verification(const char *refname,
> const struct object_id *new_oid,
> unsigned int flags,
> + unsigned int reflog,
> struct strbuf *err)
> {
> if (flags & REF_SKIP_REFNAME_VERIFICATION)
> return 0;
>
> if (is_pseudo_ref(refname)) {
> - strbuf_addf(err, _("refusing to update pseudoref '%s'"),
> - refname);
> + if (reflog)
> + strbuf_addf(err, _("refusing to update reflog for pseudoref '%s'"),
> + refname);
> + else
> + strbuf_addf(err, _("refusing to update pseudoref '%s'"),
> + refname);Maybe:
const char *what = reflog ? "reflog for pseudoref" : "pseudoref";
strbuf_addf(err, _("refusing to update %s '%s'"), what, refname);Show 12 quoted lines
> return -1;
> } else if ((new_oid && !is_null_oid(new_oid)) ?
> check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
> !refname_is_safe(refname)) {
> - strbuf_addf(err, _("refusing to update ref with bad name '%s'"),
> - refname);
> + if (reflog)
> + strbuf_addf(err, _("refusing to update reflog with bad name '%s'"),
> + refname);
> + else
> + strbuf_addf(err, _("refusing to update ref with bad name '%s'"),
> + refname);Maybe:
const char *what = reflog ? "reflog with bad name" :
"ref with bad name";
strbuf_addf(err, _("refusing to update %s '%s'"), what, refname);> return -1; > }
[...]
Show 9 quoted lines
> int ref_transaction_create(struct ref_transaction *transaction, > - const char *refname, > - const struct object_id *new_oid, > - const char *new_target, > - unsigned int flags, const char *msg, > - struct strbuf *err) > + const char *refname, const struct object_id *new_oid, > + const char *new_target, unsigned int flags, > + const char *msg, struct strbuf *err)
This looks like a wrapping or indenting only change. If you really want to do it, it should probably be in its own preparatory commit.
Show 10 quoted lines
> index a5bedf48cf6de91005a7e8d0bf58ca98350397a6..b86d2cd87be33f7bb1b31fce711d6c7c8d9491c9 100644 > --- a/refs.h > +++ b/refs.h > @@ -727,6 +727,18 @@ int ref_transaction_update(struct ref_transaction *transaction, > unsigned int flags, const char *msg, > struct strbuf *err); > > +/* > + * Similar to `ref_transaction_update`, but this function is only for adding > + * a reflog updates.
"a reflog update" or "reflog updates".