Re: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function
- From
karthik nayak <karthik.188@gmail.com>
- Date
- Dec 11, 2024, 18:06 UTC
- Message-ID
- <CAOLa=ZQFAMdW78QbPYSoCtqetwT1S6z8dmLS_XG6Ex0nUsp9Cg@mail.gmail.com>
- In-Reply-To
- <CAP8UFD1vTEH=7HBX1sudwkv7now9VUkFR3Y347EVNGfToZt8fA@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
[snip]
Show 30 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, > > 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. >
I think this is a great suggestion, it would reduce the congnitive load of the commit and make it easier to review. Will do.
Show 18 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);
>Indeed, I thought there was a better way. This is what I needed to have done.
Show 32 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);
>Much nicer, will add.
Show 19 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);
>Similar, will do.
Show 18 quoted lines
>> return -1; >> } > > [...] > >> 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. >
I think it was the auto linter, will remove.
Show 12 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".
Makes sense. Thanks.