Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 28, 2025, 17:19 UTC
- Message-ID
- <xmqqbjp4w7uu.fsf@gitster.g>
- In-Reply-To
- <20250728-pks-remote-rename-improvements-v1-3-f654f2b5c5ae@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> But more importantly it is also extremely inperformant. The number of
Is "inperformant" a real word? "it performs extremely poorly"?
Show 10 quoted lines
> +static void renamed_refname(struct rename_info *rename,
> + const char *refname,
> + struct strbuf *out)
> +{
> + strbuf_reset(out);
> + strbuf_addstr(out, refname);
> + strbuf_splice(out, strlen("refs/remotes/"), strlen(rename->old_name),
> + rename->new_name, strlen(rename->new_name));
> +}
> +The function name felt somewhat iffy (sounded as if you are letting a third-party know that you have renamed a ref), but I cannot come up with a better alternative X-<.
Show 8 quoted lines
> +static int rename_one_reflog_entry(const char *old_refname UNUSED,
> + struct object_id *old_oid,
> + struct object_id *new_oid,
> + const char *committer,
> + timestamp_t timestamp, int tz,
> + const char *msg, void *cb_data)
> {
> struct rename_info *rename = cb_data;Using a name of a system call for an unrelated variable, even if a local one in a function scope, makes me nauseous. Not a new problem introduced by this change, though.
Show 19 quoted lines
> + struct strbuf *identity = rename->buf1; > + struct strbuf *name = rename->buf2; > + struct strbuf *mail = rename->buf3; > + struct ident_split ident; > + const char *date; > + int error; > + > + if (split_ident_line(&ident, committer, strlen(committer)) < 0) > + return -1; > + > + strbuf_reset(name); > + strbuf_add(name, ident.name_begin, ident.name_end - ident.name_begin); > + strbuf_reset(mail); > + strbuf_add(mail, ident.mail_begin, ident.mail_end - ident.mail_begin); > + > + date = show_date(timestamp, tz, DATE_MODE(NORMAL)); > + strbuf_reset(identity); > + strbuf_addstr(identity, fmt_ident(name->buf, mail->buf, > + WANT_BLANK_IDENT, date, 0));
It is somewhat unfortunate that we need to do all of the above only so that we can recreate the full ident with the given committer with a timestamp that is given separately. This probably cannot be helped, though. The backend may not be keeping this information as a single string anyway.
Show 5 quoted lines
> +static int rename_one_reflog(const char *old_refname,
> + const struct object_id *old_oid,
> + struct rename_info *rename)
> +{
> + struct strbuf *message = rename->buf1;As these temporary strbuf's passed around as part of the rename_info structure are never released or recreated during the run, this is safe, but feels dirty, because we saw rename_one_reflog_entry() uses this exact one for totally different purpose. Perhaps it would make it easier to follow if you left "message" uninitialized here, before refs_for_each_reflog_ent() returns. And then ...
Show 18 quoted lines
> + int error; > + > + if (!refs_reflog_exists(get_main_ref_store(the_repository), old_refname)) > + return 0; > + > + error = refs_for_each_reflog_ent(get_main_ref_store(the_repository), > + old_refname, rename_one_reflog_entry, rename); > + if (error < 0) > + return error; > + > + /* > + * Manually write the reflog entry for the now-renamed ref. We cannot > + * rely on `rename_one_ref()` to do this for us as that would screw > + * over order in which reflog entries are being written. > + * > + * Furthermore, we only append the entry in case the reference > + * resolves. Missing references shouldn't have reflogs anyway. > + */
... give the "message" synonym to rename->buf1 here.
Show 12 quoted lines
> + strbuf_reset(message); > + strbuf_addf(message, "remote: renamed %s to %s", old_refname, > + rename->new_refname->buf); > + > + error = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf, > + old_oid, old_oid, git_committer_info(0), > + message->buf, rename->index++, rename->err); > + if (error < 0) > + return error; > + > + return error; > +}
Show 32 quoted lines
> +static int rename_one_ref(const char *old_refname, const char *referent,
> + const struct object_id *oid,
> + int flags, void *cb_data)
> +{
> + struct rename_info *rename = cb_data;
> + struct strbuf *new_referent = rename->buf1;
> + const char *ptr = old_refname;
> + int error;
> +
> + if (!skip_prefix(ptr, "refs/remotes/", &ptr) ||
> + !skip_prefix(ptr, rename->old_name, &ptr) ||
> + !skip_prefix(ptr, "/", &ptr)) {
> + error = 0;
> + goto out;
> }
> - strbuf_release(&buf);
>
> - return 0;
> + renamed_refname(rename, old_refname, rename->new_refname);
> +
> + if (flags & REF_ISSYMREF) {
> + /*
> + * Stupidly enough `referent` is not pointing to the immediate
> + * target of a symref, but it's the recursively resolved value.
> + * So symrefs pointing to symrefs would be misresolved, and
> + * unborn symrefs don't have any value for the `referent` at all.
> + */
> + referent = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> + old_refname, RESOLVE_REF_NO_RECURSE,
> + NULL, NULL);
> + renamed_refname(rename, referent, new_referent);
> + oid = NULL;Yuck, but this cannot be helped, I guess X-<.
> + struct rename_info rename = {
> + .buf1 = &buf,
> + .buf2 = &buf2,
> + .buf3 = &buf3,These can be embedded in the struct, not left as three separate strbuf instances whose addresses are known to this struct, no? We'd need to do strbuf_release() on them at the end anyway, so it would not be a huge deal, though.
Show 6 quoted lines
> strbuf_release(&buf); > strbuf_release(&buf2); > strbuf_release(&buf3); > + strbuf_release(&err); > return result; > }