On Mon, Jul 28, 2025 at 10:19:53AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> > But more importantly it is also extremely inperformant. The number of
>
> Is "inperformant" a real word? "it performs extremely poorly"?
Well, in my head it is :) But it doesn't seem to exist anywhere else, so
I'll reword this.
> > +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-<.
We could name it `compute_renamed_refname()` to make it a bit more
verb-y.
> > +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.
Yeah, I don't love it, either.
> > +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 ...
>
> > + 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.
I was quite on the edge whether these buffers are really worth it in the
first place as an optimization -- I mostly adopted it from the migration
code. But I've benchmarked it now and couldn't really make out much of a
difference at all. So let's just drop all of this buffer reusing infra.
> > +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-<.
I dunno. I feel like this is something we should eventually fix.
Currently this is misleading and basically useless.
#leftoverbits
Patrick
