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"?
> +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-<.
> +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.
> + 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.
> +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.
> + 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;
> +}
> +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.
> strbuf_release(&buf);
> strbuf_release(&buf2);
> strbuf_release(&buf3);
> + strbuf_release(&err);
> return result;
> }
